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

[Axel Lin]

2014-05-05 23:10 GMT+08:00 Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>:
> 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@xxxxxxxxxxx>
> 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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/