Patchwork [U-Boot] mx53loco: Remove unneeded 'retval' variable

login
register
mail settings
Submitter Wolfgang Denk
Date Dec. 27, 2012, 7:27 p.m.
Message ID <20121227192738.41ED2200622@gemini.denx.de>
Download mbox | patch
Permalink /patch/208354/
State Changes Requested
Delegated to: Stefano Babic
Headers show

Comments

Wolfgang Denk - Dec. 27, 2012, 7:27 p.m.
Dear Fabio Estevam,

In message <CAOMZO5DkcNEzw0pHUVWUHD6q9uBJCLMP4KKe1Wecpuaurs+L2g@mail.gmail.com> 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

Patch

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)