On 10/19/2011 03:23 PM, Helmut Raiger wrote: > On 10/08/2011 06:36 PM, Stefano Babic wrote: >> Signed-off-by: Stefano Babic<sba...@denx.de> >> --- >> drivers/misc/Makefile | 1 - >> drivers/misc/fsl_pmic.c | 235 >> ----------------------------------------------- >> 2 files changed, 0 insertions(+), 236 deletions(-) >> delete mode 100644 drivers/misc/fsl_pmic.c >>
Hi Helmut, > I just checked PMIC action on our board (i.mx31 and mc13783) and the new > code is not working here, it even: > > TT01> pmic write 20 17 > raise: Signal # 8 caught > <reg num> = 32 is invalid. Should be less than 0 > TT01> pmic read 20 > <reg num> = 32 is invalid. Should be less than 0 > PMIC: Register read failed > > 0x20: 0x00000000 > > At first glance I found in pmic_fsl.c: > > static u32 pmic_spi_prepare_tx(u32 reg, u32 *val, u32 write) > { > if ((val == NULL) && (write)) > return *val & ~(1 << 31); > else > return (write << 31) | (reg << 25) | (*val & 0x00FFFFFF); > } > > which must be wrong. NULL is de-referenced in both cases and this error > is even forced by pmic_spi.c: This is wrong - it is checked for NULL and then de-referenced... > > if (write) { > pmic_tx = p->hw.spi.prepare_tx(0, NULL, write); > pmic_tx &= ~(1 << 31); > > > Probably val == NULL was meant as escape not to touch the pmic_tx value, > in the original driver it's done that way. Then there is too much statements. If spi.prepare_tx() should clear the MSB with ~(1 << 31), why we need the second one : pmic_tx &= ~(1 << 31); something went simply wrong... Because (from the old driver) the second SPI transfer is done to read back the programmed value, we can maybe use : if (write) { pmic_tx = p->hw.spi.prepare_tx(reg, &val, 0); and of course, we should fix the wrong prepare_tx().. > One could fix this by using a static variable in pmic_spi_prepare_tx(), > but I'm > not sure if this was the intention. > Well, it makes no sense - the prepare_tx() should not know the history, and only rely on the parameters. > I wonder why it was missed during testing as it seems configuration > independent. I wonder, too. I have tested with the 'date' command, that means writing into the internal RTC registers of the MC13783. I cannot explain what goes wrong, but of course the code is buggy and must be fixed. 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-0 Fax: +49-8142-66989-80 Email: off...@denx.de ===================================================================== _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot