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

[Heiko Schocher]

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 ...

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 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.

Hmm... Ok, if we want to have a i2c_client struct instead an int ...
What do others think?

But, if we(you ;-) touch this, please start with using the DM for I2C!
This is also a step which should be done, and I do not want to have another
API, if somedays we find time to switch to DM!

>>> 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 ...

> 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?

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 ...

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 ...

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 :-(

> 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!

>>>> 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.

Sound like use/introduce DM for I2C!

> 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" ...

>>> 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

Full Ack.

> 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.

Ok, it was worth a try ;-)

So what do you think about defining functions like in [1] ?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany