RE: [PATCH v1 1/2] regulator: pca9450: add enable_val for all bucks

[Robin Gong]

On 2020/08/31 18:53 Marco Felsch <m.felsch@xxxxxxxxxxxxxx> wrote:
> Hi Robin,
> 
> On 20-09-01 00:48, Robin Gong wrote:
> > BuckX enable mode
> > 00b = OFF
> > 01b = ON by PMIC_ON_REQ = H
> > 10b = ON by PMIC_ON_REQ = H && PMIC_STBY_REQ = L 11b = Always ON
> >
> > For such enable mode, enable_value should be clearly set in requlator
> > desc,
> > 00/11 is not enough, correct it now for different bucks. For example,
> > buck2 is designed for vddarm which could be off in 'PMIC_STBY_REQ = H'
> > after kernel enter suspend, so should be set '10b' as ON, while others is '01b'
> as ON.
> > All are the same as the default setting which means bucks no need to
> > be enabled again during kernel boot even if they have been enabled
> > already after pmic on.
> 
> I wouldn't hard-code the regulator behaviour because the behaviour comes
> from the system design which in most cases are comming from our hw-guys.
> Till now I saw a few intelligent designs don't following the pmic user
> recommendations to save money. I would love to specify the regulator
> behaviour/mode within the dt or acpi.
Well, if so the better way is moving into dts. Will implement it in v2.

> 
> > Signed-off-by: Robin Gong <yibin.gong@xxxxxxx>
> > ---
> >  drivers/regulator/pca9450-regulator.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/regulator/pca9450-regulator.c
> > b/drivers/regulator/pca9450-regulator.c
> > index eb5822b..79f2a5a 100644
> > --- a/drivers/regulator/pca9450-regulator.c
> > +++ b/drivers/regulator/pca9450-regulator.c
> > @@ -249,6 +249,7 @@ static const struct pca9450_regulator_desc
> pca9450a_regulators[] = {
> >  			.vsel_mask = BUCK1OUT_DVS0_MASK,
> >  			.enable_reg = PCA9450_REG_BUCK1CTRL,
> >  			.enable_mask = BUCK1_ENMODE_MASK,
> > +			.enable_val = BUCK_ENMODE_ONREQ,
> >  			.owner = THIS_MODULE,
> >  			.of_parse_cb = pca9450_set_dvs_levels,
> >  		},
> > @@ -273,7 +274,8 @@ static const struct pca9450_regulator_desc
> pca9450a_regulators[] = {
> >  			.vsel_reg = PCA9450_REG_BUCK2OUT_DVS0,
> >  			.vsel_mask = BUCK2OUT_DVS0_MASK,
> >  			.enable_reg = PCA9450_REG_BUCK2CTRL,
> > -			.enable_mask = BUCK1_ENMODE_MASK,
> > +			.enable_mask = BUCK2_ENMODE_MASK,
> 
> Unrelated change?
Yes, that's just correct it minor literal since that's BUCK2 although
they're the same, will split it anyway.