From patchwork Thu Dec 27 19:27:38 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Wolfgang Denk X-Patchwork-Id: 208354 X-Patchwork-Delegate: sbabic@denx.de 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 B58372C00AC for ; Fri, 28 Dec 2012 06:27:48 +1100 (EST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 1E61C4A098; Thu, 27 Dec 2012 20:27:46 +0100 (CET) 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 MjEDtl-637x8; Thu, 27 Dec 2012 20:27:45 +0100 (CET) Received: from theia.denx.de (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 77BA14A08F; Thu, 27 Dec 2012 20:27:44 +0100 (CET) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 236ED4A08F for ; Thu, 27 Dec 2012 20:27:43 +0100 (CET) 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 AWcAob7sWF0J for ; Thu, 27 Dec 2012 20:27:42 +0100 (CET) X-policyd-weight: NOT_IN_SBL_XBL_SPAMHAUS=-1.5 NOT_IN_SPAMCOP=-1.5 NOT_IN_BL_NJABL=-1.5 (only DNSBL check requested) Received: from mail-out.m-online.net (mail-out.m-online.net [212.18.0.9]) by theia.denx.de (Postfix) with ESMTPS id 3FE9A4A08B for ; Thu, 27 Dec 2012 20:27:40 +0100 (CET) Received: from frontend1.mail.m-online.net (unknown [192.168.8.180]) by mail-out.m-online.net (Postfix) with ESMTP id 3YXLfh0Yqdz4KK7N; Thu, 27 Dec 2012 20:27:40 +0100 (CET) Received: from localhost (dynscan1.mnet-online.de [192.168.6.68]) by mail.m-online.net (Postfix) with ESMTP id 3YXLfh0PQqzbbd0; Thu, 27 Dec 2012 20:27:40 +0100 (CET) X-Virus-Scanned: amavisd-new at mnet-online.de Received: from smtp-auth.mnet-online.de ([192.168.8.180]) by localhost (dynscan1.mail.m-online.net [192.168.6.68]) (amavisd-new, port 10024) with ESMTP id XKRgAspiBEhN; Thu, 27 Dec 2012 20:23:15 +0100 (CET) X-Auth-Info: dva0HO/AwnKkPHhEHxU5R5wmH0JJI/L5PXEkf5F6be8= Received: from diddl.denx.de (host-80-81-18-216.customer.m-online.net [80.81.18.216]) by smtp-auth.mnet-online.de (Postfix) with ESMTPA; Thu, 27 Dec 2012 20:27:38 +0100 (CET) Received: from gemini.denx.de (unknown [10.0.0.2]) by diddl.denx.de (Postfix) with ESMTP id 55B021A6E0E; Thu, 27 Dec 2012 20:27:38 +0100 (CET) Received: from gemini.denx.de (localhost [IPv6:::1]) by gemini.denx.de (Postfix) with ESMTP id 41ED2200622; Thu, 27 Dec 2012 20:27:38 +0100 (CET) To: Fabio Estevam From: Wolfgang Denk MIME-Version: 1.0 In-reply-to: References: <1356604017-9699-1-git-send-email-festevam@gmail.com> <20121227103505.40769200F7A@gemini.denx.de> <50DC2B8F.2030709@denx.de> Comments: In-reply-to Fabio Estevam message dated "Thu, 27 Dec 2012 09:14:36 -0200." Date: Thu, 27 Dec 2012 20:27:38 +0100 Message-Id: <20121227192738.41ED2200622@gemini.denx.de> Cc: Fabio Estevam , u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH] mx53loco: Remove unneeded 'retval' variable X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.11 Precedence: list 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 Dear Fabio Estevam, In message you wrote: > > > However, only one of the two branch can run, because i2c_probe() fails > > if the PMIC is not found. > > Yes, correct. We have either Dialog DA9052 or FSL MC34708 on the > mx53loco boards, so only one of the branches will run. This is what the code assumes, indeed. But if somebody attaches some I2C device using the "free" address, things go wrong. I consider this a bug - and it's easy to fix, I think. If we agree that there can always be only one PMIC we can leave the function after having found the first one anyway, i. e. something like this: But to be honest, I dislike the whole code. The error handling is broken as is. 361 ret = pmic_reg_write(p, DA9053_BUCKCORE_REG, val); 362 363 ret |= pmic_reg_read(p, DA9053_SUPPLY_REG, &val); 364 val |= DA9052_SUPPLY_VBCOREGO; 365 ret |= pmic_reg_write(p, DA9053_SUPPLY_REG, val); ... If the first pmic_reg_write() fails, we should abort this function (and probably with a descriptive error message, too). Instead we continue to perform more pmic_reg_read() and pmic_reg_write() operations... An error for any of these calls should immediately abort this function. Best regards, Wolfgang Denk diff --git a/board/freescale/mx53loco/mx53loco.c b/board/freescale/mx53loco/mx53loco.c index 2c8cb7a..47c7fd9 100644 --- a/board/freescale/mx53loco/mx53loco.c +++ b/board/freescale/mx53loco/mx53loco.c @@ -343,14 +343,14 @@ static void setup_iomux_i2c(void) static int power_init(void) { unsigned int val; - int ret = -1; + int ret; struct pmic *p; int retval; if (!i2c_probe(CONFIG_SYS_DIALOG_PMIC_I2C_ADDR)) { - retval = pmic_dialog_init(I2C_PMIC); - if (retval) - return retval; + ret = pmic_dialog_init(I2C_PMIC); + if (ret) + return ret; p = pmic_get("DIALOG_PMIC"); if (!p) @@ -367,12 +367,14 @@ static int power_init(void) /* Set Vcc peripheral to 1.30V */ ret |= pmic_reg_write(p, DA9053_BUCKPRO_REG, 0x62); ret |= pmic_reg_write(p, DA9053_SUPPLY_REG, 0x62); + + return ret; } if (!i2c_probe(CONFIG_SYS_FSL_PMIC_I2C_ADDR)) { - retval = pmic_init(I2C_PMIC); - if (retval) - return retval; + ret = pmic_init(I2C_PMIC); + if (ret) + return ret; p = pmic_get("FSL_PMIC"); if (!p) @@ -401,9 +403,11 @@ static int power_init(void) /* Set SWBST to 5V in auto mode */ val = SWBST_AUTO; ret |= pmic_reg_write(p, SWBST_CTRL, val); + + return ret; } - return ret; + return -1; } static void clock_1GHz(void)