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

[Stephen Warren]

On 10/29/2012 03:47 AM, Heiko Schocher wrote:
> Hello Stephen,
> 
> On 26.10.2012 18:07, Stephen Warren wrote:
>> On 10/25/2012 11:48 PM, Heiko Schocher 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.
>>
>> That rather relies on their being a concept of a "current" I2C adapter.
>> It seems a little limiting to require that. What if the "current"
>> adapter is the user-selected adapter for commands to operate on, but
>> e.g. some power-management driver wants to use I2C to communicate with a
>> PMIC during the internals of some other command. Sure, you could save
>> and later restore the I2C core's idea of "current" adapter, but it'd
>> surely be cleaner to just pass around the I2C adapter ID or struct
>> pointer everywhere to avoid the need for save/restore.
> 
> Yes, you are right, but just the same problem with current code!
> You mixed here two things!

I think you're reading more into what I was saying than what I actually
said.

If there are e.g. 4 I2C controllers in an SoC, the driver needs to know
which one is in use. Passing that information directly to the driver
functions is much simple than requiring the SoC I2C driver to go grovel
in some I2C core global variables to find out the same information.

This is all unrelated to I2C bus muxes; they shouldn't be implemented as
part of an SoC I2C driver anyway, so the driver shouldn't know about bus
muxes before or after this patch - the I2C core should manage that.