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.k...@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 <adur...@chromium.org>
>>> Signed-off-by: Simon Glass <s...@chromium.org>
>>> Signed-off-by: Leela Krishna Amudala <l.kris...@samsung.com>
>>> Reviewed-by: Doug Anderson <diand...@google.com>
>>> Acked-by: Simon Glass <s...@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@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>
Thanks,
Minkyu Kang.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
