From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefano Babic <sbabic@denx.de>
Date: Wed, 19 Oct 2011 17:39:24 +0200
Subject: [U-Boot] [PATCH 13/13] misc: pmic: drop old Freescale's pmic
 driver
In-Reply-To: <4E9ECF4F.5050802@hale.at>
References: <1318091769-30979-1-git-send-email-sbabic@denx.de>
	<1318091769-30979-14-git-send-email-sbabic@denx.de>
	<4E9ECF4F.5050802@hale.at>
Message-ID: <4E9EEF2C.9050906@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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<sbabic@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: office at denx.de
=====================================================================