Input: gpio-keys - request and configure GPIOs

RFC PATCH Applied Herbert Valerio Riedel 3 participants 9 messages Nov 16, 2007 – Nov 21, 2007

Subsystems (2)

HID Input drivers

lore

Share & tools

lore

Herbert Valerio Riedel

author

2007-11-16 11:46 UTC #0 lore raw

RFC PATCH 1/1 Input: gpio-keys - request and configure GPIOs

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 <[email protected]>

Cc Philipp Zabel <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Cc Philip Blundell <[email protected]>

---
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(-)

drivers/input/keyboard/gpio_keys.c +24 -2

index 3eddf52..445265c 100644
@@ -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);

Signature · 1 line

1.5.3.4

Herbert Valerio Riedel

author

2007-11-16 11:55 UTC #1 lore raw

Quoted 5 lines · 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.

Signed-off-by Herbert Valerio Riedel <[email protected]>

D'oh! :-)

Dmitry Torokhov

maintainer

2007-11-16 13:38 UTC #2 lore raw

In reply to Herbert Valerio Riedel

Hi,

Quoted 6 lines · 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.

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

Signature · 1 line

Dmitry

pHilipp Zabel

2007-11-16 14:02 UTC #3 lore raw

Quoted 7 lines · Dmitry Torokhov

Hi,
Quoted 4 lines · 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().

Quoted 5 lines

Quoted 2 lines
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

Herbert Valerio Riedel

author

2007-11-16 14:15 UTC #4 lore raw

Quoted 11 lines · pHilipp Zabel

Quoted 9 lines · Dmitry Torokhov
Quoted 5 lines · 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.
Makes sense from where I sit but let's see what guys who actually use the module say... ;)

Quoted 6 lines

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:

Quoted 6 lines

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

Herbert Valerio Riedel

author

2007-11-16 15:30 UTC #5 lore raw

In reply to Dmitry Torokhov

Quoted 9 lines · Dmitry Torokhov

Quoted 5 lines · 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.
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

pHilipp Zabel

2007-11-16 14:20 UTC #6 lore raw

Quoted 16 lines · Herbert Valerio Riedel

Quoted 9 lines · Dmitry Torokhov
Quoted 5 lines · 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.
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

Quoted 8 lines

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

Herbert Valerio Riedel

author

2007-11-21 18:40 UTC #7 lore raw

Quoted 9 lines · pHilipp Zabel

Quoted 9 lines
Quoted 9 lines · Herbert Valerio Riedel
Quoted 5 lines
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... ;)

[..]

Quoted 5 lines

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

Dmitry Torokhov

maintainer Applied

2007-11-21 19:44 UTC #8 lore raw

Quoted 16 lines · Herbert Valerio Riedel

Quoted 9 lines · pHilipp Zabel
Quoted 9 lines
Quoted 9 lines · Herbert Valerio Riedel
Quoted 5 lines
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... ;)
[..]
Quoted 1 line
We have quite a big number of users in the handhelds.org CVS kernel tree.

Quoted 8 lines

Quoted 3 lines
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.

Signature · 1 line

Dmitry