[U-Boot-Users] PATCH: Add command support for multiple I2C controllers

[Ben Warren]

Nishanth,

Thanks for the thorough, constructive feedback.  Comments below.

regards,
Ben

On Wed, 2006-05-17 at 18:28 -0500, Menon, Nishanth wrote:

> Hi Ben,
> Thanks for the patch, I think we are in the right direction.
> 
> Here are my comments:
> +#if defined(CFG_I2C_NOPROBES) && defined(CFG_I2C_MULTI_NOPROBES)
> +#error "Only one of CFG_I2C_NOPROBES or CFG_I2C_MULTI_NOPROBES may be
> used"
> +#endif
> Multiprobe needs sanity check with CONFIG_I2C_MULTI_BUS..

Good idea.  The best kind of bug is one that's caught by the compiler!

> 
> #if defined(CFG_I2C_NOPROBES)
>  static uchar i2c_no_probes[] = CFG_I2C_NOPROBES;
> +static uchar *pNoProbes = i2c_no_probes;
> +#elif defined(CFG_I2C_MULTI_NOPROBES)
> +static uchar i2c_no_probes[] = CFG_I2C_MULTI_NOPROBES;
> +static uchar *pNoProbes = i2c_no_probes;
>  #endif
> 
> One problem: On 10 bit addressing mode, uchar is not enough to define
> the address. This is present through out the cmd_i2c.c!

I've never used 10-bit addressing, but sure enough - there it is in the
Philips spec!  I guess we should consider moving everything to a ushort.

> 
> Line 556
> +		return 0;
> Return error value
> 
> #if defined(CFG_I2C_NOPROBES) || defined(CFG_I2C_MULTI_NOPROBES)     
>                 skip = 0;                                            
>                 for(k=0; ((k < sizeof(i2c_no_probes)) && (*(pNoProbes
>                 {                                                    
>                         if(*(pNoProbes + k) == j)                    
>                         {                                            
> The scan for "don't probe addresses" stops at the first 0xff in the
> pNoProbes array. This logic is fine for the first bus, how does it
> handle the subsequent busses? I do "ibus 2 400" and then do a probe, how
> will it pick up the non-probe-able addresses of bus 2?
> 
> I think since the context of the bus is known only to the platform
> driver, the usage of the same should be passed on there/do something of
> the following logic:
> 1. in ibus, store bus index (say bidx) in cmd_i2c.c
> 2. in iprobe, 
> 	a) use a local pNoProbes which is indexed at the (bidx -1)th 
> 	   occurance of 0xFF in the global pNoProbes
> 	b) use the rest of the scan logic..


I'm glad you delved a bit deeper (in other e-mail) and found that this
really does work.  

> 
> Additional command for user to get i2c bus stats: ibi (i2c bus info)
> which will call i2c_get_bus_num and i2c_get_bus_speed and provide users.

I'm not sure we need this, since you can get the same info using ibus
with no arguments.  Maybe if we do make an I2C command tree as Wolfgang
has requested, another command would make sense.

> 
> Regards,
> Nishanth Menon
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.denx.de/pipermail/u-boot/attachments/20060518/34096e43/attachment.htm