[U-Boot] [PATCH 4/4] imx: mx6: Set Pfuze mode to decrease power number for DSM

[Li Ye-B37916 <b37916@freescale.com>]

Hi Przemyslaw,

On 11/4/2014 11:57 PM, Przemyslaw Marczak wrote:
> Hello Ye Li,
>
> On 09/10/2014 11:08 AM, Ye.Li wrote:
>> Set all switches APS mode in normal and PFM mode in standby. So when
>> mx6 entering DSM mode, the power number can be decreased. There is
>> no impact for mx6 in run mode.
>>
>> Changes for boards:
>> -mx6 sabreauto
>> -mx6 sabresd
>> -mx6slevk
>> -mx6sxsabresd
>>
>> Signed-off-by: Ye.Li <B37916@freescale.com>
>> ---
>>   board/freescale/mx6qsabreauto/mx6qsabreauto.c |   36 +++++++++++++++++++++++++
>>   board/freescale/mx6sabresd/mx6sabresd.c       |   36 +++++++++++++++++++++++++
>>   board/freescale/mx6slevk/mx6slevk.c           |   36 +++++++++++++++++++++++++
>>   board/freescale/mx6sxsabresd/mx6sxsabresd.c   |   36 +++++++++++++++++++++++++
>>   4 files changed, 144 insertions(+), 0 deletions(-)
>>
>> diff --git a/board/freescale/mx6qsabreauto/mx6qsabreauto.c b/board/freescale/mx6qsabreauto/mx6qsabreauto.c
>> index 76b024b..9e79915 100644
>> --- a/board/freescale/mx6qsabreauto/mx6qsabreauto.c
>> +++ b/board/freescale/mx6qsabreauto/mx6qsabreauto.c
>> @@ -263,6 +263,37 @@ int board_init(void)
>>       return 0;
>>   }
>>
>> +/* set all switches APS in normal and PFM mode in standby */
>
> I think that the "chip" is quite misleading.
> Just use chipid and check the real value instead of using it as bool.
> And the PF100 DEVICEID should be defined in pf100 header.
>
Accept. Will change it in next version.

