[U-Boot] [PATCH v2 13/40] i2c: Add high-level API

[Thierry Reding]

On Wed, Aug 27, 2014 at 07:21:51AM +0200, Heiko Schocher wrote:
> Hello Thierry,
> 
> Am 26.08.2014 17:34, schrieb Thierry Reding:
> >From: Thierry Reding<treding at nvidia.com>
> >
> >This API operates on I2C adapters or I2C clients (a new type of object
> 
> which is a bad idea ...
> 
> >that refers to a particular slave connected to an adapter). This is
> >useful to avoid having to call i2c_set_bus_num() whenever a device is
> >being accessed.
> 
> But thats the supose of i2c_set_bus_num()! ... if you use an i2c bus,
> you must check before every access, if you are on it, if not, you must
> switch back to it...

That's not what code does today. Everybody calls i2c_set_bus_num() once
and then does a bunch of transactions on that bus. Given that U-Boot
doesn't run multithreaded this works. If you're really concerned about
this being a problem, then it should be solved at a different level. It
could be part of i2c_client for example, so that i2c_client_read() and
i2c_client_write() would always perform this step. Burdening users with
this isn't going to work (in a multithreaded environment the switch to a
different mux could happen between the call to i2c_set_bus_num() and the
bus access).

In fact I think this would even have to be solved at the controller
level if you want to make sure that client transactions are atomic.

> This is collected in i2c_set_bus_num() ... before, every "user" did
> this on his own ... if you are on the bus you want to access, the
> overhead is not so big, see:
> 
> http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/i2c_core.c;h=18d6736601c161f45cb7d81b5eae53bdeaaf6b0b;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l278
> 
>  278 int i2c_set_bus_num(unsigned int bus)
>  279 {
>  280         int max;
>  281
>  282         if ((bus == I2C_BUS) && (I2C_ADAP->init_done > 0))
>  283                 return 0;
> 
> And you must be aware of i2c muxes! You directly use the read/write
> functions from the i2c adapter, but what is if you have i2c muxes?

That's complexity that users shouldn't have to worry about. They should
simply access an adapter and the adapter (or rather the core) should
take care of setting up any muxes correctly.

> Maybe there is on one i2c adapter a i2c mux with 4 ports. On one is
> an eeprom, on the other is a PMIC ... your code in patch
> "power: Add AMS AS3722 PMIC support" does access with your functions
> the PMIC ... what is, if between this accesses someone accesses the eeprom?
> If he switches the mux, you never switch back!
> 
> Your code did not check this!

Like I said, a lot of code in U-Boot doesn't check this. And quite
frankly as long as this isn't handled in the core I don't think people
will get it right.

> Why is i2c_set_bus_num() such a problem?

Because it's completely confusing. And it's exposing an implementation
detail to users instead of handling it transparently in the core.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140827/9e77418e/attachment.pgp>