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

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

[Ben Warren]

Wolfgang,

Thanks for the feedback.  Comments below.

regards,
Ben

On Thu, 2006-05-18 at 00:52 +0200, Wolfgang Denk wrote:
> In message <1147893695.16780.239.camel@saruman.qstreams.net> you wrote:
> > 
> > Attached is a second stab at a patch to allow access to multiple I2C
> > controllers.  It enhances the command set by adding the following
> > command, which changes the 'current' I2C bus.  Further write, read,
> > probe etc. commands will deal with the newly selected bus:
> > 
> > ibus [bus_index] [speed in Hz]
> 
> No. Please read my posting again. I want to see this as compatible as
> possible with other commands that operate  on  devices  connected  to
> busses. We use "ide dev ...", so I want to see "i2c dev" here, too.
> 
> As mentioned before, in the long term all i2c related commands should
> become sub-commands to the new "i2c" command.
> 
I understand and agree with your reasoning for moving all I2C commands
to a separate tree.  On the other hand, I'm very focused on your goal of
keeping the interface small and ALWAYS maintaining backwards
compatibility.  If you'd like me to move all I2C commands to a separate
tree, it should be trivial, but makes for a bigger package.  Please
advise.  
> > In addition, the logic for ignoring I2C devices during a probe command
> > has been enhanced to work with multiple controllers.  The software has
> > been tested with new and old configurations (i.e. is backwards
> > compatible).
> > 
> > Old way of ignoring devices:
> > 
> > #define CFG_I2C_NOPROBES {0x11, 0x22}
> > 
> > New way:
> > 
> > #define CFG_I2C_MULTI_NOPROBES {0x11, 0x22, 0xff, 0x33, 0x44, 0xff}
> 
> Argh... This is pretty unreadable. Can';t we just  use  an  array  of
> lists?
> 
I agree that at first glance this is unreadable.  However, it is quite
efficient and works well with macros.  I played around with lots of
different data structures, but I've usually found multi-dimensional
arrays in C to be more trouble than they're worth, especially if you're
using macros and don't know the size ahead of time.  Keeping things as a
1-D array allows the use of sizeof() to determine the size of the list,
not just the size of the pointer.  Maybe you can teach me something here
- even though I've been using C for many years, there are always new
things to learn...  Here's a simpler approach that you may think is OK:
	
#define I2C_DELIM  /* or something like that */
#define CFG_I2C_MULTI_NOPROBES {0x11, 0x22, I2C_DELIM, 0x33, 0x44 ...}


> Best regards,
> 
> Wolfgang Denk
> 

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

[Wolfgang Denk]

Dear Ben,

in message <1147960283.16780.263.camel@saruman.qstreams.net> you wrote:
> 
> Thanks for the feedback.  Comments below.

Can you please  stop  top-posting  and  delete  irrelevant  parts  of
previous messages? Thanks.

> > No. Please read my posting again. I want to see this as compatible as
> > possible with other commands that operate  on  devices  connected  to
> > busses. We use "ide dev ...", so I want to see "i2c dev" here, too.
> > 
> > As mentioned before, in the long term all i2c related commands should
> > become sub-commands to the new "i2c" command.
> > 
> I understand and agree with your reasoning for moving all I2C commands
> to a separate tree.  On the other hand, I'm very focused on your goal of
> keeping the interface small and ALWAYS maintaining backwards
> compatibility.  If you'd like me to move all I2C commands to a separate

Yes, me too. But I don't see how adding a new "i2c dev" would disturb
backward compatibility?

> tree, it should be trivial, but makes for a bigger package.  Please
> advise.  

I'm not convinced that the new sheme will  be  significantly  bigger.
Yes,  for the transition period (when we support both the old and the
new sytax) code  will  be  bigger.  But  me  might  even  #ifdef  the
compatibility calls out....

> I agree that at first glance this is unreadable.  However, it is quite
> efficient and works well with macros.  I played around with lots of

Macros may be evil.

