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

[Thierry Reding <thierry.reding@gmail.com>]

On Wed, Aug 27, 2014 at 09:07:58AM +0200, Heiko Schocher wrote:
> Hello Thierry,
> 
> Am 27.08.2014 08:21, schrieb 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@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
> 
> Yes, sadly. This has historical reasons ...
> 
> >doesn't run multithreaded this works. If you're really concerned about
> 
> Yes, U-Boot is singlethread only.
> 
> >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
> 
> Exactly, thats right, and this is a goal from the CONFIG_SYS_I2C API!
> 
> But why do you introduce i2c_client_read/write and do not add this step
> to i2c_read/write?
> 
> - convert all i2c drivers, which are not yet converted to CONFIG_SYS_I2C
>   (get also rid od CONFIG_HARD_I2C)
> - add busnumber to i2c_read/write API and make i2c_set_bus_num() static ...
>   and fix all i2c_read/write() calls in U-Boot code ...

I don't think adding a bus number as parameter is useful. Why not just
use the I2C adapter directly? That way we don't have to keep looking it
up in an array every time.

> I know, this is a big change and a lot of work ... thats the reason
> why we are not at this point ... nobody volunteered to go forward, and
> I did not found time to do it ...

I suppose that would be one possibility to do it. But I consider
i2c_client more of a convenience around the lower-level i2c_read() and
i2c_write(). The idea is that users set up an I2C client once and then
refer to the client, which will automatically use the correct adapter
and slave address rather than having that duplicated in every driver.

> >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.
> 
> As U-Boot is single threaded all i2c_read/writes are atomic.

In which case you don't have to call i2c_set_bus_num() for every access,
only whenever you don't know exactly where you're coming from. Functions
that perform a sequence of accesses only need to set it once.

Also, if we directly talk to an adapter instead, then the bulk of what
i2c_set_bus_num() does isn't even required. It would require that
adapters are made aware of a hierarchy if there are muxes, but I think
that's worthwhile to do in any case. Also if ever I2C muxing needs to
gain device tree support having the muxes set up dynamically will be
pretty much a prerequisite.

> >>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
> 
> Exactly!
> 
> >simply access an adapter and the adapter (or rather the core) should
> >take care of setting up any muxes correctly.
> 
> Yes!
> 
> I think you mix here i2c adapter with bus. An "U-Boot i2c adapter" is a
> hw adapter (or special case soft i2c adapter). An "i2c bus" is a hw adapter
> maybe with m muxes, and each bus has exactly one way through the
> i2c muxes, see for an example the README:
> 
> http://git.denx.de/?p=u-boot.git;a=blob;f=README;h=14d6b227d689825025f9dfc98fb305021882446d;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l2349
> 
> So the only thing a User must know when he wants to use an i2c bus is
> his number. The switching to this i2c adapter, initializing it and maybe
> set i2c muxes does the i2c subsystem ...

The above doesn't preclude an I2C adapter representing one of the ports
of a mux. That way you can still talk to an adapter rather than having
to refer to the bus by number. Adapter would become a little more
abstract than it is now, since it would be simply an output that I2C
slaves are connected to (either a HW controller directly or a mux
connected to a HW controller).

> >>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
> 
> With using i2c_set_bus_num() you have not to check this! You only have
> to call i2c_set_bus_num() before calling i2c_read/write ... and yes,
> that would be nice, if we just pass the bus number to i2c_read/write()
> and drop the i2c_set_bus_num() call all over the code ...
> 
> Patches welcome!

How about a slightly different proposal: introduce a new level of
abstraction (like i2c_client) and start using it in new I2C slave
drivers. At the same time existing drivers could be converted one at a
time without having the big flag date when i2c_read() and i2c_write()
are switched over all at once.

When that new level of abstraction is used, we can hide all the
details behind that and the implementation no longer influences any of
the drivers. Then we could transparently rework adapters and muxes to
our heart's content without needing to update users of the high-level
API.

> >frankly as long as this isn't handled in the core I don't think people
> >will get it right.
> 
> Yes, full ack, which is the goal from CONFIG_SYS_I2C. If all i2c
> driver are converted to it, we can make i2c_set_bus_num() static, and
> add to the i2c API the bus number as a function parameter!
> 
> >>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.
> 
> Yes! Full Ack ... but I do not accept a new API for that! Please
> fix the i2c_read/write() functions!

Doing this kind of conversion is a nightmare. We'd be changing an API
that has around 600 occurrences in U-Boot, all of which need to be
changed *at once* to avoid build breakage. At the same time we need to
make sure any patches in development use the same API, which means that
they can't even be build-tested until the the API has been changed.

Transitioning step by step is a lot less complicated.

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/ff1ac85e/attachment.pgp>