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

[Thierry Reding]

On Wed, Aug 27, 2014 at 11:56:41AM +0200, Heiko Schocher wrote:
> Hello Thierry,
> 
> Am 27.08.2014 10:51, schrieb Thierry Reding:
> >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 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
> >>
> >>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.
> 
> You again just talk from i2c_adapter ... why you ignore i2c muxes?
> A bus is not only an i2c adapter ...

I know. I keep saying i2c_adapter because in the rough sketch that I
have in mind for how this could work eventually, an mux is just another
special kind of i2c_adapter.

> Currently we have two "versions" of i2c_adapter:
> 
> In a system without muxes, you can say: i2c bus == i2c adapter
> but in a system with muxes we have:     i2c bus == i2c_bus_hose
> 
> i2c commands use also a "bus number" starting from 0 ... the bus number
> has historical reasons... we could change this ...
> 
> But if we introduce a new API, please with mux functionallity ...
> Hmm.. thinking about it ... if you want to introduce a new API, please
> start with using the DM for I2C!

I can look into it, but it sounds like a task way beyond what I have
time for right now.

> >>>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.
> 
> Yes ... which really is a pro for having i2c_set_bus_num() not static ...

Or if said functionality is encapsulated within adapters then they can
be done as necessary with the same effect.

> >Also, if we directly talk to an adapter instead, then the bulk of what
> 
> You ignore again muxes ...
> 
> >i2c_set_bus_num() does isn't even required. It would require that
> 
> What does i2c_set_bus_num() ?
> 
> - first check, if current bus = new bus, and if it is initialized
>   if so -> all is good, return!
> 
> Thats the main case ... is this so expensive? This checks should
> be always done (except we do a bulk of i2c accesses, yes). I think,
> this has also to be done somewhere with your approach ... or?

It's currently done as part of i2c_adapter_get() which as you already
point out only work when muxes aren't used. But the main issue about the
current approach is that we have these global indices and pass them
around in global state and need to be careful to always set the current
bus where appropriate.

If we simply pass adapters around (provided they handle muxes correctly)
we can simply address each slave as an (adapter, slave address) pair,
rather than repeating the same arguments over and over and calling very
low-level bus management functions.

But we already agree that this isn't optimal. The disagreement seems to
be mostly about the implementation details.

> What is done in the case, we switch to another bus:
> 
> - If we have no muxes, save new bus for further use
> - look if new bus is initialized, if not initialize it
> 
> All this is hidden from the user ... and actions, which must be done,
> whatever API you define ...
> 
> If I understand you correct, your big change is not using "int bus"
> instead introducing i2c_client ... and define some functions, which
> use this struct as parameter ...

Correct.

> Why not defining:
> 
> [1]:
> int i2c_bus_read(int bus, uint8_t chip,
>              unsigned int address, size_t alen, void *buffer,
>              size_t size)
> {
>     i2c_set_bus_num(bus);
>     return i2c_read(chip, address, alen, buffer, size);
> }
> 
> int i2c_bus_write(int bus, uint8_t chip,
>               unsigned int address, size_t alen, void *buffer,
>               size_t size)
> {
>     i2c_set_bus_num(bus);
>     return i2c_write(chip, address, alen, buffer, size);
> }
> 
> and you never have to call something like i2c_init(), as
> i2c_set_bus_num() does this all for you! You only have to use in your
> driver i2c_bus_read/write ... without taking any attention ...

Yes, that sounds like a useful change in itself. But it still requires
users to explicitly manage the bus and chip numbers. The goal of the
i2c_client API is to standardize on how to talk to clients. Essentially
the goal would be for a driver to get a hold of a struct i2c_client via
some standard interface (perhaps with DM this could be done completely
outside of the driver itself) and then talks to the device via this
standard object. Currently drivers either need to hardcode the bus
number and slave addresses, or they need to store them in some custom
structure. i2c_client gives uniformity to that.

There are really two parts of a puzzle here. One is the client-side API
that drivers for slave devices use and the other is the I2C controller
drivers that provide the struct i2c_adapter.

> looking deeper in your approach:
> 
> +int i2c_client_init(struct i2c_client *client, struct i2c_adapter *adapter,
> +            uint8_t address)
> +{
> +    client->adapter = adapter;
> +    client->address = address;
> +
> +    return 0;
> +}
> 
> Where is here called adapter->init() ? Nor you check, if the i2c
> adapter is intialized ... if more than one driver uses this function,
> each intializes the hw? Or currently none :-(

It's done as part of the i2c_adapter_get(). It takes an index (which is
roughly the bus number) initializes the corresponding adapter (if found)
and returns a pointer to it.

To handle muxes I think this could work by adding hooks that could be
called before doing transactions to make sure that the correct muxing is
set up.

> >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
> 
> Oh!
> 
> >slaves are connected to (either a HW controller directly or a mux
> >connected to a HW controller).
> 
> Thats sounds for me, that you should use DM ... I do not know, what
> your plans are!

Yes, perhaps DM is part of the solution. But I don't have any concrete
plans. I'd rather tackle this step by step rather than as one monolithic
patch series.

> >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.
> 
> Ok, with some functions like in [1], maybe you introduce i2c_client,
> and use this instead "int bus" ...

This would probably work fine to approach the problem from two sides. An
API like the i2c_client could be used to abstract away all the
implementation details on the controller/mux side and then more work
could be done to convert the adapter side of things to whatever works
best.

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