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

[Minkyu Kang]

On 03/01/14 08:37, Leela Krishna Amudala wrote:
> Hi Minkyu Kang,
> 
> On Thu, Dec 5, 2013 at 2:50 PM, Minkyu Kang <mk7.kang at samsung.com> wrote:
>> Dear Leela Krishna Amudala,
>>
>> On 12/11/13 19:04, Leela Krishna Amudala wrote:
>>> 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>
>>> Acked-by: Simon Glass <sjg at chromium.org>
>>> ---
>>>  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 ac76870..3cafa4d 100644
>>> --- a/drivers/power/power_i2c.c
>>> +++ b/drivers/power/power_i2c.c
>>> @@ -16,9 +16,45 @@
>>>  #include <i2c.h>
>>>  #include <compiler.h>
>>>
>>> +static int pmic_select(struct pmic *p)
>>> +{
>>> +     int ret, old_bus;
>>
>> I think, old prefix is meaningless.
>> please fix it globally.
>>
> 
> I think, it is necessary to differentiate with the current bus.
> Please see my below commets for clear picture.

there's no current bus variable.
also, we knew that p->bus is current bus.

> 
>>> +
>>> +     old_bus = i2c_get_bus_num();
>>> +     if (old_bus != p->bus) {
>>
>> How about return immediately if get a bus.
>>
>> if (old_bus == p->bus)
>>         return old_bus;
>>
> 
> The current code is also doing the same but in other way.
> If old_bus != p->bus then set the new bus otherwise no code to execute
> and return old_bus.
> This is same as what you suggested.

I know.
I'm talking about code quality.
please think, which one is better.

> 
>>> +             debug("%s: Select bus %d\n", __func__, p->bus);
>>> +             ret = i2c_set_bus_num(p->bus);
>>> +             if (ret) {
>>> +                     debug("%s: Cannot select pmic %s, err %d\n",
>>> +                           __func__, p->name, ret);
>>> +                     return -1;
>>> +             }
>>> +     }
>>> +
>>> +     return old_bus;
>>> +}
>>> +
>>> +static int pmic_deselect(int old_bus)
>>
>> in your patch, you never check a return value.
>> then is it void function or wrong usage?
>>
> 
> Okay, I'll change the function return type.
> 
>> And I think pmic_deselect function is almost same with pmic_select.
>> If you change the parameter for pmic_select to "int bus" then,
>> What is different with pmic_select?
>>
>> for example.
>>
>> bus = pmic_select(p->bus);
>> /* do something */
>> pmic_deselect(bus);
>>
>> is same with.
>>
>> bus = pmic_select(p->bus);
>> /* do something */
>> pmic_select(bus);
>>
>> How do you think?
>>
> 
> Yes, pmic_deselect is almost same as pmic_select but there is a minor
> difference.
> 
> pmic_select() is used to set new bus and this function returns the old
> bus number. we will hold this old_bus number and once we are done with
> our work we have to restore the old_bus so we are passing old_bus to
> pmic_deselect()
> 
> Now, pmic_deselect() takes old_bus as argument and trying to set it.
> This function won't return any bus number.
> 
> Hope this explanation helps.

I know.
the focus is that almost codes were duplicated.
My suggestion is one of example for reducing code duplication.
Please think about it.

> 
>>> +{
>>> +     int ret;
>>> +
>>> +     if (old_bus != i2c_get_bus_num()) {
>>> +             ret = i2c_set_bus_num(old_bus);
>>> +             debug("%s: Select bus %d\n", __func__, old_bus);
>>> +             if (ret) {
>>> +                     debug("%s: Cannot restore i2c bus, err %d\n",
>>> +                           __func__, ret);
>>> +                     return -1;
>>> +             }
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>>  int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
>>>  {
>>>       unsigned char buf[4] = { 0 };
>>> +     int ret, old_bus;
>>>
>>>       if (check_reg(p, reg))
>>>               return -1;
>>> @@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
>>>               return -1;
>>>       }
>>>
>>> -     if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
>>> +     old_bus = pmic_select(p);
>>> +     if (old_bus < 0)
>>>               return -1;
>>>
>>> -     return 0;
>>> +     ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
>>> +     pmic_deselect(old_bus);
>>
>> please add blank line.
>>
> 
> Okay, will do it.
> 
>>> +     return ret;
>>>  }
>>>
>>>  int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
>>>  {
>>>       unsigned char buf[4] = { 0 };
>>>       u32 ret_val = 0;
>>> +     int ret, old_bus;
>>>
>>>       if (check_reg(p, reg))
>>>               return -1;
>>>
>>> -     if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
>>> +     old_bus = pmic_select(p);
>>> +     if (old_bus < 0)
>>>               return -1;
>>>
>>> +     ret = i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
>>> +     pmic_deselect(old_bus);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>>       switch (pmic_i2c_tx_num) {
>>>       case 3:
>>>               if (p->sensor_byte_order == PMIC_SENSOR_BYTE_ORDER_BIG)
>>> @@ -98,9 +144,15 @@ int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
>>>
>>>  int pmic_probe(struct pmic *p)
>>>  {
>>> -     i2c_set_bus_num(p->bus);
>>> +     int ret, old_bus;
>>> +
>>> +     old_bus = pmic_select(p);
>>> +     if (old_bus < 0)
>>> +             return -1;
>>
>> please add blank line.
>>
> 
> Okay, will do it.
> 
> Best Wishes,
> Leela Krishna.
> 
>>>       debug("Bus: %d PMIC:%s probed!\n", p->bus, p->name);
>>> -     if (i2c_probe(pmic_i2c_addr)) {
>>> +     ret = i2c_probe(pmic_i2c_addr);
>>> +     pmic_deselect(old_bus);
>>> +     if (ret) {
>>>               printf("Can't find PMIC:%s\n", p->name);
>>>               return -1;
>>>       }
>>>
>>
>> Thanks,
>> Minkyu Kang.
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
> 

Thanks,
Minkyu Kang.