> #define I2C_DELIM  /* or something like that */
> #define CFG_I2C_MULTI_NOPROBES {0x11, 0x22, I2C_DELIM, 0x33, 0x44 ...}

That doesn't make it more readable. Also, how often are you going  to
use that macro in your code?

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
To the systems programmer,  users  and  applications  serve  only  to
provide a test load.

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

[Ben Warren]

Dear Wolfgang,

> Can you please  stop  top-posting  and  delete  irrelevant  parts  of
> previous messages? Thanks.
OK

> 
> Yes, me too. But I don't see how adding a new "i2c dev" would disturb
> backward compatibility?
If you'd prefer, I can certainly rename 'ibus' to 'i2c dev', while
keeping everything else intact.
> 

> I'm not convinced that the new sheme will  be  significantly  bigger.
> Yes,  for the transition period (when we support both the old and the
> new sytax) code  will  be  bigger.  But  me  might  even  #ifdef  the
> compatibility calls out....
How about something like:

#ifndef(CONFIG_I2C_COMMAND_TREE)
existing U_BOOT_CMD stuff
#else
new I2C command tree
#endif

> 
> Macros may be evil.
Amen
> 
> > #define I2C_DELIM  /* or something like that */
> > #define CFG_I2C_MULTI_NOPROBES {0x11, 0x22, I2C_DELIM, 0x33, 0x44 ...}
> 
> That doesn't make it more readable. Also, how often are you going  to
> use that macro in your code?
> 
This macro (I forgot to assign a value, by the way) is only used in two
commands - the probing function and the one that changes buses (it moves
a pointer to the correct point in the list).  You'll notice that I have
both compile time and run time checks in place that verify that the list
is properly formed, and hopefully enough comments to show how to create
the list.  I'm very open to alternative suggestions, other than 'no'.
> Best regards,
> 
> Wolfgang Denk
> 
I've spent a fair amount of time writing and testing code that I hope
will benefit others.  I'm afraid if you feel a complete re-write is
necessary, somebody else will have to do it.  On the other hand, if you
feel that this is at least an incremental improvement over the existing
code, and are willing to entertain the notion of adding it to U-boot,
I'll gladly continue.  Please advise.

regards,
Ben

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

[Wolfgang Denk]

In message <1147971865.16780.310.camel@saruman.qstreams.net> you wrote:
> 
> #ifndef(CONFIG_I2C_COMMAND_TREE)
> existing U_BOOT_CMD stuff
> #else
> new I2C command tree
> #endif

No, because this will always be mutually exclusie. I thinkt hat,  for
a  transition  period  and  on  sytems  that  can  afford  the memory
footprint, both the old and the new syntac should be available at the
same time (just  like  we  still  support  the  $(variable)  and  the
${variable} formats in parallel - but $(var) will be dropped RSN).

> This macro (I forgot to assign a value, by the way) is only used in two
> commands - the probing function and the one that changes buses (it moves
> a pointer to the correct point in the list).  You'll notice that I have
> both compile time and run time checks in place that verify that the list

Yes, but this and the fact that multiple instances of the  macro  are
used means that we add more than needed to the memory footprint.

> is properly formed, and hopefully enough comments to show how to create
> the list.  I'm very open to alternative suggestions, other than 'no'.

Use arrays, please.

> I've spent a fair amount of time writing and testing code that I hope
> will benefit others.  I'm afraid if you feel a complete re-write is
> necessary, somebody else will have to do it.  On the other hand, if you

I'm sorry if my continuous rejection of your work is frustrating you.
I apologize. It  is  not  my  intention  to  make  life  harder  than
necessary  to  you.  But I think when we change this, we should do it
right.

> feel that this is at least an incremental improvement over the existing
> code, and are willing to entertain the notion of adding it to U-boot,
> I'll gladly continue.  Please advise.

I definitely see an improvement with each iteration, and  my  feeling
is  that  you  understand pretty well what I mean. I encourage you to
continue. Please go on.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Q:  Why do mountain climbers rope themselves together?
A:  To prevent the sensible ones from going home.

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

