From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Fri, 04 Oct 2013 07:23:23 +0200 Subject: [U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus In-Reply-To: <20131003181559.4671f68f@amdc308.digital.local> References: <1380637926-26242-1-git-send-email-l.krishna@samsung.com> <1380637926-26242-3-git-send-email-l.krishna@samsung.com> <20131002171117.08bf21d1@amdc308.digital.local> <524D061F.7080204@denx.de> <20131003181559.4671f68f@amdc308.digital.local> Message-ID: <524E50CB.6080208@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>> Signed-off-by: Simon Glass >>>> Signed-off-by: Leela Krishna Amudala >>>> Reviewed-by: Doug Anderson >>>> --- >>>> 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