>> +static int pfuze_setup_mode(struct pmic *p, int chip)
>> +{
>> +    unsigned char offset, i, switch_num, value;
>> +
>> +    if (!chip) {
>> +        /* pfuze100 */
>> +        switch_num = 6;
>> +        offset = 0x31;
>> +    } else {
>> +        /* pfuze200 */
>> +        switch_num = 4;
>> +        offset = 0x38;
>> +    }
>> +
>> +    value = 0xc;
>
> If you change "magic" values (e.g. 0xc) with proper defines in pmic header, then it could be reused in the future and the code will be more readable.
>
Ok.

>> +    if (pmic_reg_write(p, 0x23, value)) {
>> +        printf("Set SW1AB mode error!\n");
>> +        return -1;
>> +    }
>> +
>> +    for (i = 0; i < switch_num - 1; i++) {
>> +        if (pmic_reg_write(p, offset + i * 7, value)) {
>> +            printf("Set switch%x mode error!\n", offset);
>> +            return -1;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>
> The function above is duplicated few times in each board code.
> Why not make it common and just one?
>

I already have made it as a common function in v4. The patch you reviewed seems not the latest one.  I will address your other comments and send out v5.

>> +
>>   static int pfuze_init(void)
>>   {
>>       struct pmic *p;
>> @@ -281,6 +312,11 @@ static int pfuze_init(void)
>>       pmic_reg_read(p, PFUZE100_DEVICEID, &reg);
>>       printf("PMIC:  PFUZE100 ID=0x%02x\n", reg);
>>
>> +    if (pfuze_setup_mode(p, (reg & 0xf))) {
>> +        printf("setup pfuze mode error!\n");
>> +        return -1;
>> +    }
>> +
>>       /* Set SW1AB stanby volage to 0.975V */
>>       pmic_reg_read(p, PFUZE100_SW1ABSTBY, &reg);
>>       reg &= ~0x3f;
>> diff --git a/board/freescale/mx6sabresd/mx6sabresd.c b/board/freescale/mx6sabresd/mx6sabresd.c
>> index 72d6562..810fe13 100644
>> --- a/board/freescale/mx6sabresd/mx6sabresd.c
>> +++ b/board/freescale/mx6sabresd/mx6sabresd.c
>> @@ -456,6 +456,37 @@ int board_init(void)
>>       return 0;
>>   }
>>
>> +/* set all switches APS in normal and PFM mode in standby */
>> +static int pfuze_setup_mode(struct pmic *p, int chip)
>> +{
>> +    unsigned char offset, i, switch_num, value;
>> +
>> +    if (!chip) {
>> +        /* pfuze100 */
>> +        switch_num = 6;
>> +        offset = 0x31;
>> +    } else {
>> +        /* pfuze200 */
>> +        switch_num = 4;
>> +        offset = 0x38;
>> +    }
>> +
>> +    value = 0xc;
>> +    if (pmic_reg_write(p, 0x23, value)) {
>> +        printf("Set SW1AB mode error!\n");
>> +        return -1;
>> +    }
>> +
>> +    for (i = 0; i < switch_num - 1; i++) {
>> +        if (pmic_reg_write(p, offset + i * 7, value)) {
>> +            printf("Set switch%x mode error!\n", offset);
>> +            return -1;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static int pfuze_init(void)
>>   {
>>       struct pmic *p;
>> @@ -475,6 +506,11 @@ static int pfuze_init(void)
>>       printf("PMIC:  PFUZE%s ID=0x%02x\n",
>>           ((reg & 0xf) == 0) ? "100" : "200", reg);
>>
>> +    if (pfuze_setup_mode(p, (reg & 0xf))) {
>> +        printf("setup pfuze mode error!\n");
>> +        return -1;
>> +    }
>> +
>>       /* Increase VGEN3 from 2.5 to 2.8V */
>>       pmic_reg_read(p, PFUZE100_VGEN3VOL, &reg);
>>       reg &= ~0xf;
>> diff --git a/board/freescale/mx6slevk/mx6slevk.c b/board/freescale/mx6slevk/mx6slevk.c
>> index 8b6a79c..fe5e37d 100644
>> --- a/board/freescale/mx6slevk/mx6slevk.c
>> +++ b/board/freescale/mx6slevk/mx6slevk.c
>> @@ -195,6 +195,37 @@ int board_init(void)
>>       return 0;
>>   }
>>
>> +/* set all switches APS in normal and PFM mode in standby */
>> +static int pfuze_setup_mode(struct pmic *p, int chip)
>> +{
>> +    unsigned char offset, i, switch_num, value;
>> +
>> +    if (!chip) {
>> +        /* pfuze100 */
>> +        switch_num = 6;
>> +        offset = 0x31;
>> +    } else {
>> +        /* pfuze200 */
>> +        switch_num = 4;
>> +        offset = 0x38;
>> +    }
>> +
>> +    value = 0xc;
>> +    if (pmic_reg_write(p, 0x23, value)) {
>> +        printf("Set SW1AB mode error!\n");
>> +        return -1;
>> +    }
>> +
>> +    for (i = 0; i < switch_num - 1; i++) {
>> +        if (pmic_reg_write(p, offset + i * 7, value)) {
>> +            printf("Set switch%x mode error!\n", offset);
>> +            return -1;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static int pfuze_init(void)
>>   {
>>       struct pmic *p;
>> @@ -214,6 +245,11 @@ static int pfuze_init(void)
>>       printf("PMIC:  PFUZE%s ID=0x%02x\n",
>>           ((reg & 0xf) == 0) ? "100" : "200", reg);
>>
>> +    if (pfuze_setup_mode(p, (reg & 0xf))) {
>> +        printf("setup pfuze mode error!\n");
>> +        return -1;
>> +    }
>> +
>>       /* Set SW1AB stanby volage to 0.975V */
>>       pmic_reg_read(p, PFUZE100_SW1ABSTBY, &reg);
>>       reg &= ~0x3f;
>> diff --git a/board/freescale/mx6sxsabresd/mx6sxsabresd.c b/board/freescale/mx6sxsabresd/mx6sxsabresd.c
>> index 80d2d99..d6a33cd 100644
>> --- a/board/freescale/mx6sxsabresd/mx6sxsabresd.c
>> +++ b/board/freescale/mx6sxsabresd/mx6sxsabresd.c
>> @@ -170,6 +170,37 @@ struct i2c_pads_info i2c_pad_info1 = {
>>       },
>>   };
>>
>> +/* set all switches APS in normal and PFM mode in standby */
>> +static int pfuze_setup_mode(struct pmic *p, int chip)
>> +{
>> +    unsigned char offset, i, switch_num, value;
>> +
>> +    if (!chip) {
>> +        /* pfuze100 */
>> +        switch_num = 6;
>> +        offset = 0x31;
>> +    } else {
>> +        /* pfuze200 */
>> +        switch_num = 4;
>> +        offset = 0x38;
>> +    }
>> +
>> +    value = 0xc;
>> +    if (pmic_reg_write(p, 0x23, value)) {
>> +        printf("Set SW1AB mode error!\n");
>> +        return -1;
>> +    }
>> +
>> +    for (i = 0; i < switch_num - 1; i++) {
>> +        if (pmic_reg_write(p, offset + i * 7, value)) {
>> +            printf("Set switch%x mode error!\n", offset);
>> +            return -1;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static int pfuze_init(void)
>>   {
>>       struct pmic *p;
>> @@ -188,6 +219,11 @@ static int pfuze_init(void)
>>       pmic_reg_read(p, PFUZE100_DEVICEID, &reg);
>>       printf("PMIC:  PFUZE100 ID=0x%02x\n", reg);
>>
>> +    if (pfuze_setup_mode(p, (reg & 0xf))) {
>> +        printf("setup pfuze mode error!\n");
>> +        return -1;
>> +    }
>> +
>>       /* Set SW1AB standby voltage to 0.975V */
>>       pmic_reg_read(p, PFUZE100_SW1ABSTBY, &reg);
>>       reg &= ~0x3f;
>>
>
> And as Fabio wrote, please return real error values or use errno instead of "-1".
>
Have fixed it in v4 patch.

> Best regards,

Best regards,
Ye Li