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

From: Heiko Schocher <hs@denx.de>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH v2 13/40] i2c: Add high-level API
Date: Wed, 27 Aug 2014 09:07:58 +0200	[thread overview]
Message-ID: <53FD83CE.3050105@denx.de> (raw)
In-Reply-To: <20140827062124.GE15640@ulmo>

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

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

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

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

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

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