Skip to contentSkip to conversation

gpiolib: allow poll() on value

PATCH In review David Brownell 2 participants 3 messages Aug 28, 2009 – Sep 2, 2009

Subsystems (2)
Documentation GPIO
lore
Share & tools
lore

David Brownell

author
#0 lore raw

PATCH 1/1 gpiolib: allow poll() on value

From: Daniel Glöckner <[email protected]>
Subject: [PATCH v3] gpiolib: allow poll(2) on gpio value

Many gpio chips allow to generate interrupts when the value of a pin changes. This patch gives usermode application the opportunity to make use of this feature by calling poll(2) on the /sys/class/gpio/gpioN/value sysfs file. The edge to trigger can be set in the edge file in the same directory. Possible values are "none", "rising", "falling", and "both".

Using level triggers is not possible with current sysfs since nothing changes the GPIO value (and the IRQ keeps triggering). Edge triggering will "just work". Note that if there was an event between read() and poll(), the poll() returns immediately.

Also note that this version only supports true GPIO interrupts. Some later patch might be able to synthesize this behavior by timer-driven polling; some systems seem to need that.

[[email protected]: align ids to 16 bit ids; whitespace]

Signed-off-by Daniel Glöckner <[email protected]>
Signed-off-by David Brownell <[email protected]>
---
 Documentation/ABI/testing/sysfs-gpio |    1 
 Documentation/gpio.txt               |    7 +
 drivers/gpio/gpiolib.c               |  208 ++++++++++++++++++++++++++++++++-
 3 files changed, 211 insertions(+), 5 deletions(-)
Documentation/ABI/testing/sysfs-gpio +1 -0
@@ -19,6 +19,7 @@ Description:
 	/gpioN ... for each exported GPIO #N
 	    /value ... always readable, writes fail for input GPIOs
 	    /direction ... r/w as: in, out (default low); write: high, low
+	    /edge ... r/w as: none, falling, rising, both
 	/gpiochipN ... for each gpiochip; #N is its first GPIO
 	    /base ... (r/o) same as N
 	    /label ... (r/o) descriptive, not necessarily unique
Documentation/gpio.txt +7 -0
@@ -524,6 +524,13 @@ and have the following read/write attrib
 		is configured as an output, this value may be written;
 		any nonzero value is treated as high.
 
+	"edge" ... reads as either "none", "rising", "falling", or
+		"both". Write these strings to select the signal edge(s)
+		that will make poll(2) on the "value" file return.
+
+		This file exists only if the pin can be configured as an
+		interrupt generating input pin.
+
 GPIO controllers have paths like /sys/class/gpio/chipchip42/ (for the
 controller implementing GPIOs starting at #42) and have the following
 read-only attributes:
drivers/gpio/gpiolib.c +203 -5

267 lines not on this page · raw message

#1 lore raw
Quoted 14 lines · On Thu, 27 Aug 2009 16:50:57 -0700 David Brownell <[email protected]> wrote:

From: Daniel Gl__ckner <[email protected]>
Subject: [PATCH v3] gpiolib: allow poll(2) on gpio value

Many gpio chips allow to generate interrupts when the value of a pin changes. This patch gives usermode application the opportunity to make use of this feature by calling poll(2) on the /sys/class/gpio/gpioN/value sysfs file. The edge to trigger can be set in the edge file in the same directory. Possible values are "none", "rising", "falling", and "both".

Using level triggers is not possible with current sysfs since nothing changes the GPIO value (and the IRQ keeps triggering). Edge triggering will "just work". Note that if there was an event between read() and poll(), the poll() returns immediately.

Quoted 7 lines

Also note that this version only supports true GPIO interrupts. Some later patch might be able to synthesize this behavior by timer-driven polling; some systems seem to need that.

...

+static struct idr pdesc_idr;

There's no locking to protect this tree. If that isn't a bug then I'd suggest that a comment be added here explaining why.

drivers/gpio/gpiolib.c static ssize_t gpio_value_store(struct d · line 315

Quoted 7 lines
drivers/gpio/gpiolib.c +7 -0
+static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
+{
+	struct work_struct	*work = priv;
+
+	schedule_work(work);
+	return IRQ_HANDLED;
+}

The only place where we explicitly cancel the pending work is in gpio_setup_irq(). Is that sufficient? Is there any way in which the work callback can occur after things have been freed/closed/deinitialised/etc?

author
#2 lore raw
Quoted 6 lines · Andrew Morton
Quoted 3 lines

...

+static struct idr pdesc_idr;

There's no locking to protect this tree.  If that isn't a bug then I'd suggest that a comment be added here explaining why.

Covered by mutex_lock(&sysfs_lock) calls per a quick re-read...

Quoted 4 lines
Quoted 4 lines
drivers/gpio/gpiolib.c +4 -0
+static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
+{
+     struct work_struct      *work = priv;
+
Quoted 8 lines
Quoted 3 lines

+     schedule_work(work);
+     return IRQ_HANDLED;
+}

The only place where we explicitly cancel the pending work is in gpio_setup_irq().  Is that sufficient?  Is there any way in which the work callback can occur after things have been freed/closed/deinitialised/etc?

Not that I noticed; it's not supposed to be possible to free/etc with IRQs active.

- Dave