Re: [U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus
Hi Heiko,
Hello Lukasz,
Am 03.10.2013 18:15, schrieb Lukasz Majewski:
Hi Heiko,
Sorry for a late reply.
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 Durbinadur...@chromium.org
Signed-off-by: Simon Glasss...@chromium.org
Signed-off-by: Leela Krishna Amudalal.kris...@samsung.com
Reviewed-by: Doug Andersondiand...@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
[...]
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 ...
Yes, you are right. I've overlooked it.
With -Os compiler flag this compiles to a few ASM instructions.
Obviously it is NOT a performance killer :-) (I made unnecessary
fuzzz... sorry).
No problem!
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;
This looks nice.
Ok! I post soon a patch for it ...
- 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?
I think that it is acceptable.
Good.
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 ...
Yes, I see.
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.
My 2 cents. I understand that pmic_select() preserves old i2c bus
number, when PMIC performs transmission. This is probably done to
not break the legacy code (where one driver assumed, that it is
alone).
If this is necessary, then I'm OK with this. However I personally
think, that drivers shall call API functions from i2c core (like
i2c_bus_num()) only with bus number to switch and do not store and
preserve the i2c value. This is my personal comment.
Full Ack. I am just thinking, that we can get rid of such constructs,
independent of the new i2c framework switch. We just need to introduce
a current_i2c_cmd_bus in common/cmd_i2c.c. This var stores the
current i2c bus where i2c commands are executed ...
I think that last used bus variable shall be stored/managed at
i2c_core.c. I can use i2c without cmd_i2c.c compiled (as it is with
pmic and fuel gauge, which use different buses).
and all other
subsystems, which use the i2c_api can call i2c_set_bus_num() without
a previous save old bus and after the i2c bus usage a restore i2c
bus ...
I try to look into this, maybe we can do this before all
i2c drivers are ported to the new framework ...
Ok, lets wait for a patch.
bye,
Heiko
--
Best regards,
Lukasz Majewski
Samsung RD Institute Poland (SRPOL) | Linux Platform Group
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus
Hello Lukasz, Am 04.10.2013 10:58, schrieb Lukasz Majewski: Hi Heiko, Hello Lukasz, Am 03.10.2013 18:15, schrieb Lukasz Majewski: Hi Heiko, Sorry for a late reply. Hello Lukasz, Am 02.10.2013 17:11, schrieb Lukasz Majewski: Hi Leela, [...] 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. My 2 cents. I understand that pmic_select() preserves old i2c bus number, when PMIC performs transmission. This is probably done to not break the legacy code (where one driver assumed, that it is alone). If this is necessary, then I'm OK with this. However I personally think, that drivers shall call API functions from i2c core (like i2c_bus_num()) only with bus number to switch and do not store and preserve the i2c value. This is my personal comment. Full Ack. I am just thinking, that we can get rid of such constructs, independent of the new i2c framework switch. We just need to introduce a current_i2c_cmd_bus in common/cmd_i2c.c. This var stores the current i2c bus where i2c commands are executed ... I think that last used bus variable shall be stored/managed at i2c_core.c. I can use i2c without cmd_i2c.c compiled (as it is with Thats the case for the new framework! It stores the current bus, and so i2c_set_bus_num() can decide, if it is necessary to switch to a new bus ... Additionally to get rid of such store oldbus, switch to new, restore old bus constructs, the i2c commands needs to store somewhere, which i2c bus the i2c commands currently use, because you can do a i2c md ... then a date (maybe on antoher i2c bus) and then again a i2c md ... I think, thats the real historical reason for doing all over the code such save oldbus code ... That var (saying i2c_cur_cmd_bus) can easily stored in common/cmd_i2c.c, as commands are only useable after relocation. So, we must just add a i2c_set_bus_num(i2c_cur_cmd_bus) call before a command is executed ... That can be done in common/cmd_i2c.c also ... So, in the end, before every i2c access we call i2c_set_bus_num() and have no longer to store an old bus ... and if we have reached this point, we can make i2c_set_bus_num() static, and add to the i2c api a new first int bus parameter and can delete all i2c_set_bus_num() calls ... so every i2c device must know, on which i2c bus it works ... and as a bonus, we can get rid of all i2c_init() calls all over around the code, as i2c_set_bus_num() checks, if the drivers is initialized or not, if not initialize it, look if i2c muxes are involved on this bus, and so on ... pmic and fuel gauge, which use different buses). Yep, and its enough to call i2c_set_bus_num() before you want to access a device ... i2c_set_bus_num() do all necessary steps for you. and all other subsystems, which use the i2c_api can call i2c_set_bus_num() without a previous save old bus and after the i2c bus usage a restore i2c bus ... I try to look into this, maybe we can do this before all i2c drivers are ported to the new framework ... Ok, lets wait for a patch. I have such a patch in my queue, but it works only for drivers, which use the new framework I think ... so I must think about it, if this works for old style drivers too ... 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
Re: [U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus
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.
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 RD Institute Poland (SRPOL) | Linux Platform Group
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus
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 RD 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,
Re: [U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus
Hi Heiko,
Sorry for a late reply.
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 Durbinadur...@chromium.org
Signed-off-by: Simon Glasss...@chromium.org
Signed-off-by: Leela Krishna Amudalal.kris...@samsung.com
Reviewed-by: Doug Andersondiand...@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 @@
#includei2c.h
#includecompiler.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 ...
Yes, you are right. I've overlooked it.
With -Os compiler flag this compiles to a few ASM instructions.
Obviously it is NOT a performance killer :-) (I made unnecessary
fuzzz... sorry).
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;
This looks nice.
- 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?
I think that it is acceptable.
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 ...
Yes, I see.
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.
My 2 cents. I understand that pmic_select() preserves old i2c bus
number, when PMIC performs transmission. This is probably done to not
break the legacy code (where one driver assumed, that it is alone).
If this is necessary, then I'm OK with this. However I personally think,
that drivers shall call API functions from i2c core (like i2c_bus_num())
only with bus number
Re: [U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus
Hello Lukasz,
Am 03.10.2013 18:15, schrieb Lukasz Majewski:
Hi Heiko,
Sorry for a late reply.
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 Durbinadur...@chromium.org
Signed-off-by: Simon Glasss...@chromium.org
Signed-off-by: Leela Krishna Amudalal.kris...@samsung.com
Reviewed-by: Doug Andersondiand...@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
[...]
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 ...
Yes, you are right. I've overlooked it.
With -Os compiler flag this compiles to a few ASM instructions.
Obviously it is NOT a performance killer :-) (I made unnecessary
fuzzz... sorry).
No problem!
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;
This looks nice.
Ok! I post soon a patch for it ...
- 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?
I think that it is acceptable.
Good.
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 ...
Yes, I see.
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.
My 2 cents. I understand that pmic_select() preserves old i2c bus
number, when PMIC performs transmission. This is probably done to not
break the legacy code (where one driver assumed, that it is alone).
If this is necessary, then I'm OK with this. However I personally think,
that drivers shall call API functions from i2c core (like i2c_bus_num())
only with bus number to switch and do not store and preserve the i2c
value. This is my personal comment.
Full Ack. I am just thinking, that we can get rid of such constructs,
independent of the new i2c framework switch. We just need to introduce
a current_i2c_cmd_bus in common/cmd_i2c.c. This var stores the
current i2c bus where i2c commands are executed ... and all other
subsystems, which use the i2c_api can call i2c_set_bus_num() without
a previous save old bus and after the i2c bus usage a restore i2c
bus ... I try to look into this, maybe we can do this before all
i2c drivers are ported to the new framework ...
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
Re: [U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus
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) ?
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).
+ 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 RD Institute Poland (SRPOL) | Linux Platform Group
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus
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 Durbinadur...@chromium.org
Signed-off-by: Simon Glasss...@chromium.org
Signed-off-by: Leela Krishna Amudalal.kris...@samsung.com
Reviewed-by: Doug Andersondiand...@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 @@
#includei2c.h
#includecompiler.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
[U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus
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);
+ 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;
}
--
1.7.10.4
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
