[U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus

[Heiko Schocher]

Hello Lukasz,

Am 03.10.2013 18:15, schrieb Lukasz Majewski:
> Hi Heiko,
>
> Sorry for a late reply.
>
>> Hello Lukasz,
>>
>> Am 02.10.2013 17:11, schrieb Lukasz Majewski:
>>> Hi Leela,
>>>
>>>> The current pmic i2c code assumes the current i2c bus is
>>>> the same as the pmic device's bus. There is nothing ensuring
>>>> that to be true. Therefore, select the proper bus before performing
>>>> a transaction.
>>>>
>>>> Signed-off-by: Aaron Durbin<adurbin at chromium.org>
>>>> Signed-off-by: Simon Glass<sjg at chromium.org>
>>>> Signed-off-by: Leela Krishna Amudala<l.krishna at samsung.com>
>>>> Reviewed-by: Doug Anderson<dianders at google.com>
>>>> ---
>>>>    drivers/power/power_i2c.c |   62
>>>> +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57
>>>> insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
>>>> index 47c606f..c22e01f 100644
>>>> --- a/drivers/power/power_i2c.c
>>>> +++ b/drivers/power/power_i2c.c
[...]
>> Yes, maybe we could optimze this in drivers/i2c/i2c_core.c. It should
>> be enough to detect the max adapter once ... but it is not a
>> "search"... ll_entry_count() calculates the number ...
>
> Yes, you are right. I've overlooked it.
>
> With -Os compiler flag this compiles to a few ASM instructions.
> Obviously it is NOT a performance killer :-) (I made unnecessary
> fuzzz... sorry).

No problem!

>> Looking in i2c_set_bus_num(), I think it can be optimized ...
>> lets speaking code:
>>
>> diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c
>> index d1072e8..170423a 100644
>> --- a/drivers/i2c/i2c_core.c
>> +++ b/drivers/i2c/i2c_core.c
>> @@ -278,20 +278,22 @@ unsigned int i2c_get_bus_num(void)
>>     */
>>    int i2c_set_bus_num(unsigned int bus)
>>    {
>> -       int max = ll_entry_count(struct i2c_adapter, i2c);
>> +       int max;
>> +
>> +       if ((bus == I2C_BUS)&&  (I2C_ADAP->init_done>  0))
>> +               return 0;
>
> This looks nice.

Ok! I post soon a patch for it ...

>> -       if (I2C_ADAPTER(bus)>= max) {
>> -               printf("Error, wrong i2c adapter %d max %d
>> possible\n",
>> -                      I2C_ADAPTER(bus), max);
>> -               return -2;
>> -       }
>>    #ifndef CONFIG_SYS_I2C_DIRECT_BUS
>>           if (bus>= CONFIG_SYS_NUM_I2C_BUSES)
>>                   return -1;
>>    #endif
>>
>> -       if ((bus == I2C_BUS)&&  (I2C_ADAP->init_done>  0))
>> -               return 0;
>> +       max = ll_entry_count(struct i2c_adapter, i2c);
>> +       if (I2C_ADAPTER(bus)>= max) {
>> +               printf("Error, wrong i2c adapter %d max %d
>> possible\n",
>> +                      I2C_ADAPTER(bus), max);
>> +               return -2;
>> +       }
>>
>>    #ifndef CONFIG_SYS_I2C_DIRECT_BUS
>>           i2c_mux_disconnet_all();
>>
>> So, first check, if we are on the correct bus, and return immediately!
>> What do you think?
>
> I think that it is acceptable.

Good.

>> Beside of that, pmic_select() does the check, if we are on the correct
>> bus too, and calls i2c_set_bus_num() only, if not ... so this is here
>> no problem ...
>
> Yes, I see.
>
>> but exactly I want to get rid of this code as it is in
>> pmic_select() someday, when all i2c drivers converted to the new i2c
>> framework.
>
> My 2 cents. I understand that pmic_select() preserves old i2c bus
> number, when PMIC performs transmission. This is probably done to not
> break the legacy code (where one driver assumed, that it is alone).
>
> If this is necessary, then I'm OK with this. However I personally think,
> that drivers shall call API functions from i2c core (like i2c_bus_num())
> only with bus number to switch and do not store and preserve the i2c
> value. This is my personal comment.

Full Ack. I am just thinking, that we can get rid of such constructs,
independent of the new i2c framework switch. We just need to introduce
a "current_i2c_cmd_bus" in common/cmd_i2c.c. This var stores the
current i2c bus where i2c commands are executed ... and all other
subsystems, which use the i2c_api can call i2c_set_bus_num() without
a previous "save old bus" and after the i2c bus usage a "restore i2c
bus" ... I try to look into this, maybe we can do this before all
i2c drivers are ported to the new framework ...

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