[PATCH,RFC] Input: gpio-keys - request and configure GPIOs

[PATCH,RFC] Input: gpio-keys - request and configure GPIOs

[Herbert Valerio Riedel]

Currently, gpio_keys.c assumes the GPIOs to be already properly configured;
this patch changes gpio-keys to perform explicit calls to gpio_request() and
gpio_configure_input().

This matches the behaviour of leds-gpio.

Cc: Paul Sokolovsky <pmiscml@gmail.com>
Cc: Philipp Zabel <philipp.zabel@gmail.com>
Cc: Dmitry Torokhov <dtor@mail.ru>
Cc: Philip Blundell <philb@gnu.org>
---
btw, I'm unsure whether the '?:' operator is allowed in kernel source;
but since I found quite a bit of occurences throughout the kernel source
(including gpio_keys.c), I assume it's tolerated... :-)

 drivers/input/keyboard/gpio_keys.c |   26 ++++++++++++++++++++++++--
 1 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 3eddf52..445265c 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -75,9 +75,26 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 
 	for (i = 0; i < pdata->nbuttons; i++) {
 		struct gpio_keys_button *button = &pdata->buttons[i];
-		int irq = gpio_to_irq(button->gpio);
+		int irq;
 		unsigned int type = button->type ?: EV_KEY;
 
+		error = gpio_request(button->gpio, button->desc ?: "gpio_keys");
+		if (error < 0) {
+			pr_err("gpio-keys: failed to request GPIO %d,"
+				" error %d\n", button->gpio, error);
+			goto fail;
+		}
+
+		error = gpio_direction_input(button->gpio);
+		if (error < 0) {
+			pr_err("gpio-keys: failed to configure input"
+				" direction for GPIO %d, error %d\n",
+				button->gpio, error);
+			gpio_free(button->gpio);
+			goto fail;
+		}
+
+		irq = gpio_to_irq(button->gpio);
 		if (irq < 0) {
 			error = irq;
 			printk(KERN_ERR
@@ -85,6 +102,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 				"Unable to get irq number for GPIO %d,"
 				"error %d\n",
 				button->gpio, error);
+			gpio_free(button->gpio);
 			goto fail;
 		}
 
@@ -97,6 +115,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 			printk(KERN_ERR
 				"gpio-keys: Unable to claim irq %d; error %d\n",
 				irq, error);
+			gpio_free(button->gpio);
 			goto fail;
 		}
 
@@ -119,8 +138,10 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 	return 0;
 
  fail:
-	while (--i >= 0)
+	while (--i >= 0) {
 		free_irq(gpio_to_irq(pdata->buttons[i].gpio), pdev);
+		gpio_free(pdata->buttons[i].gpio);
+	}
 
 	platform_set_drvdata(pdev, NULL);
 	input_free_device(input);
@@ -139,6 +160,7 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev)
 	for (i = 0; i < pdata->nbuttons; i++) {
 		int irq = gpio_to_irq(pdata->buttons[i].gpio);
 		free_irq(irq, pdev);
+		gpio_free(pdata->buttons[i].gpio);
 	}
 
 	input_unregister_device(input);
-- 
1.5.3.4

Re: [PATCH,RFC] Input: gpio-keys - request and configure GPIOs

[Herbert Valerio Riedel]

On Fri, 16 Nov 2007 12:33:50 +0100, Herbert Valerio Riedel wrote:
> Currently, gpio_keys.c assumes the GPIOs to be already properly
> configured; this patch changes gpio-keys to perform explicit calls to
> gpio_request() and gpio_configure_input().
> 
> This matches the behaviour of leds-gpio.

Signed-off-by: Herbert Valerio Riedel <hvr@gnu.org>

D'oh! :-)

Re: [PATCH,RFC] Input: gpio-keys - request and configure GPIOs

[Dmitry Torokhov]

Hi,

On Nov 16, 2007 6:33 AM, Herbert Valerio Riedel <hvr@gnu.org> wrote:
> Currently, gpio_keys.c assumes the GPIOs to be already properly configured;
> this patch changes gpio-keys to perform explicit calls to gpio_request() and
> gpio_configure_input().
>
> This matches the behaviour of leds-gpio.
>

