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

[Przemyslaw Marczak]

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.

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,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com