| Message ID | 1384250684-16124-3-git-send-email-l.krishna@samsung.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Minkyu Kang |
| Headers | show |
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@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> > Acked-by: Simon Glass <sjg@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. > + > + 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; > + 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? 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? > +{ > + 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. > + 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. > 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.
Hi Minkyu Kang, On Thu, Dec 5, 2013 at 2:50 PM, Minkyu Kang <mk7.kang@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@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> >> Acked-by: Simon Glass <sjg@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@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
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@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@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> >>> Acked-by: Simon Glass <sjg@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.
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; + + 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); + 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) @@ -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; 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; }