[U-Boot-Users] PATCH: Add command support for second I2C controller

[U-Boot-Users] PATCH: Add command support for second I2C controller

[Ben Warren]

Hello,

Attached is a patch to common/cmd_i2c.c that allows access to two I2C
controllers on a board.  Note that this doesn't actually change any
hardware control - it just enhances the command set and passes more
information to whatever version of i2c_read(), i2c_write() etc. that
you're using.  I've implemented driver changes on MPC8349 hardware, but
they're not quite ready for review yet.  New definitions:
CONFIG_I2C_2_CTRLS - board has two I2C controllers
CFG_I2C2_NOPROBES {} - list of devices on bus 2 to ignore when probing

CHANGELOG: 
	If CONFIG_I2C_2_CTRLS is defined, the 'chip' parameter of all I2C
commands will accept an optional controller argument.    
e.g.  'imd 50.1 0' displays data at offset 0 of controller 1 device 50
      'imd 50.2 0' displays data at offset 0 of controller 2 device 50
      'iprobe 2' probes for devices on the second bus

regards,
Ben
-------------- next part --------------
A non-text attachment was scrubbed...
Name: CMD_I2C.patch
Type: text/x-patch
Size: 12679 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20060515/5eb0fa18/attachment.bin 

[U-Boot-Users] PATCH: Add command support for second I2C controller

[Wolfgang Denk]

In message <1147723625.16780.140.camel@saruman.qstreams.net> you wrote:
> 
> Attached is a patch to common/cmd_i2c.c that allows access to two I2C
> controllers on a board.  Note that this doesn't actually change any
> hardware control - it just enhances the command set and passes more
> information to whatever version of i2c_read(), i2c_write() etc. that
> you're using.  I've implemented driver changes on MPC8349 hardware, but
> they're not quite ready for review yet.  New definitions:
> CONFIG_I2C_2_CTRLS - board has two I2C controllers
> CFG_I2C2_NOPROBES {} - list of devices on bus 2 to ignore when probing
> 
> CHANGELOG: 
> 	If CONFIG_I2C_2_CTRLS is defined, the 'chip' parameter of all I2C
> commands will accept an optional controller argument.    
> e.g.  'imd 50.1 0' displays data at offset 0 of controller 1 device 50
>       'imd 50.2 0' displays data at offset 0 of controller 2 device 50
>       'iprobe 2' probes for devices on the second bus

I reject this patch.

As discussed before, I don't like the command  format.  Second,  what
happens  if  there comes a board with 3 I2C busses? Then we touch the
code gain... No, thanks.

Also note that your patch has trailing white space,  so  it  violates
the coding standard.

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
A rolling stone gathers momentum.

[U-Boot-Users] PATCH: Add command support for second I2C controller

[Ben Warren]

Thanks Wolfgang.  

I have no idea how you got this again - strange things are afoot with
the sourceforge mailing list.  Anyway, I'll be on the lookout for
traling white space in future submissions.

regards,
Ben

On Thu, 2006-05-18 at 00:24 +0200, Wolfgang Denk wrote:

> In message <1147723625.16780.140.camel@saruman.qstreams.net> you wrote:
> > 
> > Attached is a patch to common/cmd_i2c.c that allows access to two I2C
> > controllers on a board.  Note that this doesn't actually change any
> > hardware control - it just enhances the command set and passes more
> > information to whatever version of i2c_read(), i2c_write() etc. that
> > you're using.  I've implemented driver changes on MPC8349 hardware, but
> > they're not quite ready for review yet.  New definitions:
> > CONFIG_I2C_2_CTRLS - board has two I2C controllers
> > CFG_I2C2_NOPROBES {} - list of devices on bus 2 to ignore when probing
> > 
> > CHANGELOG: 
> > 	If CONFIG_I2C_2_CTRLS is defined, the 'chip' parameter of all I2C
> > commands will accept an optional controller argument.    
> > e.g.  'imd 50.1 0' displays data at offset 0 of controller 1 device 50
> >       'imd 50.2 0' displays data at offset 0 of controller 2 device 50
> >       'iprobe 2' probes for devices on the second bus
> 
> I reject this patch.
> 
> As discussed before, I don't like the command  format.  Second,  what
> happens  if  there comes a board with 3 I2C busses? Then we touch the
> code gain... No, thanks.
> 
> Also note that your patch has trailing white space,  so  it  violates
> the coding standard.
> 
> Best regards,
> 
> Wolfgang Denk
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.denx.de/pipermail/u-boot/attachments/20060518/f9e499e6/attachment.htm 

