[PATCH 3/7] misc: ixp4-beeper: use gpiolib strictly

[PATCH 3/7] misc: ixp4-beeper: use gpiolib strictly

[Linus Walleij]

Request and free the GPIO line used for the beeper properly.
Then use the gpiolib API to flip the output of the GPIO pin
instead of relying on hacks to poke the register bits.

Cc: Imre Kaloz <kaloz@openwrt.org>
Cc: Krzysztof Halasa <khc@pm.waw.pl>
Cc: Alexandre Courbot <acourbot@nvidia.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Arnd/Greg: seeking your ACK to take this through the GPIO tree
as part of the attempt at cleaning out custom GPIO implementations.
---
 drivers/input/misc/ixp4xx-beeper.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/ixp4xx-beeper.c b/drivers/input/misc/ixp4xx-beeper.c
index f14afd0..fb3bec5 100644
--- a/drivers/input/misc/ixp4xx-beeper.c
+++ b/drivers/input/misc/ixp4xx-beeper.c
@@ -76,11 +76,13 @@ static int ixp4xx_spkr_event(struct input_dev *dev, unsigned int type, unsigned
 
 static irqreturn_t ixp4xx_spkr_interrupt(int irq, void *dev_id)
 {
+	unsigned int pin = (unsigned int) dev_id;
+
 	/* clear interrupt */
 	*IXP4XX_OSST = IXP4XX_OSST_TIMER_2_PEND;
 
 	/* flip the beeper output */
-	*IXP4XX_GPIO_GPOUTR ^= (1 << (unsigned int) dev_id);
+	gpio_set_value(pin, ~gpio_get_value(pin));
 
 	return IRQ_HANDLED;
 }
@@ -108,11 +110,15 @@ static int ixp4xx_spkr_probe(struct platform_device *dev)
 	input_dev->sndbit[0] = BIT_MASK(SND_BELL) | BIT_MASK(SND_TONE);
 	input_dev->event = ixp4xx_spkr_event;
 
+	err = gpio_request(dev->id, "ixp4-beeper");
+	if (err)
+		goto err_free_device;
+
 	err = request_irq(IRQ_IXP4XX_TIMER2, &ixp4xx_spkr_interrupt,
 			  IRQF_NO_SUSPEND, "ixp4xx-beeper",
 			  (void *) dev->id);
 	if (err)
-		goto err_free_device;
+		goto err_free_gpio;
 
 	err = input_register_device(input_dev);
 	if (err)
@@ -124,6 +130,8 @@ static int ixp4xx_spkr_probe(struct platform_device *dev)
 
  err_free_irq:
 	free_irq(IRQ_IXP4XX_TIMER2, (void *)dev->id);
+ err_free_gpio:
+	gpio_free(dev->id);
  err_free_device:
 	input_free_device(input_dev);
 
@@ -142,6 +150,7 @@ static int ixp4xx_spkr_remove(struct platform_device *dev)
 	ixp4xx_spkr_control(pin, 0);
 
 	free_irq(IRQ_IXP4XX_TIMER2, (void *)dev->id);
+	gpio_free(dev->id);
 
 	return 0;
 }
-- 
1.8.3.1

Re: [PATCH 3/7] misc: ixp4-beeper: use gpiolib strictly

[Arnd Bergmann]

On Tuesday 10 September 2013, Linus Walleij wrote:
> Request and free the GPIO line used for the beeper properly.
> Then use the gpiolib API to flip the output of the GPIO pin
> instead of relying on hacks to poke the register bits.
> 
> Cc: Imre Kaloz <kaloz@openwrt.org>
> Cc: Krzysztof Halasa <khc@pm.waw.pl>
> Cc: Alexandre Courbot <acourbot@nvidia.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Arnd/Greg: seeking your ACK to take this through the GPIO tree
> as part of the attempt at cleaning out custom GPIO implementations.


Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 3/7] misc: ixp4-beeper: use gpiolib strictly

[Alex Courbot]

