<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 TRANSITIONAL//EN">
<HTML>
<HEAD>
<META HTTP-EQUIV="Content-Type" CONTENT="text/html; CHARSET=UTF-8">
<META NAME="GENERATOR" CONTENT="GtkHTML/3.8.1">
</HEAD>
<BODY>
Nishanth,<BR>
<BR>
Thanks for the thorough, constructive feedback. Comments below.<BR>
<BR>
regards,<BR>
Ben<BR>
<BR>
On Wed, 2006-05-17 at 18:28 -0500, Menon, Nishanth wrote:
<BLOCKQUOTE TYPE=CITE>
<PRE>
<FONT COLOR="#000000">Hi Ben,</FONT>
<FONT COLOR="#000000">Thanks for the patch, I think we are in the right direction.</FONT>
<FONT COLOR="#000000">Here are my comments:</FONT>
<FONT COLOR="#000000">+#if defined(CFG_I2C_NOPROBES) && defined(CFG_I2C_MULTI_NOPROBES)</FONT>
<FONT COLOR="#000000">+#error "Only one of CFG_I2C_NOPROBES or CFG_I2C_MULTI_NOPROBES may be</FONT>
<FONT COLOR="#000000">used"</FONT>
<FONT COLOR="#000000">+#endif</FONT>
<FONT COLOR="#000000">Multiprobe needs sanity check with CONFIG_I2C_MULTI_BUS..</FONT>
</PRE>
</BLOCKQUOTE>
Good idea. The best kind of bug is one that's caught by the compiler!
<BLOCKQUOTE TYPE=CITE>
<PRE>
<FONT COLOR="#000000">#if defined(CFG_I2C_NOPROBES)</FONT>
<FONT COLOR="#000000"> static uchar i2c_no_probes[] = CFG_I2C_NOPROBES;</FONT>
<FONT COLOR="#000000">+static uchar *pNoProbes = i2c_no_probes;</FONT>
<FONT COLOR="#000000">+#elif defined(CFG_I2C_MULTI_NOPROBES)</FONT>
<FONT COLOR="#000000">+static uchar i2c_no_probes[] = CFG_I2C_MULTI_NOPROBES;</FONT>
<FONT COLOR="#000000">+static uchar *pNoProbes = i2c_no_probes;</FONT>
<FONT COLOR="#000000"> #endif</FONT>
<FONT COLOR="#000000">One problem: On 10 bit addressing mode, uchar is not enough to define</FONT>
<FONT COLOR="#000000">the address. This is present through out the cmd_i2c.c!</FONT>
</PRE>
</BLOCKQUOTE>
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.
<BLOCKQUOTE TYPE=CITE>
<PRE>
<FONT COLOR="#000000">Line 556</FONT>
<FONT COLOR="#000000">+ return 0;</FONT>
<FONT COLOR="#000000">Return error value</FONT>
<FONT COLOR="#000000">#if defined(CFG_I2C_NOPROBES) || defined(CFG_I2C_MULTI_NOPROBES) </FONT>
<FONT COLOR="#000000"> skip = 0; </FONT>
<FONT COLOR="#000000"> for(k=0; ((k < sizeof(i2c_no_probes)) && (*(pNoProbes</FONT>
<FONT COLOR="#000000"> { </FONT>
<FONT COLOR="#000000"> if(*(pNoProbes + k) == j) </FONT>
<FONT COLOR="#000000"> { </FONT>
<FONT COLOR="#000000">The scan for "don't probe addresses" stops at the first 0xff in the</FONT>
<FONT COLOR="#000000">pNoProbes array. This logic is fine for the first bus, how does it</FONT>
<FONT COLOR="#000000">handle the subsequent busses? I do "ibus 2 400" and then do a probe, how</FONT>
<FONT COLOR="#000000">will it pick up the non-probe-able addresses of bus 2?</FONT>
<FONT COLOR="#000000">I think since the context of the bus is known only to the platform</FONT>
<FONT COLOR="#000000">driver, the usage of the same should be passed on there/do something of</FONT>
<FONT COLOR="#000000">the following logic:</FONT>
<FONT COLOR="#000000">1. in ibus, store bus index (say bidx) in cmd_i2c.c</FONT>
<FONT COLOR="#000000">2. in iprobe, </FONT>
<FONT COLOR="#000000"> a) use a local pNoProbes which is indexed at the (bidx -1)th </FONT>
<FONT COLOR="#000000"> occurance of 0xFF in the global pNoProbes</FONT>
<FONT COLOR="#000000"> b) use the rest of the scan logic..</FONT>
</PRE>
</BLOCKQUOTE>
<BR>
I'm glad you delved a bit deeper (in other e-mail) and found that this really does work.
<BLOCKQUOTE TYPE=CITE>
<PRE>
<FONT COLOR="#000000">Additional command for user to get i2c bus stats: ibi (i2c bus info)</FONT>
<FONT COLOR="#000000">which will call i2c_get_bus_num and i2c_get_bus_speed and provide users.</FONT>
</PRE>
</BLOCKQUOTE>
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.
<BLOCKQUOTE TYPE=CITE>
<PRE>
<FONT COLOR="#000000">Regards,</FONT>
<FONT COLOR="#000000">Nishanth Menon</FONT>
</PRE>
</BLOCKQUOTE>
</BODY>
</HTML>