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

[Vitaly Wool]

Jean,
>>> When sending more than one 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.
>>     
>
> No, stop is also sent at the end of every message (either received or
> transmitted):
>
> 		if (alg_data->mif.len == 1) {
> 			/* Last byte, issue a stop */
> 			val |= stop_bit;
> 		}
>
> Which is OK if it is the last message, but not if there are more
> messages to follow. Is the technical documentation for this chip
> available? Please see if it mentions repeated start. Maybe you need to
> set both start_bit and stop_bit to generate a repeated start condition.
>
> What testing did this driver receive so far?
>   
It's used for audio volume control, power management chip (PNX010x) and 
USB configuration (PNX4008).
It was also tested with simple EEPROM on both.
>   
>>>> +	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?
>>     
>
> Only if marked __init, and this isn't the case here.
>   
Ah, ok, I missed __initdata in there. I'd rather set it in 
arch/arm/mach-pnx4008/i2c.c as it seems to be completely wrong to rely 
on  the assumption that no other pnx target's i2c arch-specific init 
won't do that.
>   
>>>> +	iounmap((void *)alg_data->ioaddr);
>>>>         
>>> Unnecessary cast?
>>>       
>> ioaddr is u32
>>     
>
> Ah, I missed that, good point. Now this raises another question, why is
> it an u32 in the first place, rather than the void* I'd expect?
>   
It was done in order not to have reverse casts for I2C_REG_xxx macros.
>   
>>>> +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.
>>     
>
> OK, but this driver can still be selected as a module. What if the USB
> part is built-in, or also built as a module but loaded first? What about
> other platforms which wouldn't need this early init?
>   
I don't care too much about wrong module loading sequence.
As of other... Hmm, what about a) adding #ifndef  MODULE for 
subsys_initcall; b) adding a config option; c) adding a cmdline parameter?
>   
>>>> +	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.
>>     
>
> How is it incompatible with moving their definitions in i2c-pnx.c?
>   
It's not. :) Sorry.

Vitaly