[Travis B. Sawyer]

Wolfgang Denk wrote:

>I definitely see an improvement with each iteration, and  my  feeling
>is  that  you  understand pretty well what I mean. I encourage you to
>continue. Please go on.
>
>Best regards,
>
>Wolfgang Denk
>
>  
>
Yes, do please continue.  I currently have a hackish implementation that 
is used only on
the sandburst boards, as I didn't want to pollute the tree making 
others' images larger.
I added code for the 2nd i2c bus on the ppc440gx, as our boards use the 
2nd bus for
temp sensors/fan control and wanted the fans turned on during boot.  If 
the OS doesn't
load for some reason the board would cook as the fans weren't being 
touched until the
kernel started.

I'm v.interested in how this turns out.

Thanks

Travis Sawyer
(Broadcom Corp, formerly Sandburst Corp).

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

[Tolunay Orkun]

Travis B. Sawyer wrote:
> Wolfgang Denk wrote:
> 
>> I definitely see an improvement with each iteration, and  my  feeling
>> is  that  you  understand pretty well what I mean. I encourage you to
>> continue. Please go on.
>>
>> Best regards,
>>
>> Wolfgang Denk
>>
>>  
>>
> Yes, do please continue.  I currently have a hackish implementation that 
> is used only on
> the sandburst boards, as I didn't want to pollute the tree making 
> others' images larger.

Me too. :) I too would benefit from this as the board that I am working 
(405EP based) has 2 I2C busses; The regular hardware i2c bus and the 
soft i2c using 2 GPIO pins. I too would like to do initialization in 
U-Boot on the soft I2C bus. Standard support for multiple I2C busses 
would be great.

Best regards,
Tolunay

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

[Ben Warren]

Wolfgang,

> No, because this will always be mutually exclusie. I thinkt hat,  for
> a  transition  period  and  on  sytems  that  can  afford  the memory
> footprint, both the old and the new syntac should be available at the
> same time (just  like  we  still  support  the  $(variable)  and  the
> ${variable} formats in parallel - but $(var) will be dropped RSN).
OK
> 
> > This macro (I forgot to assign a value, by the way) is only used in two
> > commands - the probing function and the one that changes buses (it moves
> > a pointer to the correct point in the list).  You'll notice that I have
> > both compile time and run time checks in place that verify that the list
> 
> Yes, but this and the fact that multiple instances of the  macro  are
> used means that we add more than needed to the memory footprint.
> 
> > is properly formed, and hopefully enough comments to show how to create
> > the list.  I'm very open to alternative suggestions, other than 'no'.
> 
> Use arrays, please.
> 
Sorry, I mis-stated this.  The list defined by the macro is only stored
in one place - the 1-D array called 'i2c_no_probes'.  Manipulation is
purely by pointers, so this isn't a memory hog.  I would much prefer to
use a 2-D array, but the C language doesn't allow the initialization of
static 2-D arrays with an arbitrary number of columns (representing
entries in the list).  I can easily define:

#define CFG_I2C_NUM_BUSES    2
#define CFG_I2C_MULTI_NOPROBES {{0x1, 0x2, 0x3}, {0xa, 0xb}}

but the following won't compile:

static uchar [CFG_I2C_NUM_BUSES][] = CFG_I2C_MULTI_NOPROBES;

I could add yet another #define, perhaps  #define CFG_I2C_MAX_NOPROBES 3

static uchar [CFG_I2C_NUM_BUSES][CFG_I2C_MAX_NOPROBES] =
CFG_I2C_MULTI_NOPROBES;

which will work, but isn't very flexible and adds yet another thing the
user must think about.  Of course, we could #define a 1-D array for each
bus, but as you can see this grows ungainly.  My approach of a single
delimited 1-D array has the advantage of being simple, and allows an
arbitrary number of controllers and no-probes in each one, while only
requiring a single #define CFG.