Makes sense from where I sit but let's see what guys who actually use
the module say... ;)

-- 
Dmitry

Re: [PATCH,RFC] Input: gpio-keys - request and configure GPIOs

[pHilipp Zabel]

On Nov 16, 2007 2:38 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> Hi,
>
> On Nov 16, 2007 6:33 AM, Herbert Valerio Riedel <hvr@gnu.org> wrote:
> > Currently, gpio_keys.c assumes the GPIOs to be already properly configured;
> > this patch changes gpio-keys to perform explicit calls to gpio_request() and
> > gpio_configure_input().
> >
> > This matches the behaviour of leds-gpio.
> >
>
> Makes sense from where I sit but let's see what guys who actually use
> the module say... ;)

Looks good to me, too. I have yet to test, but at least the gpio_direction call
is mandatory as per gpio api docs, so this fixes an actual bug.
gpio_request is optional and claims the GPIO for this driver's use
only (on architectures where
this is supported), so I'm not sure if this is really needed, but it
shouldn't harm any of the current users of gpio-keys.

regards
Philipp

Re: [PATCH,RFC] Input: gpio-keys - request and configure GPIOs

[Herbert Valerio Riedel]

On Fri, 2007-11-16 at 15:02 +0100, pHilipp Zabel wrote:
> On Nov 16, 2007 2:38 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > On Nov 16, 2007 6:33 AM, Herbert Valerio Riedel <hvr@gnu.org> wrote:
> > > Currently, gpio_keys.c assumes the GPIOs to be already properly configured;
> > > this patch changes gpio-keys to perform explicit calls to gpio_request() and
> > > gpio_configure_input().
> > >
> > > This matches the behaviour of leds-gpio.

> > Makes sense from where I sit but let's see what guys who actually use
> > the module say... ;)

> Looks good to me, too. I have yet to test, but at least the gpio_direction call
> is mandatory as per gpio api docs, so this fixes an actual bug.

> gpio_request is optional and claims the GPIO for this driver's use
> only (on architectures where this is supported), so I'm not sure if this is really needed, but it
> shouldn't harm any of the current users of gpio-keys.

yes, optional and no-harm, to quote the gpio API doc:

> These two calls are optional because not not all current Linux platforms
> offer such functionality in their GPIO support; a valid implementation
> could return success for all gpio_request() calls.  Unlike the other calls,
> the state they represent doesn't normally match anything from a hardware
> register; it's just a software bitmap which clearly is not necessary for
> correct operation of hardware or (bug free) drivers.

...but for instance, the current mach-orion/gpio.c implementation,
prints annoying printk warnings when someone fiddles with "unrequested"
GPIOs... :-)

cheers,
hvr

Re: [PATCH,RFC] Input: gpio-keys - request and configure GPIOs

[Herbert Valerio Riedel]

On Fri, 2007-11-16 at 08:38 -0500, Dmitry Torokhov wrote:
> On Nov 16, 2007 6:33 AM, Herbert Valerio Riedel <hvr@gnu.org> wrote:
> > Currently, gpio_keys.c assumes the GPIOs to be already properly configured;
> > this patch changes gpio-keys to perform explicit calls to gpio_request() and
> > gpio_configure_input().
> >
> > This matches the behaviour of leds-gpio.

> Makes sense from where I sit but let's see what guys who actually use
> the module say... ;)

I grepped through my 2.6.24-git source tre, and the only places I found
gpio_keys.h included was

drivers/input/keyboard/gpio_keys.c:#include <linux/gpio_keys.h>
arch/arm/mach-orion/dns323-setup.c:#include <linux/gpio_keys.h>
arch/arm/mach-at91/board-sam9261ek.c:#include <linux/gpio_keys.h>

with dn323-setup.c being maintained by myself, and board-sam9261ek.c
having seemingly non-compilable code wrt to gpio_keys (for instance, it
uses ".keycode" gpio_keys_button  struct members which should have been
".code"), there don't seem to exist many users of that module...? :-)

cheers,
hvr

Re: [PATCH,RFC] Input: gpio-keys - request and configure GPIOs

