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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <1399357529.5441.4.camel@AMDC1943>
Date:	Tue, 06 May 2014 08:25:29 +0200
From:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
To:	Axel Lin <axel.lin@...ics.com>
Cc:	Sangbeom Kim <sbkim73@...sung.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Sachin Kamat <sachin.kamat@...aro.org>
Subject: Re: [PATCH 2/2] regulator: s2mpa01: Fix accidental enable of buck4
 ramp delay

On pon, 2014-05-05 at 23:27 +0800, Axel Lin wrote:
> 2014-05-05 23:10 GMT+08:00 Krzysztof Kozlowski <k.kozlowski@...sung.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@...sung.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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/