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

[Heiko Schocher]

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

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

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

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

> This last suggestion can be dealt with later, but I thought I would bring it up.

I am happy about every comment! :-)

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