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 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 ...
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 R&D Institute Poland (SRPOL) | Linux Platform Group _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot