mx53loco: Remove unneeded 'retval' variable

From: Fabio Estevam <[email protected]>

commit c73368150 (pmic: Extend PMIC framework to support multiple instances of PMIC devices) introduced an extra 'retval' variable, but this is not necessary since we have already the variable 'ret' in place.

So use 'ret' to store the return values from the pmic related calls and remove 'retval'.

---
 board/freescale/mx53loco/mx53loco.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

index 63a4f8b..b1bfb90 100644
@@ -345,12 +345,11 @@ static int power_init(void)
 	unsigned int val;
 	int ret = -1;
 	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)

@@ -370,9 +369,9 @@ static int power_init(void)
 	}
 
 	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)

1.7.9.5

Dear Fabio Estevam,

Hm...

I think the hole error code handling is borked in this function.

Assume you enter this branch:

349
350 if (!i2c_probe(CONFIG_SYS_DIALOG_PMIC_I2C_ADDR)) {

This will set "ret":

361 ret = pmic_reg_write(p, DA9053_BUCKCORE_REG, val);
362
363 ret |= pmic_reg_read(p, DA9053_SUPPLY_REG, &val);
...
365 ret |= pmic_reg_write(p, DA9053_SUPPLY_REG, val);
...

368                 ret |= pmic_reg_write(p, DA9053_BUCKPRO_REG, 0x62);
369                 ret |= pmic_reg_write(p, DA9053_SUPPLY_REG, 0x62);

370 }

Assume any of these calls returns an error condition.

Now we enter the second branch:

371
372 if (!i2c_probe(CONFIG_SYS_FSL_PMIC_I2C_ADDR)) {
...

But here we will unconditionally set "ret", no matter what it contained before:

384 ret = pmic_reg_write(p, REG_SW_0, val);

So if both code sections for DIALOG_PMIC_I2C_ADDR and for FSL_PMIC_I2C_ADDR get executed, then any errors in the first part go undetected...

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
An Elephant is a mouse with an Operating System.              - Knuth

Hi Wolfgang,

I think it relies on the fact that only one of the two PMICs is mounted on the board. There are versions of the board with the Dialog PMIC, and other versions with Frescale's. Worse it is, there is no easy way to detect which version of the board is running.

However, only one of the two branch can run, because i2c_probe() fails if the PMIC is not found.

Agree, but physically not possible, until Freescale decides to mount both PMICs on the mx53loco...(but this is a nonsense)

Best regards,
Stefano Babic

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

Yes, correct. We have either Dialog DA9052 or FSL MC34708 on the mx53loco boards, so only one of the branches will run.

Regards,

Fabio Estevam

Dear Fabio Estevam,

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:

index 2c8cb7a..47c7fd9 100644
@@ -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?

Dear Stefano,

The code should be fixed anyway - it is trivial to rewrite such that there are no problems even if both branches would be executed.

Please note that the test is a plain i2c_probe(), so any other I2C device you may attach to such a board can cause a false positive here.

Thinking again about this, the approach of using i2c_probe() is kind of questionable.

Who guarantees that no other I2C device has been attached that uses the "free" address?

The needed change is small, and defensive programming has always been a good idea. Please let's have this fixed.

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
"Here's a fish hangs in the net like a poor man's right in  the  law.
'Twill hardly come out."     - Shakespeare, Pericles, Act II, Scene 1

Hi Wolfgang,

Agree - this makes simpler to understand the code even without knowing mx53loco's schematics.

I am not sure about this. i2c_probe() calls i2c_read for the specific address, and for the specific bus. The case you mention could happen if we attach an I2C device using the expansion port connector (J13). However, on J13 the second controller (I2C-2) is connected, and the PMIC is attached to I2C-1. Due to different instances of the I2C busses, the case cannot happen.

This does not mean that we cannot rewrite the code to make it more readable ;-)

Well, IMHO a preferred approach is to recognize which PMIC is mounted and only then trying to access to it. But there is nothing on the board to detect at runtime which PMIC is mounted, and Freescale's PMIC simply substitutes the Dialog's on the I2C bus with another address. I think there is not another way to detect which PMIC is on board except to try to access it with i2c_probe(), even with its limitations. We cannot distinguish if i2c_probe() fails because the PMIC is not mounted or it does not answer to the bus.

Hardware limitations on the board. But surely it is bad if someone takes this board as reference for code, and then implements his own custom board introducing new errors.

Agree on that

Best regards,
Stefano Babic

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================