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

From: Lukasz Majewski <l.majewski@samsung.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus
Date: Thu, 03 Oct 2013 11:44:53 +0200	[thread overview]
Message-ID: <20131003114453.7bd69ce4@amdc308.digital.local> (raw)
In-Reply-To: <CAL1wa8cAZbwST6k3xksLpKfWBDrsCm81bmfNO65sZt38fBAi4g@mail.gmail.com>

Hi Leela,

> Hello Lukasz,
> 
> Thanks for reviewing the patch.
> 
> On Wed, Oct 2, 2013 at 8:41 PM, Lukasz Majewski
> <l.majewski@samsung.com> wrote:
> > 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@chromium.org>
> >> Signed-off-by: Simon Glass <sjg@chromium.org>
> >> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> >> Reviewed-by: Doug Anderson <dianders@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
> >> @@ -16,9 +16,45 @@
> >>  #include <i2c.h>
> >>  #include <compiler.h>
> >>
> >> +static int pmic_select(struct pmic *p)
> >> +{
> >> +     int ret, old_bus;
> >> +
> >> +     old_bus = i2c_get_bus_num();
> >> +     if (old_bus != p->bus) {
> >> +             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)
> >> +{
> >> +     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);
> >
> > I'm wondering if setting the bus before each, single i2c write
> > (when we usually perform several writes to one device) will not be
> > an overkill (we search the ll_entry_count linker list each time to
> > find max i2c adapter names) ?
> >
> 
> Here we are not setting the bus before each i2c write, If you look at
> the pmic_select() function code it shows like
> static int pmic_select(struct pmic *p)
> {
>     /......./
>     old_bus = i2c_get_bus_num();
>     if (old_bus != p->bus) {
>     /...../
>     }
>     return old_bus;
> }
> 
> Here we are trying to get the bus number and if it is equal to the bus
> on which we are going to write we are returning immediately which is
> minimal.

Thanks for explanation.

> 
> > The i2c_set_bus_num() is now done at pmic_probe(), but this also
> > introduces overkill for "probing" each device when we want to read
> > from it.
> >
> > As a side note - I would appreciate if you would add Stefano Babic
> > and me on the Cc (as we are listed at e.g. power_core.c).
> >
> >
> 
> Okay, will do it.
> 
> Best Wishes,
> Leela Krishna
> 
> >> +     pmic_deselect(old_bus);
> >> +     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) @@ -117,9 +163,15 @@ int
> >> pmic_reg_update(struct pmic *p, int reg, uint regval)
> >>  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;
> >>       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;
> >>       }
> >
> >
> >
> > --
> > Best regards,
> >
> > Lukasz Majewski
> >
> > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot

-- 
Best regards,

Lukasz Majewski

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