Hello Lukasz,
Am 02.10.2013 17:11, schrieb Lukasz Majewski:
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) ?
Yes, maybe we could optimze this in drivers/i2c/i2c_core.c. It should be
enough to detect the max adapter once ... but it is not a "search"...
ll_entry_count() calculates the number ...
Looking in i2c_set_bus_num(), I think it can be optimized ...
lets speaking code:
diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c
index d1072e8..170423a 100644
--- a/drivers/i2c/i2c_core.c
+++ b/drivers/i2c/i2c_core.c
@@ -278,20 +278,22 @@ unsigned int i2c_get_bus_num(void)
*/
int i2c_set_bus_num(unsigned int bus)
{
- int max = ll_entry_count(struct i2c_adapter, i2c);
+ int max;
+
+ if ((bus == I2C_BUS) && (I2C_ADAP->init_done > 0))
+ return 0;
- if (I2C_ADAPTER(bus) >= max) {
- printf("Error, wrong i2c adapter %d max %d possible\n",
- I2C_ADAPTER(bus), max);
- return -2;
- }
#ifndef CONFIG_SYS_I2C_DIRECT_BUS
if (bus >= CONFIG_SYS_NUM_I2C_BUSES)
return -1;
#endif
- if ((bus == I2C_BUS) && (I2C_ADAP->init_done > 0))
- return 0;
+ max = ll_entry_count(struct i2c_adapter, i2c);
+ if (I2C_ADAPTER(bus) >= max) {
+ printf("Error, wrong i2c adapter %d max %d possible\n",
+ I2C_ADAPTER(bus), max);
+ return -2;
+ }
#ifndef CONFIG_SYS_I2C_DIRECT_BUS
i2c_mux_disconnet_all();
So, first check, if we are on the correct bus, and return immediately!
What do you think?
Beside of that, pmic_select() does the check, if we are on the correct
bus too, and calls i2c_set_bus_num() only, if not ... so this is here
no problem ... but exactly I want to get rid of this code as it is in
pmic_select() someday, when all i2c drivers converted to the new i2c
framework. i2c_set_bus_num() should go static then in drivers/i2c/i2c_core.c
and i2c_read/write/... become a new "int bus" parameter ... but this
will be a big api change ... but will prevent exactly such code
all over the u-boot code ...
bye,
Heiko
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
