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

[Wolfgang Denk]

Dear Fabio Estevam,

In message <CAOMZO5DkcNEzw0pHUVWUHD6q9uBJCLMP4KKe1Wecpuaurs+L2g at 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:

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)



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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Do you suppose the reason the ends of the `Intel Inside'  logo  don't
match up is that it was drawn on a Pentium?