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

[Simon Glass]

Hi Stephen,

On Mon, Oct 29, 2012 at 8:34 AM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> 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.

I think Heiko agreed with this, just that he wants to take things a
step at a time.

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

Regards,
Simon