On 09/10/2013 09:30 PM, Linus Walleij wrote:
> Request and free the GPIO line used for the beeper properly.
> Then use the gpiolib API to flip the output of the GPIO pin
> instead of relying on hacks to poke the register bits.
>
> Cc: Imre Kaloz <kaloz@openwrt.org>
> Cc: Krzysztof Halasa <khc@pm.waw.pl>
> Cc: Alexandre Courbot <acourbot@nvidia.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Arnd/Greg: seeking your ACK to take this through the GPIO tree
> as part of the attempt at cleaning out custom GPIO implementations.
> ---
>   drivers/input/misc/ixp4xx-beeper.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/misc/ixp4xx-beeper.c b/drivers/input/misc/ixp4xx-beeper.c
> index f14afd0..fb3bec5 100644
> --- a/drivers/input/misc/ixp4xx-beeper.c
> +++ b/drivers/input/misc/ixp4xx-beeper.c
> @@ -76,11 +76,13 @@ static int ixp4xx_spkr_event(struct input_dev *dev, unsigned int type, unsigned
>
>   static irqreturn_t ixp4xx_spkr_interrupt(int irq, void *dev_id)
>   {
> +	unsigned int pin = (unsigned int) dev_id;
> +
>   	/* clear interrupt */
>   	*IXP4XX_OSST = IXP4XX_OSST_TIMER_2_PEND;
>
>   	/* flip the beeper output */
> -	*IXP4XX_GPIO_GPOUTR ^= (1 << (unsigned int) dev_id);
> +	gpio_set_value(pin, ~gpio_get_value(pin));

Don't you want

	gpio_set_value(pin, !gpio_get_value(pin));

instead? From the deleted line I suppose you guess to invert the GPIO 
value, but using ~ will invert all the bits of the integer returned by 
gpio_get_value(), meaning that if it returns 1 you will end up calling 
gpio_set_value(-2). In practice your gpio is very unlikely to ever be 
set to 0.

>
>   	return IRQ_HANDLED;
>   }
> @@ -108,11 +110,15 @@ static int ixp4xx_spkr_probe(struct platform_device *dev)
>   	input_dev->sndbit[0] = BIT_MASK(SND_BELL) | BIT_MASK(SND_TONE);
>   	input_dev->event = ixp4xx_spkr_event;
>
> +	err = gpio_request(dev->id, "ixp4-beeper");
> +	if (err)
> +		goto err_free_device;
> +
>   	err = request_irq(IRQ_IXP4XX_TIMER2, &ixp4xx_spkr_interrupt,
>   			  IRQF_NO_SUSPEND, "ixp4xx-beeper",
>   			  (void *) dev->id);
>   	if (err)
> -		goto err_free_device;
> +		goto err_free_gpio;
>
>   	err = input_register_device(input_dev);
>   	if (err)
> @@ -124,6 +130,8 @@ static int ixp4xx_spkr_probe(struct platform_device *dev)
>
>    err_free_irq:
>   	free_irq(IRQ_IXP4XX_TIMER2, (void *)dev->id);
> + err_free_gpio:
> +	gpio_free(dev->id);
>    err_free_device:
>   	input_free_device(input_dev);
>
> @@ -142,6 +150,7 @@ static int ixp4xx_spkr_remove(struct platform_device *dev)
>   	ixp4xx_spkr_control(pin, 0);
>
>   	free_irq(IRQ_IXP4XX_TIMER2, (void *)dev->id);
> +	gpio_free(dev->id);

Just wondering, is there a reason for not using the devm_gpio functions 
here?

>
>   	return 0;
>   }
>

Re: [PATCH 3/7] misc: ixp4-beeper: use gpiolib strictly

[Linus Walleij]

On Wed, Sep 11, 2013 at 3:58 AM, Alex Courbot <acourbot@nvidia.com> wrote:
> On 09/10/2013 09:30 PM, Linus Walleij wrote:
>>         /* flip the beeper output */
>> -       *IXP4XX_GPIO_GPOUTR ^= (1 << (unsigned int) dev_id);
>> +       gpio_set_value(pin, ~gpio_get_value(pin));
>
>
> Don't you want
>
>         gpio_set_value(pin, !gpio_get_value(pin));
>
> instead? From the deleted line I suppose you guess to invert the GPIO value,
> but using ~ will invert all the bits of the integer returned by
> gpio_get_value(), meaning that if it returns 1 you will end up calling
> gpio_set_value(-2). In practice your gpio is very unlikely to ever be set to
> 0.

You're right, fixed this.

>>         free_irq(IRQ_IXP4XX_TIMER2, (void *)dev->id);
>> +       gpio_free(dev->id);
>
>
> Just wondering, is there a reason for not using the devm_gpio functions
> here?

I didn't want to change the style of the whole driver, but I can make
a separate patch switching the whole driver to use managed
resources at some later point.

Yours,
Linus Walleij

Re: [PATCH 3/7] misc: ixp4-beeper: use gpiolib strictly

[Greg Kroah-Hartman]

On Tue, Sep 10, 2013 at 02:30:26PM +0200, Linus Walleij wrote:
> Request and free the GPIO line used for the beeper properly.
> Then use the gpiolib API to flip the output of the GPIO pin
> instead of relying on hacks to poke the register bits.
> 
> Cc: Imre Kaloz <kaloz@openwrt.org>
> Cc: Krzysztof Halasa <khc@pm.waw.pl>
> Cc: Alexandre Courbot <acourbot@nvidia.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Arnd/Greg: seeking your ACK to take this through the GPIO tree
> as part of the attempt at cleaning out custom GPIO implementations.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>