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

[Leela Krishna Amudala]

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.

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

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

>> +{
>> +     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