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

[Lukasz Majewski]

Hi Heiko,

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

I think that "last used bus" variable shall be stored/managed at
i2c_core.c. I can use i2c without cmd_i2c.c compiled (as it is with
pmic and fuel gauge, which use different buses).

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

Ok, lets wait for a patch.

> 
> bye,
> Heiko



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group