pwm: ab8500: Fix wrong value shift for disable/enable pwm
PATCH 1/1 pwm: ab8500: Fix wrong value shift for disable/enable pwm
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.
--- drivers/pwm/pwm-ab8500.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
drivers/pwm/pwm-ab8500.c +2 -7
@@ -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 = {
Signature · 1 line
1.8.1.2
2013/12/15 Axel Lin <[email protected]>:
Quoted 8 lines
Current code only works when
pdev->idis 1. Fix it by passing correct bitvalues argument toabx500_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
On Sun, Dec 15, 2013 at 11:32:19AM +0800, Axel Lin wrote:
[...]
s/pwm/PWM/ in the subject, please.
[...]
drivers/pwm/pwm-ab8500.c static int ab8500_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) · line 63
Quoted 6 lines
drivers/pwm/pwm-ab8500.c +1
-1@@ -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
Quoted 41 lines · Axel Lin
Current code only works when
pdev->idis 1. Fix it by passing correct bitvalues argument toabx500_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 <[email protected]>--- drivers/pwm/pwm-ab8500.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)drivers/pwm/pwm-ab8500.c +1
-5@@ -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,
drivers/pwm/pwm-ab8500.c static void ab8500_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) · line 82
Quoted 10 lines
drivers/pwm/pwm-ab8500.c +1
-2AB8500_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
But allow Philippe & Alexandre (now added to To:)
to have a look at it too.
Yours,
Linus Walleij
Alexandre BOURDIOL
Acked-by Linus WalleijHi,
That is fine for me.
Best regards and happy new year
Alexandre BOURDIOL
-----Original Message-----
From: Linus Walleij [mailto:[email protected]]
Sent: vendredi 20 décembre 2013 18:10
To: Axel Lin; Philippe BEGNIC ST; Alexandre BOURDIOL
Cc: Thierry Reding; [email protected]
Subject: Re: [PATCH] pwm: ab8500: Fix wrong value shift for disable/enable pwm
Quoted 45 lines · Axel Lin
Current code only works when
pdev->idis 1. Fix it by passing correct bitvalues argument toabx500_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 <[email protected]>--- drivers/pwm/pwm-ab8500.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)drivers/pwm/pwm-ab8500.c index renamed from drivers/pwm/pwm-ab8500.c +0
-01d07a6f..d51dc15 100644
drivers/pwm/pwm-ab8500.c +2
-5@@ -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,
drivers/pwm/pwm-ab8500.c static void ab8500_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) · line 82
Quoted 9 lines
drivers/pwm/pwm-ab8500.c +1
-2- 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
But allow Philippe & Alexandre (now added to To:) to have a look at it too.
Yours,
Linus Walleij
2013-12-17 17:30 GMT+08:00 Thierry Reding <[email protected]>:
Quoted 8 lines · Axel Lin
[...]
s/pwm/PWM/ in the subject, please.
[...]
Quoted 2 lines
(quoted diff) +0
-0@@ -64,7 +60,7 @@ static int ab8500_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
Quoted 8 lines
drivers/pwm/pwm-ab8500.c static int ab8500_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) · line 63
Quoted 4 lines
drivers/pwm/pwm-ab8500.c +1
-1ret =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->basebeing 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