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@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.

On Thu, Jan 15, 2015 at 8:18 AM, Peng Fan Peng.Fan@freescale.com wrote:

@@ -515,11 +516,16 @@ int board_spi_cs_gpio(unsigned bus, unsigned cs) int power_init_board(void) { struct pmic *p;

•   unsigned int ret;
  
    p = pfuze_common_init(I2C_PMIC);
    if (!p)
            return -ENODEV;
  
  

•   ret = pfuze_mode_init(p, APS_PFM);
  

•   if (ret < 0)
  

•           return -EIO;
  

You should return 'ret' here instead.

•   return 0;
  

}

diff --git a/board/freescale/mx6sabresd/mx6sabresd.c b/board/freescale/mx6sabresd/mx6sabresd.c index 2f7198d..59544d9 100644 --- a/board/freescale/mx6sabresd/mx6sabresd.c +++ b/board/freescale/mx6sabresd/mx6sabresd.c @@ -631,12 +631,16 @@ int board_init(void) int power_init_board(void) { struct pmic *p;

•   unsigned int reg;
  

•   unsigned int reg, ret;
  
    p = pfuze_common_init(I2C_PMIC);
    if (!p)
            return -ENODEV;
  
  

•   ret = pfuze_mode_init(p, APS_PFM);
  

•   if (ret < 0)
  

•           return -EIO;
  

Same here.

•   /* Increase VGEN3 from 2.5 to 2.8V */
    pmic_reg_read(p, PFUZE100_VGEN3VOL, &reg);
    reg &= ~LDO_VOL_MASK;
  

diff --git a/board/freescale/mx6sxsabresd/mx6sxsabresd.c b/board/freescale/mx6sxsabresd/mx6sxsabresd.c index 5cc58ac..fbf3337 100644 --- a/board/freescale/mx6sxsabresd/mx6sxsabresd.c +++ b/board/freescale/mx6sxsabresd/mx6sxsabresd.c @@ -199,12 +199,16 @@ static struct i2c_pads_info i2c_pad_info1 = { int power_init_board(void) { struct pmic *p;

•   unsigned int reg;
  

•   unsigned int reg, ret;
  
    p = pfuze_common_init(I2C_PMIC);
    if (!p)
            return -ENODEV;
  
  

•   ret = pfuze_mode_init(p, APS_PFM);
  

•   if (ret < 0)
  

•           return -EIO;
  

Same here.