[U-Boot] [PATCH V7 2/3] PMIC: Add dialog pmic support

[Jason Liu]

Hi, Stefano,

2011/5/12 Stefano Babic <sbabic at denx.de>:
> On 05/11/2011 10:03 AM, Jason Liu wrote:
>> This patch add dialog pmic(DA9053) driver with I2C interface support
>> In order to not duplicate code and according to the discussion on the
>> mail-list, change fsl_pmic.c to spi_i2c_pmic.c.Actaully,spi_i2c_pmic.c
>> is just a wrapper for PMIC communication when SPI or I2C is used.
>>
>> Signed-off-by: Jason Liu <jason.hui at linaro.org>
>> Cc: sbabic at denx.de <sbabic at denx.de>
>> Cc: Detlev Zundel <dzu at denx.de>
>
> Hi Jason,
>
>> --- a/drivers/misc/fsl_pmic.c
>> +++ b/drivers/misc/spi_i2c_pmic.c
>> @@ -22,13 +22,16 @@
>>
>>  #include <config.h>
>>  #include <common.h>
>> +#include <i2c.h>
>>  #include <asm/errno.h>
>>  #include <linux/types.h>
>> +#if defined(CONFIG_FSL_PMIC)
>>  #include <fsl_pmic.h>
>> +#endif
>>
>> -static int check_param(u32 reg, u32 write)
>> +static int check_param(u32 reg, u32 write, u32 max_reg)
>>  {
>> -     if (reg > 63 || write > 1) {
>> +     if (reg > max_reg || write > 1) {
>>               printf("<reg num> = %d is invalid. Should be less then 63\n",
>>                       reg);
>>               return -1;
>> @@ -37,15 +40,13 @@ static int check_param(u32 reg, u32 write)
>>       return 0;
>>  }
>>
>> -#ifdef CONFIG_FSL_PMIC_I2C
>> -#include <i2c.h>
>> -
>> -u32 pmic_reg(u32 reg, u32 val, u32 write)
>> +#if defined(CONFIG_FSL_PMIC_I2C)
>> +u32 fsl_pmic_reg(u32 reg, u32 val, u32 write)
>>  {
>> -     unsigned char buf[4] = { 0 };
>>       u32 ret_val = 0;
>> +     unsigned char buf[4] = { 0 };
>>
>> -     if (check_param(reg, write))
>> +     if (check_param(reg, write, 63))
>>               return -1;
>>
>>       if (write) {
>> @@ -62,7 +63,44 @@ u32 pmic_reg(u32 reg, u32 val, u32 write)
>>
>>       return ret_val;
>>  }
>> -#else /* SPI interface */
>> +#endif
>> +
>> +#if defined(CONFIG_DIALOG_PMIC_I2C)
>> +u32 dlg_pmic_reg(u32 reg, u32 val, u32 write)
>> +{
>> +     u32 ret_val = 0;
>> +     unsigned char buf[1] = { 0 };
>> +
>> +     if (check_param(reg, write, 142))
>> +             return -1;
>> +
>> +     if (write) {
>> +             buf[0] = (val) & 0xff;
>> +             if (i2c_write(CONFIG_SYS_DIALOG_PMIC_I2C_ADDR, reg, 1, buf, 1))
>> +                     return -1;
>> +     } else {
>> +             if (i2c_read(CONFIG_SYS_DIALOG_PMIC_I2C_ADDR, reg, 1, buf, 1))
>> +                     return -1;
>> +             ret_val = buf[0];
>> +     }
>> +
>> +     return ret_val;
>
> This is not what I meant. You have duplicated the code, instead of merge
> the two functions together. And I think the switch is wrong.
> This file provides a general access to PMIc using SPI/I2C. There should
> not be #ifdef related to a single PMIC. Instead of that, You need
> additional CONFIG_ to select how to access the PMIC (8 bit or 32 bit).

Then can we have the CONFIG_SYS_DIALOG_PMIC_I2C_ADDR or
CONFIG_SYS_FSL_PMIC_I2C_ADDR in this file?

Please tell clear about your mean, don't let me guess what you mean?

>
> IMHO we can rid of the check_param() function, as this is a constraint
> to have an implementation independent from a single PMIC.

If you think we don't need check_param, then It's ok. I can remove it.
Since this function is added by you before.

>
> I would prefer something like this:
>
> u32 pmic_reg(u32 reg, u32 val, u32 write) {
>
>        ........
> #ifdef CONFIG_SYS_PMIC_8BIT
>
>        <read / write 8 bit register>
> #else
>
>        <actual implementation for fsl PMICs>
> #endif

Then what you prefer is function pointer or something else?
Please tell clear, otherwise, it will cost another time to do it.

>
>
>> +#if defined(CONFIG_FSL_PMIC_I2C) || defined(CONFIG_DIALOG_PMIC_I2C)
>
> Same comments apply here. We should select only between I2C and SPI, not
> the chip.
>
>> +#if defined(CONFIG_FSL_PMIC)
>> +#define PMIC_NAME "Freescale PMIC (Atlas)"
>> +#elif defined(CONFIG_DIALOG_PMIC)
>> +#define PMIC_NAME "Dialog PMIC (DA905x)"
>> +#else
>> +#error "Unkown PMIC name"
>> +#endif
>
> Instead of that, we can set a general name or put the PMIC_NAME inside
> the specific PMIC header file.

Then what general name you think it's good?

I have seen the painful for restructure with the second time, another
is the make imximage.
Can we avoid it in the future?

And BTW, do you have any comments about the 1/3 clock patch? If you
have, please tell it
as early as possible and I don't want to let the patch version goes up
very bigger and the time endless.

Thanks,

Jason

>
> Best regards,
> 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
> =====================================================================
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>