[U-Boot] [PATCH 0/3] Bring in new I2C framework

[Heiko Schocher <hs@denx.de>]

Hello Simon,

On 26.10.2012 18:08, Simon Glass wrote:
> On Thu, Oct 25, 2012 at 10:48 PM, Heiko Schocher<hs@denx.de>  wrote:
>> Hello Simon,
>>
>>
>> On 25.10.2012 23:37, Simon Glass wrote:
>>>
>>> On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocher<hs@denx.de>   wrote:
[...]
>>> 2. The init is a bit odd, in that we can call init() repeatedly.
>>> Perhaps that function should be renamed to reset, and the init should
>>> be used for calling just once at the start?
>>
>>
>> What do you mean here exactly? I couldn?t parse this ...
>
> Well there is start-of-day setup, which I think should be called init.
> This is done once on boot for each i2c adapter.

Hmm... I am not sure if this is only done on boot, because we should
"close" or "deinit" an adapter if not used any more in U-Boot as the
U-Boot principle says:

http://www.denx.de/wiki/view/U-Boot/DesignPrinciples#2_Keep_it_Fast

So I want to add in future some "deinit" to every adapter, and
call it from i2c_set_bus() when switching to another i2c adapter ...

> And then there is the i2c_init() which seems to be called whenever we
> feel like it - e.g. to change speed. I suggest that we use init() to
> mean start-of-day init and reset(), or similar, to mean any post-init.
> I am not suggest that for this series, just as a future effort.

Yes, that should be changed. We do not need an init() in the i2c
API, as i2c_set_bus_num() do this for us (and later also the deinit())

We just need a set/get_speed() and a deblock()/reset() ?

Maybe a step in the API cleanup?

>>> 3. Rather than each device having to call i2c_get_bus_num() to find
>>> out what bus is referred to, why not just pass this information in? In
>>> fact the adapter pointer can serve for this.
>>
>>
>> Not the "struct i2c_adapter" must passed, but the "struct
>> i2c_bus"!
>>
>> And each device should know, which i2c bus it uses, or? So at
>> the end we should have something like i2c_read(struct i2c_bus *bus, ...)
>> calls ... and the i2c core can detect, if this bus is the
>> current, if so go on, if not switch to this bus. So at the
>> end i2c_set_bus_num would be go static ...
>>
>>
>>> 4. So perhaps the i2c read/write functions should change from:
>>>
>>> int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
>>>
>>> to:
>>>
>>> int i2c_read((struct i2c_adapter *adapter, uint addr, int alen, uchar
>>> *buffer, int len)
>>
>>
>> Yep, exactly, see comments to point 3 ...
>>
>> That would be the best (and I think in previous discussions I defined
>> this as one goal ...), but this would be (another) big change,
>> because this is an *API* change, with maybe a lot of config file
>> changes ...
>>
>> Let us bring in the new i2c framework with all i2c drivers converted,
>> and then do the next step ... maybe one step more, if mareks device
>> model is ready, we can switch easy to DM ... and maybe get this API
>> change for free ...
>
> Yes. I certainly understand the need to fit in with what is already
> there, and avoid a massive API change, which can be performed as a
> follow-on patch anyway. With your info above I will adjust the tegra
> driver to work with this and test it.

Ok, great! So I post v2 patches after you tested ...

And Yes, we should do this API change, but I tend to do this step after
the new i2c framework is stable and all i2c drivers are converted to it ...

>>> Later, I wonder whether the concept of a 'current' i2c bus should be
>>> maintained by the command line interpreter, rather than the i2c
>>> system. Because to be honest, most of the drivers I see have to save
>>> the current bus number, set the current bus, do the operation, then
>>> set the bus back how they found it (to preserve whatever the user
>>> things is the current bus).
>>
>>
>> Yes, suboptimal ... but this is independent from the new i2c framework
>> patches!
>>
>> It is possible (with old/new i2c bus framework) to introduce a
>> "current commandline i2c bus", and then, before calling i2c_read/write
>> from the commandline, call a i2c_set_bus_num() ... then we can get rid
>> of this store/restore current i2c bus ... waiting for patches ;-)
>
> OK.
>
>>
>>
>>> Granted there is overhead with i2c muxes, but the i2c core can
>>> remember the state of these muxes and doesn't have to switch things
>>> until there has been a change since the last transaction.
>>
>>
>> This exactly do the i2c_set_bus_num() now!
>
> Great.
>
>>
>>
>>> This last suggestion can be dealt with later, but I thought I would bring
>>> it up.
>>
>>
>> I am happy about every comment! :-)
>
> Thanks,
> Simon

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