[i2c] [PATCH] I2C bus driver for Philips ARM boards

[Vitaly Wool]

Hello Jean,

thanks for the thourough and comprehensive review!
I'd agree with most of your comments/proposals, but there're some I'd 
like to argue/comment a bit.
> The name "i2c-pnx" is a bit too generic IMHO. What if another PNX board
> is later released with a different I2C implementation?
>   
All Philips boards that either use or going to use Linux have very 
similar I2C IP blocks. Everything that might differ is put into 
arch-specific.
>> + *
>> + * Provides i2c support for PNX010x/PNX4008.
>>     
>
> Mentioning that these are Philips board would be welcome.
>   
Ok, though I can't help saying that P stands for Philips in 'PNX'. ;-)
> You could use I2C_SMBUS_READ and I2C_SMBUS_WRITE instead of inventing
> your own rw_bit.
>   
rw_bit is a control bit of Tx register, so I don't think it's really 
reasonable here.

>> +		if ((stat = ioread32(I2C_REG_STS(alg_data))) & mstatus_tdi) {
>> +			printk(KERN_WARNING
>> +				"%s: TDI still set ... clearing now.\n",
>> +			       adap->name);
>> +			stat |= mstatus_tdi;
>>     
>
> This doesn't seem to have any effect, as you just checked that this bit
> was already set.
>   
No, it's cleared by writing a '1' to it.
>> +			iowrite32(stat, I2C_REG_STS(alg_data));
>> +		}
>> +		if ((stat = ioread32(I2C_REG_STS(alg_data))) & mstatus_afi) {
>> +			printk(KERN_WARNING
>> +				"%s: AFI still set ... clearing now.\n",
>> +			       adap->name);
>> +			stat |= mstatus_afi;
>>     
>
> Same here.
>
>   
Ditto :)
>
>   
>> +
>> +	/* Cleanup to be sure ... */
>> +	alg_data->mif.buf = NULL;
>> +	alg_data->mif.len = 0;
>>     
>
> Useless as far as I can see, but well...
>
>   
>> +
>> +	/* Release sem */
>> +	up(&alg_data->mif.sem);
>> +	dev_dbg(&adap->dev, "%s(): exiting, stat = %x\n",
>> +		__FUNCTION__, ioread32(I2C_REG_STS(alg_data)));
>> +
>> +	if (completed != num)
>> +		return ((rc < 0) ? rc : -EREMOTEIO);
>> +
>> +	return num;
>> +}
>>     
>
> When sending more than once message at once, the I2C protocol says
> that you should have a "repeated start" condition between messages. It
> seems that your driver is instead sending a stop condition followed by
> a start condition, which is different. Please check the hardware
> documentation for your device and the I2C specification, you are
> probably not sending the correct signals on the wire (could be checked
> with an oscilloscope.) It may still work in some cases, but some
> devices may not accept the alternative (start/stop) formatting.
>   
I don't think that I'm following you here. AFAI can see, stop is issued 
only for zero-sized transfers or when a NAK is received what complied to 
the IP block interface specification.
>> +	i2c_pnx->suspend = plat_data->suspend;
>> +	i2c_pnx->resume = plat_data->resume;
>> +	i2c_pnx->calculate_input_freq = plat_data->calculate_input_freq;
>> +	i2c_pnx->set_clock_run = plat_data->set_clock_run;
>> +	i2c_pnx->set_clock_stop = plat_data->set_clock_stop;
>>     
>
> Why don't you simply store a pointer to the platform data, rather than
> duplicating every entry?
>   
Hmmm, isn't it released after bootup?
>   
>> +	i2c_pnx->adapter = plat_data->adapter;
>> +	platform_set_drvdata(pdev, i2c_pnx);
>> +
>> +	if (i2c_pnx->calculate_input_freq)
>> +		freq_mhz = i2c_pnx->calculate_input_freq(pdev);
>> +	else {
>> +		freq_mhz = 13;
>>     
>
> Maybe define e.g. PNX_DEFAULT_FREQ at the top of the file?
>   
>> +	 * the deglitching filter length. In this example, n = 7, since
>> +	 * eight bits are needed to hold the clock divider count.
>> +	 */
>>     
>
> I don't understand the explanation, especially the last sentence. How
> do you go from 98 to 7?
>   
Oops, sorry, 'n' parameter is not used any more. Anyway, this was not 
the same thing as the one that should be 98 :)
>> +	iounmap((void *)alg_data->ioaddr);
>>     
>
> Unnecessary cast?
>   
ioaddr is u32

>> +MODULE_LICENSE("GPL");
>> +
>> +subsys_initcall(i2c_adap_pnx_init);
>>     
>
> Why not simply module_init?
>   
I2C needs to be initialized before USB, as USB IP block is configured 
via I2C.
>> +	struct i2c_pnx_mif	mif;
>> +};
>> +
>> +#define TIMEOUT			(2*(HZ))
>> +#define I2C_PNX_SPEED_KHZ	100
>> +#define I2C_BLOCK_SIZE		0x100
>>     
>
> "I2C_BLOCK_SIZE" is way too generic for something which is specific to
> your device. Also, it's the size of the I/O region, so "BLOCK" doesn't
> fit very well. I2C_PNX_REGION_SIZE maybe?
>
> This is also an implementation detail, platform doesn't need to know,
> so this define should be moved to the driver itself (i2c-pnx.c). Same
> for I2C_PNX_SPEED_KHZ.
>   
No, these two depend more on the IP block than on the implementation for 
a particular board.
>> +
>> +void __init pnx4008_register_i2c_devices(void)
>> +{
>> +	platform_add_devices(devices, ARRAY_SIZE(devices));
>> +}
>>     
>
> This function not called anywhere?
>   
It's gonna be called from arch/arm/mach-pnx4008/core.c which is not 
included into this patch as it will go to rmk for approval. :)

Best regards,
   Vitaly