Sorry if this is overly verbose.

regards,
Ben

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

[Menon, Nishanth]

Hi Ben,

> #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
Just relooked at your patched and realized that I was wrong.. my
apologies.. Trouble with scanning the code with a hood on brains :(
 
Regards,
Nishanth Menon

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

[Menon, Nishanth]

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..

#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!

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@the (bidx -1)th 
	   occurance of 0xFF in the global pNoProbes
	b) use the rest of the scan logic..

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.

Regards,
Nishanth Menon

[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 

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

[Wolfgang Denk]

In message <1147961239.16780.270.camel@saruman.qstreams.net> you wrote:
> 
> > 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.

We don't need another command. "i2c dev arg ..." will set the current
bus, and "i2c dev" without arguments  wiull  print  the  current  bus
settings, like with the existing commands.

Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
War isn't a good life, but it's life.
	-- Kirk, "A Private Little War", stardate 4211.8

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

[Ben Warren]

Hello,

Attached is a second stab at a patch to allow access to multiple I2C
controllers.  It enhances the command set by adding the following
command, which changes the 'current' I2C bus.  Further write, read,
probe etc. commands will deal with the newly selected bus:

ibus [bus_index] [speed in Hz]

In addition, the logic for ignoring I2C devices during a probe command
has been enhanced to work with multiple controllers.  The software has
been tested with new and old configurations (i.e. is backwards
compatible).

Old way of ignoring devices:

#define CFG_I2C_NOPROBES {0x11, 0x22}

New way:

#define CFG_I2C_MULTI_NOPROBES {0x11, 0x22, 0xff, 0x33, 0x44, 0xff}

where controller lists are delimited by '0xff', which is an invalid I2C
address.

Note: In order to use, drivers must be modified to implement the
following API extensions:

int	i2c_set_bus(uchar bus, int speed);
uchar	i2c_get_bus_num(void);
int	i2c_get_bus_speed(void);

Code is conditionally compiled using:

CONFIG_I2C_MULTI_BUS
CFG_I2C_MULTI_NOPROBES

CHANGELOG:
	If CONFIG_I2C_MULTI_BUS is defined, the 'ibus' command becomes
available, allowing the user to switch between I2C buses and to change
bus speeds.  Additionally, devices may be excluded from being probed by
defining a CFG_I2C_MULTI_NOPROBES array, where lists for each controller
are delimited by '0xff'

Please have a look and feed back any suggestions.

regards,
Ben


-------------- next part --------------
A non-text attachment was scrubbed...
Name: I2C_MULTIBUS.patch
Type: text/x-patch
Size: 5032 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20060517/704f7a3c/attachment.bin 

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

[Wolfgang Denk]

In message <1147893695.16780.239.camel@saruman.qstreams.net> you wrote:
> 
> Attached is a second stab at a patch to allow access to multiple I2C
> controllers.  It enhances the command set by adding the following
> command, which changes the 'current' I2C bus.  Further write, read,
> probe etc. commands will deal with the newly selected bus:
> 
> ibus [bus_index] [speed in Hz]

No. Please read my posting again. I want to see this as compatible as
possible with other commands that operate  on  devices  connected  to
busses. We use "ide dev ...", so I want to see "i2c dev" here, too.

As mentioned before, in the long term all i2c related commands should
become sub-commands to the new "i2c" command.

> In addition, the logic for ignoring I2C devices during a probe command
> has been enhanced to work with multiple controllers.  The software has
> been tested with new and old configurations (i.e. is backwards
> compatible).
> 
> Old way of ignoring devices:
> 
> #define CFG_I2C_NOPROBES {0x11, 0x22}
> 
> New way:
> 
> #define CFG_I2C_MULTI_NOPROBES {0x11, 0x22, 0xff, 0x33, 0x44, 0xff}

Argh... This is pretty unreadable. Can';t we just  use  an  array  of
lists?


Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Things are not as simple as they seem at first.        - Edward Thorp