[PATCH] gpiolib: wm8994: Pay attention to the value set when enabling as output

[PATCH] gpiolib: wm8994: Pay attention to the value set when enabling as output

[Mark Brown]

Not paying attention to the value being set is a bad thing because it
means that we'll not set the hardware up to reflect what was requested.
Not setting the hardware up to reflect what was requested means that the
caller won't get the results they wanted.
`
Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: stable@vger.kernel.org
---
 drivers/gpio/gpio-wm8994.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-wm8994.c b/drivers/gpio/gpio-wm8994.c
index f2b3d19..1c764e7 100644
--- a/drivers/gpio/gpio-wm8994.c
+++ b/drivers/gpio/gpio-wm8994.c
@@ -90,8 +90,11 @@ static int wm8994_gpio_direction_out(struct gpio_chip *chip,
 	struct wm8994_gpio *wm8994_gpio = to_wm8994_gpio(chip);
 	struct wm8994 *wm8994 = wm8994_gpio->wm8994;
 
+	if (value)
+		value = WM8994_GPN_LVL;
+
 	return wm8994_set_bits(wm8994, WM8994_GPIO_1 + offset,
-			       WM8994_GPN_DIR, 0);
+			       WM8994_GPN_DIR | WM8994_GPN_LVL, value);
 }
 
 static void wm8994_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
-- 
1.7.10

Re: [PATCH] gpiolib: wm8994: Pay attention to the value set when enabling as output

[Linus Walleij]

On Sat, Jun 9, 2012 at 5:07 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:

> Not paying attention to the value being set is a bad thing because it
> means that we'll not set the hardware up to reflect what was requested.
> Not setting the hardware up to reflect what was requested means that the
> caller won't get the results they wanted.
> `
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: stable@vger.kernel.org

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH] gpiolib: wm8994: Pay attention to the value set when enabling as output

[Linus Walleij]

On Sat, Jun 9, 2012 at 5:07 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:

> Not paying attention to the value being set is a bad thing because it
> means that we'll not set the hardware up to reflect what was requested.
> Not setting the hardware up to reflect what was requested means that the
> caller won't get the results they wanted.
> `

I removed this "`"

> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: stable@vger.kernel.org

Applied.

Yours,
Linus Walleij

[PATCH] gpiolib: wm8994: Pay attention to the value set when enabling as output

[Mark Brown]

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: stable@vger.kernel.org
---
 drivers/gpio/gpio-wm8994.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-wm8994.c b/drivers/gpio/gpio-wm8994.c
index f2b3d19..1c764e7 100644
--- a/drivers/gpio/gpio-wm8994.c
+++ b/drivers/gpio/gpio-wm8994.c
@@ -90,8 +90,11 @@ static int wm8994_gpio_direction_out(struct gpio_chip *chip,
 	struct wm8994_gpio *wm8994_gpio = to_wm8994_gpio(chip);
 	struct wm8994 *wm8994 = wm8994_gpio->wm8994;
 
+	if (value)
+		value = WM8994_GPN_LVL;
+
 	return wm8994_set_bits(wm8994, WM8994_GPIO_1 + offset,
-			       WM8994_GPN_DIR, 0);
+			       WM8994_GPN_DIR | WM8994_GPN_LVL, value);
 }
 
 static void wm8994_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
-- 
1.7.10

Re: [PATCH] gpiolib: wm8994: Pay attention to the value set when enabling as output

[Linus Walleij]

On Tue, Jun 5, 2012 at 7:12 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:

> + � � � if (value)
> + � � � � � � � value = WM8994_GPN_LVL;
> +
> � � � �return wm8994_set_bits(wm8994, WM8994_GPIO_1 + offset,
> - � � � � � � � � � � � � � � �WM8994_GPN_DIR, 0);
> + � � � � � � � � � � � � � � �WM8994_GPN_DIR | WM8994_GPN_LVL, value);
> �}

The commit message is so terse compared to what the code does that
I cannot claim to understand what this patch is doing.

Could you elaborate on the problem ans symptoms, just for a nice
commit log?

Yours,
Linus Walleij

Re: [PATCH] gpiolib: wm8994: Pay attention to the value set when enabling as output

[Mark Brown]

On Thu, Jun 07, 2012 at 01:11:20PM +0200, Linus Walleij wrote:
> <broonie@opensource.wolfsonmicro.com> wrote:

> > + ? ? ? if (value)
> > + ? ? ? ? ? ? ? value = WM8994_GPN_LVL;

> > ? ? ? ?return wm8994_set_bits(wm8994, WM8994_GPIO_1 + offset,
> > - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?WM8994_GPN_DIR, 0);
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?WM8994_GPN_DIR | WM8994_GPN_LVL, value);
> > ?}

> The commit message is so terse compared to what the code does that
> I cannot claim to understand what this patch is doing.

> Could you elaborate on the problem ans symptoms, just for a nice
> commit log?

I'm not really sure what more there is to say...  previously we were
ignoring the value that's set as part of setting output mode so if
someone wanted to set a value different to the one the chip happened to
have that wouldn't happen.  Ignoring the vaue that the user is trying to
set seems like an obvious enough bug in itself.

Re: [PATCH] gpiolib: wm8994: Pay attention to the value set when enabling as output

[Linus Walleij]

On Thu, Jun 7, 2012 at 11:53 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:

>> Could you elaborate on the problem ans symptoms, just for a nice
>> commit log?
>
> I'm not really sure what more there is to say...

But you do ;-)

> previously we were
> ignoring the value that's set as part of setting output mode so if
> someone wanted to set a value different to the one the chip happened to
> have that wouldn't happen. �Ignoring the vaue that the user is trying to
> set seems like an obvious enough bug in itself.

Throw that into the commit message and I'm in for it!
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij