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