Message ID | 1356604017-9699-1-git-send-email-festevam@gmail.com |
---|---|
State | Changes Requested |
Headers | show |
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
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
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
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
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 --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)