[U-Boot] [PATCH v2 2/3] pmic:pfuze implement regulator mode set

[Peng Fan]

Hi, Przemyslaw

On 1/15/2015 10:58 PM, Przemyslaw Marczak wrote:
> Hello Peng,
>
> On 01/15/2015 11:18 AM, Peng Fan wrote:
>> This patch is to implement pfuze_mode_init and pfuze_regulator_mode_set
>> function, and add prototype in header file.
>>
>> pfuze_mode_init is to set switching mode for all buck regulators to
>> improve system efficiency.
>> pfuze_regulator_mode_set is to set the regulator's mode according input
>> parameter.
>>
>> Mode:
>> OFF: The regulator is switched off and the output voltage is discharged.
>> PFM: In this mode, the regulator is always in PFM mode, which
>>       is useful at light loads for optimized efficiency.
>> PWM: In this mode, the regulator is always in PWM mode operation
>>       regardless of load conditions.
>> APS: In this mode, the regulator moves automatically between
>>       pulse skipping mode and PWM mode depending on load conditions.
>>
>> Signed-off-by: Peng Fan <Peng.Fan at freescale.com>
>> ---
>>
>> Changes v2:
>>   implement one function mode for one regulator.
>>
>>   board/freescale/common/pfuze.c | 43 
>> ++++++++++++++++++++++++++++++++++++++++++
>>   board/freescale/common/pfuze.h |  2 ++
>>   2 files changed, 45 insertions(+)
>>
>> diff --git a/board/freescale/common/pfuze.c 
>> b/board/freescale/common/pfuze.c
>> index 2cd1794..f2fffc1 100644
>> --- a/board/freescale/common/pfuze.c
>> +++ b/board/freescale/common/pfuze.c
>> @@ -8,6 +8,49 @@
>>   #include <power/pmic.h>
>>   #include <power/pfuze100_pmic.h>
>>
>> +/* Set one switch regulator's mode */
>
> This is not exactly what I mean.
> Actually by doing this, we have pmic_reg_write() function with just 
> the other name.
>
>> +int pfuze_regulator_mode_set(struct pmic *p, u32 regulator, u32 mode)
>> +{
>> +    return pmic_reg_write(p, regulator, mode);
>> +}
>
> The 'u32 regulator' suggests to pass the regulator number rather than 
> defined mode register address - little unclear.
>
> And the "pfuze.." function name is general and should be implemented 
> by pfuze driver, which could be 'temporary' implemented in:
>
> drivers/power/pmic/pmic_pfuze100.c
>
> And this driver should take care of the PFUZE id and write the given 
> mode to proper pmic register address.
>
> I mean 'temporary' because, soon we will move to the next pmic 
> framework, finally I hope to send it at the end of the next week.
>
>> +
>> +/* Set all switch regulators' working mode */
>
> So I think, that the below function without the loop could be 
> implemented in the pfuze driver file.
>
>> +int pfuze_mode_init(struct pmic *p, u32 mode)
>
> Starting with some like this:
>
> int pfuze_sw_regulator_mode_set(struct pmic *p, u32 ??switch_num??, 
> u32 mode)
>
>> +{
>> +    unsigned char offset, i, switch_num;
>> +    u32 id, ret;
>> +
>> +    pmic_reg_read(p, PFUZE100_DEVICEID, &id);
>> +    id = id & 0xf;
>> +
>> +    if (id == 0) {
>> +        switch_num = 6;
>> +        offset = PFUZE100_SW1CMODE;
>> +    } else if (id == 1) {
>> +        switch_num = 4;
>> +        offset = PFUZE100_SW2MODE;
>> +    } else {
>> +        printf("Not supported, id=%d\n", id);
>> +        return -1;
>> +    }
>> +
> Ending with something like this:
> ret = pmic_reg_write(p, offset + switch_num * 7, mode);
> if (ret ...
> ...
> }
>
> And this code below can stay in the 'common.c' code, with small changes:
> ret = pfuze_sw_regulator_mode_set(p, 1, mode);
>> +    ret = pfuze_regulator_mode_set(p, PFUZE100_SW1ABMODE, mode);
>> +    if (ret < 0) {
>> +        printf("Set SW1AB mode error!\n");
>> +        return ret;
>> +    }
>> +
>> +    for (i = 0; i < switch_num - 1; i++) {
>
> ret = pfuze_sw_regulator_mode_set(p, i, mode);
>
>> +        ret = pfuze_regulator_mode_set(p, offset + i * 7, mode);
>> +        if (ret < 0) {
>> +            printf("Set switch%x mode error!\n", offset + i * 7);
>> +            return ret;
>> +        }
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>   struct pmic *pfuze_common_init(unsigned char i2cbus)
>>   {
>>       struct pmic *p;
>> diff --git a/board/freescale/common/pfuze.h 
>> b/board/freescale/common/pfuze.h
>> index 7a4126c..7c316c6 100644
>> --- a/board/freescale/common/pfuze.h
>> +++ b/board/freescale/common/pfuze.h
>> @@ -8,5 +8,7 @@
>>   #define __PFUZE_BOARD_HELPER__
>>
>>   struct pmic *pfuze_common_init(unsigned char i2cbus);
>> +int pfuze_mode_init(struct pmic *p, u32 mode);
>
>> +int pfuze_regulator_mode_set(struct pmic *p, u32 regulator, u32 mode);
>
> And this:
>
> int pfuze_sw_regulator_mode_set(...);
>
> should go here:
> include/power/pfuze100_pmic.h
>
>>
>>   #endif
>>
>
> I suggested the 'sw' in the function name for the switching regulator 
> type, so then we can pass just switching regulator number as an 
> argument: e.g. 1,2,3...
>
> And maybe in the future you will need ldo or some else regulator type,
> so adding 'pfuze_ldo_regulator_mode_set' function, will keep the code 
> consistence.
>
> I would like to keep the driver specific code in driver file and 
> common code in the common file.
Thanks for your review.
Get it.  I think this first version is fine for me now, this new feature 
can be done in future patch.
>
> I think it is reasonable. If you now prefer to keep the first version 
> of this patch set, then you have still my ACK for it:).
>
> Best regards,
Regards,
Peng.