Hi Leela,
> Hello Lukasz,
>
> Thanks for reviewing the patch.
>
> On Wed, Oct 2, 2013 at 8:41 PM, Lukasz Majewski
> <l.majew...@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 <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>
> >> ---
> >> 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@lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
--
Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
