* [PATCH] pwm: ab8500: Fix wrong value shift for disable/enable pwm
@ 2013-12-15 3:32 Axel Lin
2013-12-16 8:25 ` Axel Lin
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Axel Lin @ 2013-12-15 3:32 UTC (permalink / raw)
To: Thierry Reding; +Cc: Linus Walleij, Arun Murthy, linux-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.
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 = {
--
1.8.1.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] pwm: ab8500: Fix wrong value shift for disable/enable pwm
2013-12-15 3:32 [PATCH] pwm: ab8500: Fix wrong value shift for disable/enable pwm Axel Lin
@ 2013-12-16 8:25 ` Axel Lin
2013-12-17 9:30 ` Thierry Reding
2013-12-20 17:10 ` Linus Walleij
2 siblings, 0 replies; 6+ messages in thread
From: Axel Lin @ 2013-12-16 8:25 UTC (permalink / raw)
To: Thierry Reding; +Cc: Linus Walleij, linux-pwm
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] pwm: ab8500: Fix wrong value shift for disable/enable pwm
2013-12-15 3:32 [PATCH] pwm: ab8500: Fix wrong value shift for disable/enable pwm Axel Lin
2013-12-16 8:25 ` Axel Lin
@ 2013-12-17 9:30 ` Thierry Reding
2014-04-09 13:18 ` Axel Lin
2013-12-20 17:10 ` Linus Walleij
2 siblings, 1 reply; 6+ messages in thread
From: Thierry Reding @ 2013-12-17 9:30 UTC (permalink / raw)
To: Axel Lin; +Cc: Thierry Reding, Linus Walleij, Arun Murthy, linux-pwm
[-- Attachment #1: Type: text/plain, Size: 631 bytes --]
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
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] pwm: ab8500: Fix wrong value shift for disable/enable pwm
2013-12-17 9:30 ` Thierry Reding
@ 2014-04-09 13:18 ` Axel Lin
0 siblings, 0 replies; 6+ messages in thread
From: Axel Lin @ 2014-04-09 13:18 UTC (permalink / raw)
To: Thierry Reding; +Cc: Thierry Reding, Linus Walleij, linux-pwm
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] pwm: ab8500: Fix wrong value shift for disable/enable pwm
2013-12-15 3:32 [PATCH] pwm: ab8500: Fix wrong value shift for disable/enable pwm Axel Lin
2013-12-16 8:25 ` Axel Lin
2013-12-17 9:30 ` Thierry Reding
@ 2013-12-20 17:10 ` Linus Walleij
2014-01-06 8:55 ` Alexandre BOURDIOL
2 siblings, 1 reply; 6+ messages in thread
From: Linus Walleij @ 2013-12-20 17:10 UTC (permalink / raw)
To: Axel Lin, Philippe Begnic, alexandre.bourdiol
Cc: Thierry Reding, linux-pwm@vger.kernel.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH] pwm: ab8500: Fix wrong value shift for disable/enable pwm
2013-12-20 17:10 ` Linus Walleij
@ 2014-01-06 8:55 ` Alexandre BOURDIOL
0 siblings, 0 replies; 6+ messages in thread
From: Alexandre BOURDIOL @ 2014-01-06 8:55 UTC (permalink / raw)
To: Linus Walleij, Axel Lin, Philippe BEGNIC ST
Cc: Thierry Reding, linux-pwm@vger.kernel.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-04-09 13:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-15 3:32 [PATCH] pwm: ab8500: Fix wrong value shift for disable/enable pwm Axel Lin
2013-12-16 8:25 ` Axel Lin
2013-12-17 9:30 ` Thierry Reding
2014-04-09 13:18 ` Axel Lin
2013-12-20 17:10 ` Linus Walleij
2014-01-06 8:55 ` Alexandre BOURDIOL
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).