diff mbox

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

Message ID 1356604017-9699-1-git-send-email-festevam@gmail.com
State Changes Requested
Headers show

Commit Message

Fabio Estevam Dec. 27, 2012, 10:26 a.m. UTC 
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(-)

Comments

Wolfgang Denk Dec. 27, 2012, 10:35 a.m. UTC | #1 
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
Stefano Babic Dec. 27, 2012, 11:05 a.m. UTC | #2 
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
Fabio Estevam Dec. 27, 2012, 11:14 a.m. UTC | #3 
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
Wolfgang Denk Dec. 27, 2012, 7:14 p.m. UTC | #4 
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
Stefano Babic Dec. 28, 2012, 8:18 a.m. UTC | #5 
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
diff mbox

Patch

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)