[U-Boot-Users] PATCH: Add command support for second I2C controller

[Ben Warren]

Hello,

Please ignore the previous patch - it contained a typo that would break
the compile.  Sorry... really - I did test this a lot but must have hit
a wrong key sequence in vim or something...  Really!


Attached is a patch to common/cmd_i2c.c that allows access to two I2C
controllers on a board.  Note that this doesn't actually change any
hardware control - it just enhances the command set and passes more
information to whatever version of i2c_read(), i2c_write() etc. that
you're using.  I've implemented driver changes on MPC8349 hardware, but
they're not quite ready for review yet.  New definitions:
CONFIG_I2C_2_CTRLS - board has two I2C controllers
CFG_I2C2_NOPROBES {} - list of devices on bus 2 to ignore when probing

CHANGELOG: 
        If CONFIG_I2C_2_CTRLS is defined, the 'chip' parameter of all
I2C
commands will accept an optional controller argument.    
e.g.  'imd 50.1 0' displays data at offset 0 of controller 1 device 50
      'imd 50.2 0' displays data at offset 0 of controller 2 device 50
      'iprobe 2' probes for devices on the second bus

regards,
Ben
-------------- next part --------------
A non-text attachment was scrubbed...
Name: CMD_I2C.patch
Type: text/x-patch
Size: 12674 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20060515/a03d58a4/attachment.bin 

[U-Boot-Users] PATCH: Add command support for second I2C controller

[Wolfgang Denk]

In message <1147724903.16780.145.camel@saruman.qstreams.net> you wrote:
> 
> CHANGELOG: 
>         If CONFIG_I2C_2_CTRLS is defined, the 'chip' parameter of all
> I2C
> commands will accept an optional controller argument.    
> e.g.  'imd 50.1 0' displays data at offset 0 of controller 1 device 50
>       'imd 50.2 0' displays data at offset 0 of controller 2 device 50
>       'iprobe 2' probes for devices on the second bus

I reject this patch for reasons discussed before in this thread. This
is not the interface I want to see.

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
The Wright Bothers weren't the first to fly. They were just the first
not to crash.

[U-Boot-Users] PATCH: Add command support for second I2C controller

[Menon, Nishanth]

Hi Ben,
Some reasons why I am against the approach of giving i2c bus along with
the command itself:
1. Cannot handle scenarios of HS controllers.
2. cmd_i2c is a debug interface, once a bus is selected, we would like
to do read and write operations to a specific chip. To repeat the bus
number all the time is a overhead. 
3. To a smaller extent, older users need to be aware of the change
involved.
4. a simpler and lesser intrusive implementation adding an additional
command to select bus can achieve the same functionality. (I send a
previous mail with this).
> Attached is a patch to common/cmd_i2c.c that allows access to two I2C
> controllers on a board.  Note that this doesn't actually change any
5. importantly, common/cmd_i2c.c is a bad place to limit i2c busses..
each board/.. file should be given the functionality.
Regards,
Nishanth Menon

[U-Boot-Users] PATCH: Add command support for second I2C controller

[Ben Warren]

Nishanth,

This approach sounds good.  The only down-side that I can see is that
the driver needs to keep track of context.  Personally, I prefer
self-contained code whenever possible, but in this case your approach is
better.  

In order to do this properly, we should add to the base API in
include/i2c.h something like:

void i2c_set_bus(uchar dev);
uchar i2c_get_bus(void);
void i2c_set_bus_speed(int speed);
int i2c_get_bus_speed(void);

