From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Thu, 03 Oct 2013 07:52:31 +0200 Subject: [U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus In-Reply-To: <20131002171117.08bf21d1@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> Message-ID: <524D061F.7080204@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 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 >> @@ -16,9 +16,45 @@ >> #include >> #include >> >> +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