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 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
[...]
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
