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

[Simon Glass]

Hi Heiko,

On Thu, Oct 25, 2012 at 10:48 PM, Heiko Schocher <hs at 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 at denx.de>  wrote:
>>>
>>> rebased/reworked the I2C multibus patches from Simon Glass found
>>> here:
>>>
>>> http://www.mail-archive.com/u-boot@lists.denx.de/msg75530.html
>>>
>>> It seems the timing is coming, to bring this in mainline and
>>> move boards over to the new i2c framework. As an example I
>>> converted the soft-i2c driver (and all boards using it) to
>>> the new framework, so this patchseries has to be tested
>>> intensively, as I can check compile only ...
>>
>>
>> I am very happy to see this, thank you.
>
>
> I am too ;-) and Sorry that I am only now ready ...
>
>
>> I have brought this in and tried to get it running for Tegra. A few
>> points:
>>
>> 1. The methods in struct i2c_adapter should IMO be passed a struct
>> i2c_adapter *, so they can determine which adapter is being accessed.
>> Otherwise I can't see how they would know.
>
>
> They can get the current used adapter through the defines in
> include/i2c.h:
> [...]
> #define I2C_ADAP_NR(bus)        i2c_adap[i2c_bus[bus].adapter]
> #define I2C_BUS                 ((struct i2c_bus_hose *)gd->cur_i2c_bus)
> #define I2C_ADAP                i2c_adap[I2C_BUS->adapter]
> #define I2C_ADAP_HWNR           (I2C_ADAP->hwadapnr)
>
> preparing just the fsl i2c driver and there I do for example:
> drivers/i2c/fsl_i2c.c
> [...]
> static const struct fsl_i2c *i2c_dev[2] = {
>         (struct fsl_i2c *) (CONFIG_SYS_IMMR + CONFIG_SYS_FSL_I2C_OFFSET),
> #ifdef CONFIG_SYS_FSL_I2C2_OFFSET
>         (struct fsl_i2c *) (CONFIG_SYS_IMMR + CONFIG_SYS_FSL_I2C2_OFFSET)
> #endif
> };
> [...]
> static int fsl_i2c_probe(uchar chip)
> {
>         struct fsl_i2c *dev = (struct fsl_i2c *)i2c_dev[I2C_ADAP_HWNR];
> [...]
>
> but of course, we still can change the "struct i2c_adapter" if
> needed ... but we have one more parameter ... Ok, not really a bad
> problem.
>
>
>> 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.

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.

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

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