diff mbox

pwm: ab8500: Fix wrong value shift for disable/enable pwm

Message ID 1387078339.22351.2.camel@phoenix
State Changes Requested
Headers show

Commit Message

Axel Lin Dec. 15, 2013, 3:32 a.m. UTC
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(-)

Comments

Axel Lin Dec. 16, 2013, 8:25 a.m. UTC | #1
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
Thierry Reding Dec. 17, 2013, 9:30 a.m. UTC | #2
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
Linus Walleij Dec. 20, 2013, 5:10 p.m. UTC | #3
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
Alexandre BOURDIOL Jan. 6, 2014, 8:55 a.m. UTC | #4
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
Axel Lin April 9, 2014, 1:18 p.m. UTC | #5
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 mbox

Patch

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 = {