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

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

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

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'.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 board/freescale/mx53loco/mx53loco.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/board/freescale/mx53loco/mx53loco.c b/board/freescale/mx53loco/mx53loco.c
index 63a4f8b..b1bfb90 100644
--- a/board/freescale/mx53loco/mx53loco.c
+++ b/board/freescale/mx53loco/mx53loco.c
@@ -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

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

[Wolfgang Denk]

Dear Fabio Estevam,

In message <1356604017-9699-1-git-send-email-festevam@gmail.com> you wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> 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'.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

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

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

[Stefano Babic]

On 27/12/2012 11:35, Wolfgang Denk wrote:
> Dear Fabio Estevam,
> 

Hi Wolfgang,

> In message <1356604017-9699-1-git-send-email-festevam@gmail.com> you wrote:
>> From: Fabio Estevam <fabio.estevam@freescale.com>
>>
>> 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'.
>>
>> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> 
> 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)) {
> ...
> 

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.

> But here we will unconditionally set "ret", no matter what it
> contained before:
> 
> 384                 ret = pmic_reg_write(p, REG_SW_0, val);

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
=====================================================================

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

[Fabio Estevam]

On Thu, Dec 27, 2012 at 9:05 AM, Stefano Babic <sbabic@denx.de> wrote:

> 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.

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

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

[Wolfgang Denk]

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:

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?

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

[Wolfgang Denk]

Dear Stefano,

In message <50DC2B8F.2030709@denx.de> you wrote:
>
> 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.

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.

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

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

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

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

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

[Stefano Babic]

On 27/12/2012 20:14, Wolfgang Denk wrote:
> Dear Stefano,
> 

Hi Wolfgang,

> In message <50DC2B8F.2030709@denx.de> you wrote:
>>
>> 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.
> 
> The code should be fixed anyway - it is trivial to rewrite such that
> there are no problems even if both branches would be executed.
> 

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

> 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.

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 ;-)

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

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.

> 
>> However, only one of the two branch can run, because i2c_probe() fails
>> if the PMIC is not found.
> 
> Who guarantees that no other I2C device has been attached that uses
> the "free" address?

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, but physically not possible, until Freescale decides to mount
>> both PMICs on the mx53loco...(but this is a nonsense)
> 
> The needed change is small, and defensive programming has always been
> a good idea.  Please let's have this fixed.

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
=====================================================================