or merge the index and speed as you have in your code.

The drivers can then either choose to implement or not implement this
feature, and we can add a parallel command tree based on 'i2c' as
Wolfgang has suggested.

Thoughts?

Ben

On Tue, 2006-05-16 at 10:10 -0500, Menon, Nishanth wrote:
> Hi Ben,
> Some reasons why I am against the approach of giving i2c bus along with
> the command itself:
> 1. Cannot handle scenarios of HS controllers.
> 2. cmd_i2c is a debug interface, once a bus is selected, we would like
> to do read and write operations to a specific chip. To repeat the bus
> number all the time is a overhead. 
> 3. To a smaller extent, older users need to be aware of the change
> involved.
> 4. a simpler and lesser intrusive implementation adding an additional
> command to select bus can achieve the same functionality. (I send a
> previous mail with this).
> > Attached is a patch to common/cmd_i2c.c that allows access to two I2C
> > controllers on a board.  Note that this doesn't actually change any
> 5. importantly, common/cmd_i2c.c is a bad place to limit i2c busses..
> each board/.. file should be given the functionality.
> Regards,
> Nishanth Menon

[U-Boot-Users] PATCH: Add command support for second I2C controller

[Menon, Nishanth]

Looks like my previous mail got blocked..
Here is the implementation I was speaking about:
common/cmd_i2c.c:
#if defined(CFG_I2C_BUS_SELECT)
int do_i2c_bus(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
{
       int bus_idx, bus_spd, res = 0;
       if (argc < 3) {
               printf("Usage[%d]:\n%s\n", argc, cmdtp->usage);
               return 1;
       }
       bus_idx = simple_strtoul(argv[1], NULL, 16);
       bus_spd = simple_strtoul(argv[2], NULL, 16);
       printf("Setting bus[%d] to Speed[%d]: ", bus_idx, bus_spd);
       res = select_bus(bus_idx, bus_spd);
       if (res) {
               printf("FAILED\n");
       } else {
               printf("PASS\n");
       }
       return res;
}
#endif                         /* bus select */
#if defined(CFG_I2C_BUS_SELECT)
U_BOOT_CMD(ibus, 3, 1, do_i2c_bus,
          "ibus    - Select i2c Bus\n",
          "bus_index speed\n    - Selects the bus index and sets the
speed (0x64(ST),0x190(FS),0xD48(HS))\n"
          "      (reports success/failure)\n");
#endif
Each board implementation can choose to implement select_bus() and
implement logic to handle speed mods, once a user gives a ibus, they can
use old commands to control devices on the bus..
Regards,
Nishanth Menon

> -----Original Message-----
> From: u-boot-users-admin at lists.sourceforge.net [mailto:u-boot-users-
> admin at lists.sourceforge.net] On Behalf Of Menon, Nishanth
> Sent: Tuesday, May 16, 2006 10:11 AM
> To: bwarren at qstreams.com; u-boot-users at lists.sourceforge.net
> Subject: RE: [U-Boot-Users] PATCH: Add command support for second I2C
> controller
> 
> Hi Ben,
> Some reasons why I am against the approach of giving i2c bus along
with
> the command itself:
> 1. Cannot handle scenarios of HS controllers.
> 2. cmd_i2c is a debug interface, once a bus is selected, we would like
> to do read and write operations to a specific chip. To repeat the bus
> number all the time is a overhead.
> 3. To a smaller extent, older users need to be aware of the change
> involved.
> 4. a simpler and lesser intrusive implementation adding an additional
> command to select bus can achieve the same functionality. (I send a
> previous mail with this).
> > Attached is a patch to common/cmd_i2c.c that allows access to two
I2C
> > controllers on a board.  Note that this doesn't actually change any
> 5. importantly, common/cmd_i2c.c is a bad place to limit i2c busses..
> each board/.. file should be given the functionality.
> Regards,
> Nishanth Menon
> 
> 
> -------------------------------------------------------
> Using Tomcat but need to do more? Need to support web services,
security?
> Get stuff done quickly with pre-integrated technology to make your job
> easier
> Download IBM WebSphere Application Server v.1.0.1 based on Apache
Geronimo
> http://sel.as-us.falkag.net/sel?cmd=k&kid\x120709&bid&3057&dat\x121642
> _______________________________________________
> U-Boot-Users mailing list
> U-Boot-Users at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/u-boot-users

[U-Boot-Users] PATCH: Add command support for second I2C controller

[Ben Warren]

I received your direct e-mail immediately - it's the mailing list ones
that seem to be taking a vacation for a few days...

On Tue, 2006-05-16 at 10:14 -0500, Menon, Nishanth wrote:

> Looks like my previous mail got blocked..
> Here is the implementation I was speaking about:
> common/cmd_i2c.c:
> #if defined(CFG_I2C_BUS_SELECT)
> int do_i2c_bus(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
> {
>        int bus_idx, bus_spd, res = 0;
>        if (argc < 3) {
>                printf("Usage[%d]:\n%s\n", argc, cmdtp->usage);
>                return 1;
>        }
>        bus_idx = simple_strtoul(argv[1], NULL, 16);
>        bus_spd = simple_strtoul(argv[2], NULL, 16);
>        printf("Setting bus[%d] to Speed[%d]: ", bus_idx, bus_spd);
>        res = select_bus(bus_idx, bus_spd);
>        if (res) {
>                printf("FAILED\n");
>        } else {
>                printf("PASS\n");
>        }
>        return res;
> }
> #endif                         /* bus select */
> #if defined(CFG_I2C_BUS_SELECT)
> U_BOOT_CMD(ibus, 3, 1, do_i2c_bus,
>           "ibus    - Select i2c Bus\n",
>           "bus_index speed\n    - Selects the bus index and sets the
> speed (0x64(ST),0x190(FS),0xD48(HS))\n"
>           "      (reports success/failure)\n");
> #endif
> Each board implementation can choose to implement select_bus() and
> implement logic to handle speed mods, once a user gives a ibus, they can
> use old commands to control devices on the bus..
> Regards,
> Nishanth Menon
> 
> > -----Original Message-----
> > From: u-boot-users-admin at lists.sourceforge.net [mailto:u-boot-users-
> > admin at lists.sourceforge.net] On Behalf Of Menon, Nishanth
> > Sent: Tuesday, May 16, 2006 10:11 AM
> > To: bwarren at qstreams.com; u-boot-users at lists.sourceforge.net
> > Subject: RE: [U-Boot-Users] PATCH: Add command support for second I2C
> > controller
> > 
> > Hi Ben,
> > Some reasons why I am against the approach of giving i2c bus along
> with
> > the command itself:
> > 1. Cannot handle scenarios of HS controllers.
> > 2. cmd_i2c is a debug interface, once a bus is selected, we would like
> > to do read and write operations to a specific chip. To repeat the bus
> > number all the time is a overhead.
> > 3. To a smaller extent, older users need to be aware of the change
> > involved.
> > 4. a simpler and lesser intrusive implementation adding an additional
> > command to select bus can achieve the same functionality. (I send a
> > previous mail with this).
> > > Attached is a patch to common/cmd_i2c.c that allows access to two
> I2C
> > > controllers on a board.  Note that this doesn't actually change any
> > 5. importantly, common/cmd_i2c.c is a bad place to limit i2c busses..
> > each board/.. file should be given the functionality.
> > Regards,
> > Nishanth Menon
> > 
> > 
> > -------------------------------------------------------
> > Using Tomcat but need to do more? Need to support web services,
> security?
> > Get stuff done quickly with pre-integrated technology to make your job
> > easier
> > Download IBM WebSphere Application Server v.1.0.1 based on Apache
> Geronimo
> > http://sel.as-us.falkag.net/sel?cmd=k&kid\x120709&bid&3057&dat\x121642
> > _______________________________________________
> > U-Boot-Users mailing list
> > U-Boot-Users at lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/u-boot-users
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.denx.de/pipermail/u-boot/attachments/20060516/7f65c4c9/attachment.htm