| Message ID | 1387078339.22351.2.camel@phoenix |
|---|---|
| State | Changes Requested |
| Headers | show |
2013/12/15 Axel Lin <axel.lin@ingics.com>: > Current code only works when pdev->id is 1. Fix it by passing correct bitvalues > argument to abx500_mask_and_set_register_interruptible. > > Having defines for DISABLE_PWM/ENABLE_PWM does not make the code better > in readability because the bitvalues depends on pdev->id. > Thus drop defines for DISABLE_PWM/ENABLE_PWM. > > This patch also removes a unnecessary return in ab8500_pwm_disable. Hello Linus, I don't have the hardware to test and Arun's email bounce back. So I'd appreciate if you can review and test this patch. Thanks, Axel -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Dec 15, 2013 at 11:32:19AM +0800, Axel Lin wrote: [...] s/pwm/PWM/ in the subject, please. [...] > @@ -64,7 +60,7 @@ static int ab8500_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > > ret = abx500_mask_and_set_register_interruptible(chip->dev, > AB8500_MISC, AB8500_PWM_OUT_CTRL7_REG, > - 1 << (chip->base - 1), ENABLE_PWM); > + 1 << (chip->base - 1), 1 << (chip->base - 1)); I think we really need to find some other way to do this. At some point I imagine board will move to DT, at which point we can no longer rely on chip->base being set to a meaningful value. Thierry
On Sun, Dec 15, 2013 at 4:32 AM, Axel Lin <axel.lin@ingics.com> wrote: > Current code only works when pdev->id is 1. Fix it by passing correct bitvalues > argument to abx500_mask_and_set_register_interruptible. > > Having defines for DISABLE_PWM/ENABLE_PWM does not make the code better > in readability because the bitvalues depends on pdev->id. > Thus drop defines for DISABLE_PWM/ENABLE_PWM. > > This patch also removes a unnecessary return in ab8500_pwm_disable. > > Signed-off-by: Axel Lin <axel.lin@ingics.com> > --- > drivers/pwm/pwm-ab8500.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/pwm/pwm-ab8500.c b/drivers/pwm/pwm-ab8500.c > index 1d07a6f..d51dc15 100644 > --- a/drivers/pwm/pwm-ab8500.c > +++ b/drivers/pwm/pwm-ab8500.c > @@ -20,10 +20,6 @@ > #define AB8500_PWM_OUT_CTRL2_REG 0x61 > #define AB8500_PWM_OUT_CTRL7_REG 0x66 > > -/* backlight driver constants */ > -#define ENABLE_PWM 1 > -#define DISABLE_PWM 0 > - > struct ab8500_pwm_chip { > struct pwm_chip chip; > }; > @@ -64,7 +60,7 @@ static int ab8500_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > > ret = abx500_mask_and_set_register_interruptible(chip->dev, > AB8500_MISC, AB8500_PWM_OUT_CTRL7_REG, > - 1 << (chip->base - 1), ENABLE_PWM); > + 1 << (chip->base - 1), 1 << (chip->base - 1)); > if (ret < 0) > dev_err(chip->dev, "%s: Failed to enable PWM, Error %d\n", > pwm->label, ret); > @@ -77,11 +73,10 @@ static void ab8500_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > > ret = abx500_mask_and_set_register_interruptible(chip->dev, > AB8500_MISC, AB8500_PWM_OUT_CTRL7_REG, > - 1 << (chip->base - 1), DISABLE_PWM); > + 1 << (chip->base - 1), 0); > if (ret < 0) > dev_err(chip->dev, "%s: Failed to disable PWM, Error %d\n", > pwm->label, ret); > - return; > } > > static const struct pwm_ops ab8500_pwm_ops = { Quoting verbatim som my colleagues can see it. This patch makes a lot of sense after inspection, so Acked-by: Linus Walleij <linus.walleij@linaro.org> But allow Philippe & Alexandre (now added to To:) to have a look at it too. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, That is fine for me. Best regards and happy new year Alexandre BOURDIOL -----Original Message----- From: Linus Walleij [mailto:linus.walleij@linaro.org] Sent: vendredi 20 décembre 2013 18:10 To: Axel Lin; Philippe BEGNIC ST; Alexandre BOURDIOL Cc: Thierry Reding; linux-pwm@vger.kernel.org Subject: Re: [PATCH] pwm: ab8500: Fix wrong value shift for disable/enable pwm On Sun, Dec 15, 2013 at 4:32 AM, Axel Lin <axel.lin@ingics.com> wrote: > Current code only works when pdev->id is 1. Fix it by passing correct > bitvalues argument to abx500_mask_and_set_register_interruptible. > > Having defines for DISABLE_PWM/ENABLE_PWM does not make the code > better in readability because the bitvalues depends on pdev->id. > Thus drop defines for DISABLE_PWM/ENABLE_PWM. > > This patch also removes a unnecessary return in ab8500_pwm_disable. > > Signed-off-by: Axel Lin <axel.lin@ingics.com> > --- > drivers/pwm/pwm-ab8500.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/pwm/pwm-ab8500.c b/drivers/pwm/pwm-ab8500.c index > 1d07a6f..d51dc15 100644 > --- a/drivers/pwm/pwm-ab8500.c > +++ b/drivers/pwm/pwm-ab8500.c > @@ -20,10 +20,6 @@ > #define AB8500_PWM_OUT_CTRL2_REG 0x61 > #define AB8500_PWM_OUT_CTRL7_REG 0x66 > > -/* backlight driver constants */ > -#define ENABLE_PWM 1 > -#define DISABLE_PWM 0 > - > struct ab8500_pwm_chip { > struct pwm_chip chip; > }; > @@ -64,7 +60,7 @@ static int ab8500_pwm_enable(struct pwm_chip *chip, > struct pwm_device *pwm) > > ret = abx500_mask_and_set_register_interruptible(chip->dev, > AB8500_MISC, AB8500_PWM_OUT_CTRL7_REG, > - 1 << (chip->base - 1), ENABLE_PWM); > + 1 << (chip->base - 1), 1 << > + (chip->base - 1)); > if (ret < 0) > dev_err(chip->dev, "%s: Failed to enable PWM, Error %d\n", > pwm->label, > ret); @@ -77,11 +73,10 @@ static void ab8500_pwm_disable(struct > pwm_chip *chip, struct pwm_device *pwm) > > ret = abx500_mask_and_set_register_interruptible(chip->dev, > AB8500_MISC, AB8500_PWM_OUT_CTRL7_REG, > - 1 << (chip->base - 1), DISABLE_PWM); > + 1 << (chip->base - 1), 0); > if (ret < 0) > dev_err(chip->dev, "%s: Failed to disable PWM, Error %d\n", > pwm->label, ret); > - return; > } > > static const struct pwm_ops ab8500_pwm_ops = { Quoting verbatim som my colleagues can see it. This patch makes a lot of sense after inspection, so Acked-by: Linus Walleij <linus.walleij@linaro.org> But allow Philippe & Alexandre (now added to To:) to have a look at it too. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2013-12-17 17:30 GMT+08:00 Thierry Reding <thierry.reding@gmail.com>: > On Sun, Dec 15, 2013 at 11:32:19AM +0800, Axel Lin wrote: > [...] > > s/pwm/PWM/ in the subject, please. > > [...] >> @@ -64,7 +60,7 @@ static int ab8500_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) >> >> ret = abx500_mask_and_set_register_interruptible(chip->dev, >> AB8500_MISC, AB8500_PWM_OUT_CTRL7_REG, >> - 1 << (chip->base - 1), ENABLE_PWM); >> + 1 << (chip->base - 1), 1 << (chip->base - 1)); > > I think we really need to find some other way to do this. At some point > I imagine board will move to DT, at which point we can no longer rely on > chip->base being set to a meaningful value. Hi Thierry, (I almost forgot this thread...) I just check the code in drivers/mfd/ab8500-core.c. In ab8500_devs[]: { .name = "ab8500-pwm", .of_compatible = "stericsson,ab8500-pwm", .id = 1, }, { .name = "ab8500-pwm", .of_compatible = "stericsson,ab8500-pwm", .id = 2, }, { .name = "ab8500-pwm", .of_compatible = "stericsson,ab8500-pwm", .id = 3, }, And in ab8500_pwm_probe(): ab8500->chip.base = pdev->id; My understanding is the chip->base setting is the same for DT and non-DT case. (I'm not sure if you still have the patch, so I'm re-sending the patch now.) Regards, Axel -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pwm/pwm-ab8500.c b/drivers/pwm/pwm-ab8500.c index 1d07a6f..d51dc15 100644 --- a/drivers/pwm/pwm-ab8500.c +++ b/drivers/pwm/pwm-ab8500.c @@ -20,10 +20,6 @@ #define AB8500_PWM_OUT_CTRL2_REG 0x61 #define AB8500_PWM_OUT_CTRL7_REG 0x66 -/* backlight driver constants */ -#define ENABLE_PWM 1 -#define DISABLE_PWM 0 - struct ab8500_pwm_chip { struct pwm_chip chip; }; @@ -64,7 +60,7 @@ static int ab8500_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) ret = abx500_mask_and_set_register_interruptible(chip->dev, AB8500_MISC, AB8500_PWM_OUT_CTRL7_REG, - 1 << (chip->base - 1), ENABLE_PWM); + 1 << (chip->base - 1), 1 << (chip->base - 1)); if (ret < 0) dev_err(chip->dev, "%s: Failed to enable PWM, Error %d\n", pwm->label, ret); @@ -77,11 +73,10 @@ static void ab8500_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) ret = abx500_mask_and_set_register_interruptible(chip->dev, AB8500_MISC, AB8500_PWM_OUT_CTRL7_REG, - 1 << (chip->base - 1), DISABLE_PWM); + 1 << (chip->base - 1), 0); if (ret < 0) dev_err(chip->dev, "%s: Failed to disable PWM, Error %d\n", pwm->label, ret); - return; } static const struct pwm_ops ab8500_pwm_ops = {
Current code only works when pdev->id is 1. Fix it by passing correct bitvalues argument to abx500_mask_and_set_register_interruptible. Having defines for DISABLE_PWM/ENABLE_PWM does not make the code better in readability because the bitvalues depends on pdev->id. Thus drop defines for DISABLE_PWM/ENABLE_PWM. This patch also removes a unnecessary return in ab8500_pwm_disable. Signed-off-by: Axel Lin <axel.lin@ingics.com> --- drivers/pwm/pwm-ab8500.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)