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

[Stefano Babic <sbabic@denx.de>]

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