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

[Simon Glass]

Hi Thierry,

On 27 August 2014 05:41, Thierry Reding <thierry.reding at gmail.com> wrote:
> 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.
>

I can help if you are interested. I have patches for SPI at
u-boot-dm.git (branch working) and would like to have I2C. They are
sort-of similar so it might not be to hard to convert Tegra I2C over.

The concept of a 'current' bus is broken IMO. Maybe the command line
should have this concept, and maintain a 'current hose' or whatever.
But drives and other code should specific what they need with each
transaction. With DM that will likely be automatic.

Regards,
Simon