From patchwork Thu Oct 3 05:52:31 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Heiko Schocher X-Patchwork-Id: 280222 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from theia.denx.de (theia.denx.de [85.214.87.163]) by ozlabs.org (Postfix) with ESMTP id 3B7AB2C00AC for ; Thu, 3 Oct 2013 15:53:07 +1000 (EST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 5C38B4A081; Thu, 3 Oct 2013 07:53:01 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at theia.denx.de Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ahZGqxZIshwC; Thu, 3 Oct 2013 07:53:01 +0200 (CEST) Received: from theia.denx.de (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 52E1E4A08A; Thu, 3 Oct 2013 07:53:00 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id DB3B94A08A for ; Thu, 3 Oct 2013 07:52:51 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at theia.denx.de Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 7fiFZhQ7CC57 for ; Thu, 3 Oct 2013 07:52:45 +0200 (CEST) X-policyd-weight: NOT_IN_SBL_XBL_SPAMHAUS=-1.5 NOT_IN_SPAMCOP=-1.5 BL_NJABL=ERR(-1.5) (only DNSBL check requested) Received: from relay-m1-k.invitel.net (mailout-m0-k.invitel.net [91.82.116.154]) by theia.denx.de (Postfix) with ESMTP id 21C264A081 for ; Thu, 3 Oct 2013 07:52:37 +0200 (CEST) Received: from mail.invitel.hu (kozma-invitel-mail1.isp [192.168.18.246]) by relay-m1-k.invitel.net (Invitel MailHub Service) with ESMTP id 34D11222FE; Thu, 3 Oct 2013 07:52:36 +0200 (CEST) Received: from mail.invitel.hu (localhost [127.0.0.1]) by localhost (Invitel Mail Submit Server proxy-1) with SMTP id 1DB449FF6B; Thu, 3 Oct 2013 07:52:36 +0200 (CEST) X-Invitel-SMTP-Policy: accepting mail on this level X-Invitel-SMTP-Policy: accepting mail on this level X-Invitel-SMTP-Policy: accepting mail on this level X-Invitel-SMTP-Policy: accepting mail on this level X-Invitel-SMTP-Policy: accepting mail on this level X-Invitel-SMTP-Policy: accepting mail on this level X-Invitel-SMTP-Policy: accepting mail on this level X-Invitel-SMTP-Policy: accepting mail on this level X-Invitel-SMTP-Policy: accepting mail on this level Received: from [192.168.0.110] (87-97-63-112.pool.invitel.hu [87.97.63.112]) (Authenticated sender: heiko.schocher@invitel.hu) by mail.invitel.hu (Invitel Mail Submit Server proxy-1) with ESMTPA id F184E9FE96; Thu, 3 Oct 2013 07:52:34 +0200 (CEST) Message-ID: <524D061F.7080204@denx.de> Date: Thu, 03 Oct 2013 07:52:31 +0200 From: Heiko Schocher Organization: DENX Software Engineering User-Agent: Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20120421 Thunderbird/12.0 MIME-Version: 1.0 To: Lukasz Majewski 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> In-Reply-To: <20131002171117.08bf21d1@amdc308.digital.local> X-PMX-Spam: Gauge=XXIIIIIIII, Probability=28%, Report=' SXL_IP_DYNAMIC 3, MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODY_SIZE_4000_4999 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, ECARD_KNOWN_DOMAINS 0, RDNS_GENERIC_POOLED 0, RDNS_SUSP 0, RDNS_SUSP_GENERIC 0, __ANY_URI 0, __BOUNCE_CHALLENGE_SUBJ 0, __BOUNCE_NDR_SUBJ_EXEMPT 0, __CT 0, __CTE 0, __CT_TEXT_PLAIN 0, __FORWARDED_MSG 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_REPLYTO 0, __IN_REP_TO 0, __MIME_TEXT_ONLY 0, __MIME_VERSION 0, __MOZILLA_MSGID 0, __MOZILLA_USER_AGENT 0, __MULTIPLE_RCPTS_CC_X2 0, __REPLYTO_SAMEAS_FROM_ACC 0, __REPLYTO_SAMEAS_FROM_ADDY 0, __REPLYTO_SAMEAS_FROM_DOMAIN 0, __SANE_MSGID 0, __SUBJ_ALPHA_END 0, __SUBJ_ALPHA_NEGATE 0, __TO_MALFORMED_2 0, __URI_NO_PATH 0, __URI_NO_WWW 0, __URI_NS , __USER_AGENT 0' Cc: u-boot-review@google.com, Leela Krishna Amudala , adurbin@chromium.org, dianders@google.com, u-boot@lists.denx.de, alim.akhtar@samsung.com, vbendeb@chromium.org Subject: Re: [U-Boot] [PATCH 2/6] power: Explicitly select pmic device's bus X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.11 Precedence: list Reply-To: hs@denx.de List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: u-boot-bounces@lists.denx.de Errors-To: u-boot-bounces@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: 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 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();