Re: [PATCH 2/2] regulator: s2mpa01: Fix accidental enable of buck4 ramp delay

[Krzysztof Kozlowski <k.kozlowski@samsung.com>]

On pon, 2014-05-05 at 23:27 +0800, Axel Lin wrote:
> 2014-05-05 23:10 GMT+08:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> > S2MPA01 supports enabling/disabling ramp delay only for buck[1234].
> > Other bucks have ramp delay enabled always.
> >
> > However the bit shift for enabling buck4 ramp delay in register is equal
> > to 0. When ramp delay was set for these other bucks (buck[56789] and
> > buck10), the ramp delay for buck4 was also enabled.
> >
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Fixes: f7b1a8dc1c1c ("regulator: s2mpa01: Don't check enable_shift before setting enable ramp rate")
> > ---
> >  drivers/regulator/s2mpa01.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/regulator/s2mpa01.c b/drivers/regulator/s2mpa01.c
> > index f19a30f0fb42..901504880ab6 100644
> > --- a/drivers/regulator/s2mpa01.c
> > +++ b/drivers/regulator/s2mpa01.c
> > @@ -99,7 +99,8 @@ static int s2mpa01_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
> >  {
> >         struct s2mpa01_info *s2mpa01 = rdev_get_drvdata(rdev);
> >         unsigned int ramp_val, ramp_shift, ramp_reg = S2MPA01_REG_RAMP2;
> > -       unsigned int ramp_enable = 1, enable_shift = 0;
> > +       unsigned int ramp_enable = 1;
> > +       int enable_shift = -1;
> Hi Krzysztof,
> This makes the meaning of enable_shift looks confusing.
> I think below change has better readability.
> 
> diff --git a/drivers/regulator/s2mpa01.c b/drivers/regulator/s2mpa01.c
> index f19a30f..eb5ce18 100644
> --- a/drivers/regulator/s2mpa01.c
> +++ b/drivers/regulator/s2mpa01.c
> @@ -192,11 +192,15 @@ static int s2mpa01_set_ramp_delay(struct
> regulator_dev *rdev, int ramp_delay)
>   if (!ramp_enable)
>   goto ramp_disable;
> 
> - ret = regmap_update_bits(rdev->regmap, S2MPA01_REG_RAMP1,
> - 1 << enable_shift, 1 << enable_shift);
> - if (ret) {
> - dev_err(&rdev->dev, "failed to enable ramp rate\n");
> - return ret;
> + /* S2MPA01 supports enabling/disabling ramp delay only for buck[1234] */
> + if (rdev->desc->id >= S2MPA01_BUCK1 &&
> +    rdev->desc->id <= S2MPA01_BUCK4) {
> + ret = regmap_update_bits(rdev->regmap, S2MPA01_REG_RAMP1,
> + 1 << enable_shift, 1 << enable_shift);
> + if (ret) {
> + dev_err(&rdev->dev, "failed to enable ramp rate\n");
> + return ret;
> + }
>   }
> 
>   ramp_val = get_ramp_delay(ramp_delay);

Yes, it looks better for S2MPA01. Thanks for idea, I'll send v2. 

Best regards,
Krzysztof