[pHilipp Zabel]

On Nov 16, 2007 3:09 PM, Herbert Valerio Riedel <hvr@gnu.org> wrote:
>
>
> On Fri, 2007-11-16 at 08:38 -0500, Dmitry Torokhov wrote:
> > On Nov 16, 2007 6:33 AM, Herbert Valerio Riedel <hvr@gnu.org> wrote:
> > > Currently, gpio_keys.c assumes the GPIOs to be already properly configured;
> > > this patch changes gpio-keys to perform explicit calls to gpio_request() and
> > > gpio_configure_input().
> > >
> > > This matches the behaviour of leds-gpio.
>
> > Makes sense from where I sit but let's see what guys who actually use
> > the module say... ;)
>
> I grepped through my 2.6.24-git source tre, and the only places I found
> gpio_keys.h included was
>
> drivers/input/keyboard/gpio_keys.c:#include <linux/gpio_keys.h>
> arch/arm/mach-orion/dns323-setup.c:#include <linux/gpio_keys.h>
> arch/arm/mach-at91/board-sam9261ek.c:#include <linux/gpio_keys.h>
>
> with dn323-setup.c being maintained by myself, and board-sam9261ek.c
> having seemingly non-compilable code wrt to gpio_keys (for instance, it
> uses ".keycode" gpio_keys_button  struct members which should have been
> ".code"), there don't seem to exist many users of that module...? :-)

We have quite a big number of users in the handhelds.org CVS kernel tree.
Unfortunately we are traditionally slow at pushing things to mainline, and many
of the devices depend on something like gpiolib hitting mainline for their GPIO
extenders.

regards
Philipp

Re: [PATCH,RFC] Input: gpio-keys - request and configure GPIOs

[Herbert Valerio Riedel]

On Fri, 2007-11-16 at 15:19 +0100, pHilipp Zabel wrote:
> > > On Nov 16, 2007 6:33 AM, Herbert Valerio Riedel <hvr@gnu.org> wrote:
> > > > Currently, gpio_keys.c assumes the GPIOs to be already properly configured;
> > > > this patch changes gpio-keys to perform explicit calls to gpio_request() and
> > > > gpio_configure_input().
> > > >
> > > > This matches the behaviour of leds-gpio.

> > > Makes sense from where I sit but let's see what guys who actually use
> > > the module say... ;)

[..]

> We have quite a big number of users in the handhelds.org CVS kernel tree.

> Unfortunately we are traditionally slow at pushing things to mainline, and many
> of the devices depend on something like gpiolib hitting mainline for their GPIO
> extenders.

well, is there any chance this patch of mine might make it for 2.6.25?
(or maybe even 2.6.24, since it can be regarded as being an actual
fix...?) :-)

regards,
hvr

Re: [PATCH,RFC] Input: gpio-keys - request and configure GPIOs

[Dmitry Torokhov]

On Nov 21, 2007 12:13 PM, Herbert Valerio Riedel <hvr@gnu.org> wrote:
>
> On Fri, 2007-11-16 at 15:19 +0100, pHilipp Zabel wrote:
> > > > On Nov 16, 2007 6:33 AM, Herbert Valerio Riedel <hvr@gnu.org> wrote:
> > > > > Currently, gpio_keys.c assumes the GPIOs to be already properly configured;
> > > > > this patch changes gpio-keys to perform explicit calls to gpio_request() and
> > > > > gpio_configure_input().
> > > > >
> > > > > This matches the behaviour of leds-gpio.
>
> > > > Makes sense from where I sit but let's see what guys who actually use
> > > > the module say... ;)
>
> [..]
>
> > We have quite a big number of users in the handhelds.org CVS kernel tree.
>
> > Unfortunately we are traditionally slow at pushing things to mainline, and many
> > of the devices depend on something like gpiolib hitting mainline for their GPIO
> > extenders.
>
> well, is there any chance this patch of mine might make it for 2.6.25?
> (or maybe even 2.6.24, since it can be regarded as being an actual
> fix...?) :-)
>

There is a slight one ;)

Applied to the "for-linus" branch of my tree. Thank you.

-- 
Dmitry