[U-Boot] [PATCH] Added MC13892VK Power Management driver

[Stefano Babic <sbabic@denx.de>]

Hi Tom,

Tom wrote:
> Stefano Babic wrote:
>> Added SPI driver for the Power Management  Controller
>> used with i.MX51 Processor.
>>
> 
> Errors in
> imx31_litekeit
> imx31_phycore_eet
> imx31_ads
> These must be fixed

...and I have to test with a MX31, too. I will find a board and test the
changes before sending version 2 of the patch.

>> +
>> +    tmp = pmic_tx;
>> +    for (i = 0; i < 4; i++) {
>> +        tx[i] = (tmp & 0xFF000000) >> 24;
>> +        tmp <<= 8;
>> +    }
> 
> Looks like you are converting for bigendian to little.
> It may be better to use one of the cpu_to_little functions
> see linux/byteorder/generic.h

Thanks, I will check it.

>> +    pmic_rx = (rx[0] << 24) | (rx[1] << 16) | (rx[2] << 8) | rx[3];
> 
> A conversion of little to big.
> Use existing conversion functions

ok, thanks.

>> +    rev_id = mc13892_reg(slave, 7, 0, 0);
> 
> Be consistent on use of mc13892_reg.
> If you have the read/write wrappers, use them.
> From this conext it is difficult to understand that this is a read.

Agree, this is a mistake, I forget to correct it, mc13892_reg() must not
be used outside the driver.

> 
> The return is a u32 value, not a pointer so the modifier 'volatile' is
> not needed in the declaration of rev_id

Ok, got it.


>> +    MC13892_REG_LED_CTL2,
>> +    MC13892_REG_LED_CTL3,
>> +    MC13892_REG_UNUSED12,
>> +    MC13892_REG_UNUSED13,
>> +    MC13892_REG_TRIM0,
>> +    MC13892_REG_TRIM1,
>> +    MC13892_REG_TEST0,
>> +    MC13892_REG_TEST1,        /*60 */
>> +    MC13892_REG_TEST2,
>> +    MC13892_REG_TEST3,
>> +    MC13892_REG_TEST4,
> 
> This should be converted to the reg access through struct members.

I do not know if this helps to make code more readable. We need to send
the address of the internal register via the SPI interface, and these
registers are not mapped into the microprocessor address space.
If I use a structure, I must always convert the address to the register
number before sending via the SPI interface. At the end, I have a
further step that probably makes difficult to understand.

The same approach with register number is actually used for
drivers/rtc/mc13873-rtc.c, whose interface is quite similar as this one,
even if in this case the register number is directly used. I think it is
better to have the same approach for similar chips.

> These wrappers add an unneeded layer of indirection
> There should be real read / write funtions

Agree, I will remove that indirection and avoid to export mc13892_reg.

Stefano

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