Re: [U-Boot-Users] PATCH: Add command support for multiple I2C controllers
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
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
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
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
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).
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
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
participants (4)
-
Ben Warren -
Tolunay Orkun -
Travis B. Sawyer -
Wolfgang Denk