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 on the /sys/class/gpio/gpioN/value
sysfs file. The edge to trigger can be set in the poll_edge file in the
same directory. Possible values are "none", "rising", "falling", and
"both".
Using level triggers is not possible with current sysfs as poll will
not return if the value did not change. On the other hand edge triggers
are relative to the last read by the application and not to the start
of poll. So if there was an event between read and poll, poll will
return immediately.
Signed-off-by: Daniel Glöckner <[email protected]>
---
Documentation/gpio.txt | 7 ++
drivers/gpio/gpiolib.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 148 insertions(+), 5 deletions(-)
diff --git a/Documentation/gpio.txt b/Documentation/gpio.txt
index 145c25a..eb2eb31 100644
--- a/Documentation/gpio.txt
+++ b/Documentation/gpio.txt
@@ -524,6 +524,13 @@ and have the following read/write attributes:
is configured as an output, this value may be written;
any nonzero value is treated as high.
+ "poll_edge" ... reads as either "none", "rising", "falling", or
+ "both". Write these strings to select the signal edge(s)
+ that will make poll 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:
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 51a8d41..9a37835 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1,5 +1,6 @@
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/interrupt.h>
#include <linux/irq.h>
#include <linux/spinlock.h>
#include <linux/device.h>
@@ -49,10 +50,14 @@ struct gpio_desc {
#define FLAG_RESERVED 2
#define FLAG_EXPORT 3 /* protected by sysfs_lock */
#define FLAG_SYSFS 4 /* exported via /sys/class/gpio/control */
+#define FLAG_TRIG_FALL 5 /* trigger on falling edge */
+#define FLAG_TRIG_RISE 6 /* trigger on rising edge */
#ifdef CONFIG_DEBUG_FS
const char *label;
#endif
+
+ struct sysfs_dirent *value_sd;
};
static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
@@ -188,10 +193,10 @@ static DEFINE_MUTEX(sysfs_lock);
* /value
* * always readable, subject to hardware behavior
* * may be writable, as zero/nonzero
- *
- * REVISIT there will likely be an attribute for configuring async
- * notifications, e.g. to specify polling interval or IRQ trigger type
- * that would for example trigger a poll() on the "value".
+ * /poll_edge
+ * * configures behavior of poll on /value
+ * * available only if pin can generate IRQs on input
+ * * is read/write as "none", "falling", "rising", or "both"
*/
static ssize_t gpio_direction_show(struct device *dev,
@@ -288,6 +293,106 @@ static ssize_t gpio_value_store(struct device *dev,
static /*const*/ DEVICE_ATTR(value, 0644,
gpio_value_show, gpio_value_store);
+static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
+{
+ struct gpio_desc *desc = priv;
+
+ sysfs_notify_dirent(desc->value_sd);
+ return IRQ_HANDLED;
+}
+
+static const struct {
+ const char *name;
+ unsigned long flags;
+} trigger_types[] = {
+ { "none", 0 },
+ { "falling", 1 << FLAG_TRIG_FALL },
+ { "rising", 1 << FLAG_TRIG_RISE },
+ { "both", (1 << FLAG_TRIG_FALL) | (1 << FLAG_TRIG_RISE) },
+};
+
+static ssize_t gpio_trigger_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ const struct gpio_desc *desc = dev_get_drvdata(dev);
+ ssize_t status;
+
+ mutex_lock(&sysfs_lock);
+
+ if (!test_bit(FLAG_EXPORT, &desc->flags))
+ status = -EIO;
+ else {
+ int i;
+ status = 0;
+ for (i = 0; i < ARRAY_SIZE(trigger_types); i++)
+ if ((desc->flags & ((1 << FLAG_TRIG_FALL)
+ | (1 << FLAG_TRIG_RISE)))
+ == trigger_types[i].flags) {
+ status = sprintf(buf, "%s\n",
+ trigger_types[i].name);
+ break;
+ }
+ }
+
+ mutex_unlock(&sysfs_lock);
+ return status;
+}
+
+static ssize_t gpio_trigger_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t size)
+{
+ struct gpio_desc *desc = dev_get_drvdata(dev);
+ ssize_t status;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(trigger_types); i++)
+ if (sysfs_streq(trigger_types[i].name, buf))
+ break;
+
+ mutex_lock(&sysfs_lock);
+
+ if (!test_bit(FLAG_EXPORT, &desc->flags))
+ status = -EIO;
+ else if (i == ARRAY_SIZE(trigger_types))
+ status = -EINVAL;
+ else {
+ int irq = gpio_to_irq(desc - gpio_desc);
+ if (irq >= 0) {
+ if (desc->flags & ((1 << FLAG_TRIG_FALL)
+ | (1 << FLAG_TRIG_RISE)))
+ free_irq(irq, desc);
+
+ desc->flags &= ~((1 << FLAG_TRIG_FALL)
+ | (1 << FLAG_TRIG_RISE));
+
+ if (trigger_types[i].flags & ((1 << FLAG_TRIG_FALL)
+ | (1 << FLAG_TRIG_RISE))) {
+ unsigned long flags = IRQF_SHARED;
+
+ if (test_bit(FLAG_TRIG_FALL,
+ &trigger_types[i].flags))
+ flags |= IRQF_TRIGGER_FALLING;
+ if (test_bit(FLAG_TRIG_RISE,
+ &trigger_types[i].flags))
+ flags |= IRQF_TRIGGER_RISING;
+ status = request_irq(irq, gpio_sysfs_irq, flags,
+ "gpiolib", desc);
+ if (!status) {
+ desc->flags |= trigger_types[i].flags;
+ status = size;
+ }
+ } else
+ status = size;
+ } else
+ status = -EIO;
+ }
+
+ mutex_unlock(&sysfs_lock);
+ return status;
+}
+
+static DEVICE_ATTR(poll_edge, 0644, gpio_trigger_show, gpio_trigger_store);
+
static const struct attribute *gpio_attrs[] = {
&dev_attr_direction.attr,
&dev_attr_value.attr,
@@ -481,8 +586,28 @@ int gpio_export(unsigned gpio, bool direction_may_change)
else
status = device_create_file(dev,
&dev_attr_value);
- if (status != 0)
+
+ if (!status) {
+ desc->value_sd = sysfs_get_dirent(dev->kobj.sd,
+ "value");
+ if (!desc->value_sd)
+ status = -ENODEV;
+ }
+
+ if (!status
+ && gpio_to_irq(gpio) >= 0
+ && (direction_may_change
+ || !test_bit(FLAG_IS_OUT, &desc->flags)))
+ status = device_create_file(dev,
+ &dev_attr_poll_edge);
+
+ if (status != 0) {
+ if (desc->value_sd) {
+ sysfs_put(desc->value_sd);
+ desc->value_sd = NULL;
+ }
device_unregister(dev);
+ }
} else
status = -ENODEV;
if (status == 0)
@@ -527,6 +652,17 @@ void gpio_unexport(unsigned gpio)
dev = class_find_device(&gpio_class, NULL, desc, match_export);
if (dev) {
+ if (desc->flags & ((1 << FLAG_TRIG_FALL)
+ | (1 << FLAG_TRIG_RISE))) {
+ free_irq(gpio_to_irq(gpio), desc);
+ desc->flags &= ~((1 << FLAG_TRIG_FALL)
+ | (1 << FLAG_TRIG_RISE));
+ }
+
+ if (desc->value_sd) {
+ sysfs_put(desc->value_sd);
+ desc->value_sd = NULL;
+ }
clear_bit(FLAG_EXPORT, &desc->flags);
put_device(dev);
device_unregister(dev);
--
1.6.1.3
Hey, good stuff Daniel. There's a fair few common features missing but
they can be added at a later date.
- Ability to honour rising and falling filters even if the hardware only
supports both-edge (as lots of gpio interrupts do)
- Support for polling the gpio at some interval for gpios which don't
support irqs at all
- Debounce support
- Reporting of number of changes since last read
These are all things which exist in many out-of-tree or
platform-specific implementations of this kind of thing and until
they're there I reckon people will largely stick with what they've got.
But that's really their problem of course, this is still valuable.
Regarding the code itself, not much but:
On Fri, 2009-06-05 at 16:36 +0200, Daniel Glöckner wrote:
>
> + "poll_edge" ... reads as either "none", "rising", "falling", or
IMO this is misleading, sounds like you're polling the gpio.
> +
> + struct sysfs_dirent *value_sd;
> };
No CONFIG_ option to turn all this off?
What's the .text and .data impact of this? Sure it's going to be small,
but to some people (especially those likely to care about gpio) 1k
of .data is something worth being able to turn off.
Using an IDR keyed to the gpio value and just allocating your useful
data structures when poll_edge != "none" would help too. It makes the
whole thing more future-proof as well as the addition of features from
the above list would probably bloat each struct gpio_desc further.
> static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
>
> @@ -188,10 +193,10 @@ static DEFINE_MUTEX(sysfs_lock);
> * /value
> * * always readable, subject to hardware behavior
> * * may be writable, as zero/nonzero
> - *
> - * REVISIT there will likely be an attribute for configuring async
> - * notifications, e.g. to specify polling interval or IRQ trigger type
> - * that would for example trigger a poll() on the "value".
> + * /poll_edge
> + * * configures behavior of poll on /value
Personally I like seeing poll(2) rather than poll, that word is so
overloaded clarification is be useful.
Technically the rest looks fine :-)
--Ben.
On Sat, 2009-06-06 at 13:46 +1000, Ben Nizette wrote:
> Using an IDR keyed to the gpio value and just allocating your useful
s/value/number
sorry :-)
--Ben.
On Friday 05 June 2009, Daniel Gl?ckner wrote:
> +???????else {
> +???????????????int i;
> +???????????????status = 0;
> +???????????????for (i = 0; i < ARRAY_SIZE(trigger_types); i++)
> +???????????????????????if ((desc->flags & ((1 << FLAG_TRIG_FALL)
> +??????????????????????????????????????? | (1 << FLAG_TRIG_RISE)))
> +??????????????????????? ? ? == trigger_types[i].flags) {
> +???????????????????????????????status = sprintf(buf, "%s\n",
> +??????????????????????????????????????????????? trigger_types[i].name);
> +???????????????????????????????break;
> +???????????????}
> +???????}
Looks pretty clean overall, but that snippet highlights some cosmetic
issues I'd like to see cleaned up:
- Blank line after each declaration block -- "int i" above.
- Prefer BIT(n) to (1 << n)
- And here, the ((1 << FLAG_TRIG_FALL) | (1 << FLAG_TRIG_RISE))
thing shows up all over, maybe #define as GPIO_TRIGGER_MASK
- That "== triger_types[..]" should use only tabs for indent.
And yes, like Ben I think this functionality is probably worth having,
since it's not something that *can* really be done from userspace with
the current calls.
- Dave
On Friday 05 June 2009, Ben Nizette wrote:
>
> Hey, good stuff Daniel. There's a fair few common features missing but
> they can be added at a later date.
>
> - Ability to honour rising and falling filters even if the hardware only
> supports both-edge (as lots of gpio interrupts do)
I suspect that's inadvisable. Userspace code will need to know
what trigger model it's using, yes? Lies are doubleplusungood.
> - Support for polling the gpio at some interval for gpios which don't
> support irqs at all
I had that thought. Units ... seconds? Milliseconds mapped to HZ?
Could come later.
> - Debounce support
Software? Hardware capabilties vary *widely* ... three cases that
come quickly to mind: (a) twl4030 fixed 30 msec delays, (b) at91
and avr32 "deglitch" filter, just syncs to a clock that's likely
from 30-100 MHz, (c) omap2/omap3, up to 255 cycles of 32 KiHz clock
but appplies entire GPIO banks, (d) DaVinci, no hardware support.
I can imagine a standard software filter option, but that would
need to be a separate sysfs mechanism since it wouldn't always
be desired. (And separate patch, if needed.)
For hardware options ... do that by hardware-specific sysfs hooks
if they're really needed.
> - Reporting of number of changes since last read
Feels a more than bit overkilled by now. ;)
> These are all things which exist in many out-of-tree or
> platform-specific implementations of this kind of thing and until
> they're there I reckon people will largely stick with what they've got.
> But that's really their problem of course, this is still valuable.
>
> Regarding the code itself, not much but:
>
> On Fri, 2009-06-05 at 16:36 +0200, Daniel Gl?ckner wrote:
> >
> > + "poll_edge" ... reads as either "none", "rising", "falling", or
>
> IMO this is misleading, sounds like you're polling the gpio.
So, just name the sysfs attribute "edge"?
> > +
> > + struct sysfs_dirent *value_sd;
> > };
>
> No CONFIG_ option to turn all this off?
>
> What's the .text and .data impact of this? Sure it's going to be small,
> but to some people (especially those likely to care about gpio) 1k
> of .data is something worth being able to turn off.
I think it's probably OK to have this covered by the current
GPIO_SYSFS flag.
> Using an IDR keyed to the gpio value and just allocating your useful
> data structures when poll_edge != "none" would help too.
Can do that without an IDR, I think...
On Fri, 2009-06-05 at 23:01 -0700, David Brownell wrote:
> On Friday 05 June 2009, Ben Nizette wrote:
> >
> > Hey, good stuff Daniel. There's a fair few common features missing but
> > they can be added at a later date.
> >
> > - Ability to honour rising and falling filters even if the hardware only
> > supports both-edge (as lots of gpio interrupts do)
>
> I suspect that's inadvisable. Userspace code will need to know
> what trigger model it's using, yes? Lies are doubleplusungood.
>
Well if the user wants to know that poll(2) will report each time value
transitions 0 -> 1 for example then why should it care whether that
filtering has come straight from the interrupt controller or some piece
of code has simply discarded the 1 -> 0 transitions for it?
Doing this in the kernel is a good idea IMO - the probability of the
gpio changing value between and interrupt and it's handler is much lower
than between interrupt and userspace reading "value".
Of course document that pulses smaller than interrupt latency may be
missed - a limitation which I think is perfectly fair.
>
> > - Support for polling the gpio at some interval for gpios which don't
> > support irqs at all
>
> I had that thought. Units ... seconds? Milliseconds mapped to HZ?
> Could come later.
Certainly could come later. ms are as good a choice as any, just so
long as root can set a lower bound on polling period.
>
>
> > - Debounce support
>
> Software? Hardware capabilties vary *widely* ... three cases that
> come quickly to mind: (a) twl4030 fixed 30 msec delays, (b) at91
> and avr32 "deglitch" filter, just syncs to a clock that's likely
> from 30-100 MHz, (c) omap2/omap3, up to 255 cycles of 32 KiHz clock
> but appplies entire GPIO banks, (d) DaVinci, no hardware support.
>
> I can imagine a standard software filter option, but that would
> need to be a separate sysfs mechanism since it wouldn't always
> be desired. (And separate patch, if needed.)
>
> For hardware options ... do that by hardware-specific sysfs hooks
> if they're really needed.
Oh gods, software for sure. Yeah hardware debounce support is something
which I'm not game to tackle.
I guess debouncing can be done by userspace though. In fact it almost
is done to some approximation simply by the fact that by the time the
user actually gets around to reacting the pin's probably settled anyway.
>
>
> > - Reporting of number of changes since last read
>
> Feels a more than bit overkilled by now. ;)
UIO does it and I've found it useful there. I've also had requests to
implement this in my old gpio sysfs notification implementation (now
sadly neglected). Apart from anything else it leads to easy
implementation for things like robust event counters - a common use case
for this kind of thing.
>
>
> > These are all things which exist in many out-of-tree or
> > platform-specific implementations of this kind of thing and until
> > they're there I reckon people will largely stick with what they've got.
> > But that's really their problem of course, this is still valuable.
> >
> > Regarding the code itself, not much but:
> >
> > On Fri, 2009-06-05 at 16:36 +0200, Daniel Glöckner wrote:
> > >
> > > + "poll_edge" ... reads as either "none", "rising", "falling", or
> >
> > IMO this is misleading, sounds like you're polling the gpio.
>
> So, just name the sysfs attribute "edge"?
>
Sure. Or notification_edge though it's a mouthful. "poll" is just so
horribly overloaded in this field.
>
> > > +
> > > + struct sysfs_dirent *value_sd;
> > > };
> >
> > No CONFIG_ option to turn all this off?
> >
> > What's the .text and .data impact of this? Sure it's going to be small,
> > but to some people (especially those likely to care about gpio) 1k
> > of .data is something worth being able to turn off.
>
> I think it's probably OK to have this covered by the current
> GPIO_SYSFS flag.
Fair 'nuff. AFAICT this code is covered by that option already but the
value_sd field still needs to be wrapped.
>
>
> > Using an IDR keyed to the gpio value and just allocating your useful
> > data structures when poll_edge != "none" would help too.
>
> Can do that without an IDR, I think...
Sure. If you're dynamically allocating these data structures though you
need some way to keep track of them. Either point to them with a field
in struct gpio_desc (which of course misses the point), chain them
together in a list or tree or $(EXOTIC_DATA_STRUCTURE) or stick them in
an IDR. IDRs just happen to be light, easy, fast and suit this
integer-to-pointer case.
--Ben.
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 as poll will
not return if the value did not change. On the other hand edge triggers
are relative to the last read by the application and not to the start
of poll. So if there was an event between read and poll, poll will
return immediately.
Changes compared to v1:
- use workqueue to call sysfs_notify_dirent as it takes a spinlock
- move reconfiguration into separate function
- allocate work_struct & co. as needed
- wrap additional element in gpio_desc in #ifdef CONFIG_GPIO_SYSFS
- rename poll_edge to edge
- update Documentation/ABI/testing/sysfs-gpio as well
- refer to poll syscall as "poll(2)"
- use BIT() and define trigger mask for more readable expressions
- whitespace changes
Signed-off-by: Daniel Glöckner <[email protected]>
---
Documentation/ABI/testing/sysfs-gpio | 1 +
Documentation/gpio.txt | 7 ++
drivers/gpio/gpiolib.c | 176 +++++++++++++++++++++++++++++++++-
3 files changed, 180 insertions(+), 4 deletions(-)
Due to the need for a work_struct (16 bytes) per polled pin I created a
poll_desc structure which also keeps the sysfs_dirent pointer. The memory
requirement in gpio_desc is still 4 bytes. Is this acceptable?
I have looked into IDRs to store the pointer to the poll_desc structure.
It appears the interface is not designed to allow allocation of specific
numbers. One would have to rely on idr_get_new_above to return the number
passed in. Another possibility would be to use some of the 25 unused bits
of the flags variable to store the ID.
Daniel
diff --git a/Documentation/ABI/testing/sysfs-gpio b/Documentation/ABI/testing/sysfs-gpio
index 8aab809..80f4c94 100644
--- a/Documentation/ABI/testing/sysfs-gpio
+++ b/Documentation/ABI/testing/sysfs-gpio
@@ -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
diff --git a/Documentation/gpio.txt b/Documentation/gpio.txt
index 145c25a..68848b5 100644
--- a/Documentation/gpio.txt
+++ b/Documentation/gpio.txt
@@ -524,6 +524,13 @@ and have the following read/write attributes:
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:
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 51a8d41..e239975 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1,5 +1,6 @@
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/interrupt.h>
#include <linux/irq.h>
#include <linux/spinlock.h>
#include <linux/device.h>
@@ -40,6 +41,11 @@
*/
static DEFINE_SPINLOCK(gpio_lock);
+struct poll_desc {
+ struct work_struct work;
+ struct sysfs_dirent *value_sd;
+};
+
struct gpio_desc {
struct gpio_chip *chip;
unsigned long flags;
@@ -49,10 +55,18 @@ struct gpio_desc {
#define FLAG_RESERVED 2
#define FLAG_EXPORT 3 /* protected by sysfs_lock */
#define FLAG_SYSFS 4 /* exported via /sys/class/gpio/control */
+#define FLAG_TRIG_FALL 5 /* trigger on falling edge */
+#define FLAG_TRIG_RISE 6 /* trigger on rising edge */
+
+#define GPIO_TRIGGER_MASK (BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE))
#ifdef CONFIG_DEBUG_FS
const char *label;
#endif
+
+#ifdef CONFIG_GPIO_SYSFS
+ struct poll_desc *pdesc;
+#endif
};
static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
@@ -188,10 +202,10 @@ static DEFINE_MUTEX(sysfs_lock);
* /value
* * always readable, subject to hardware behavior
* * may be writable, as zero/nonzero
- *
- * REVISIT there will likely be an attribute for configuring async
- * notifications, e.g. to specify polling interval or IRQ trigger type
- * that would for example trigger a poll() on the "value".
+ * /edge
+ * * configures behavior of poll(2) on /value
+ * * available only if pin can generate IRQs on input
+ * * is read/write as "none", "falling", "rising", or "both"
*/
static ssize_t gpio_direction_show(struct device *dev,
@@ -288,6 +302,151 @@ static ssize_t gpio_value_store(struct device *dev,
static /*const*/ DEVICE_ATTR(value, 0644,
gpio_value_show, gpio_value_store);
+static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
+{
+ struct work_struct *work = priv;
+
+ schedule_work(work);
+ return IRQ_HANDLED;
+}
+
+static void gpio_notify_sysfs(struct work_struct *work)
+{
+ struct poll_desc *pdesc;
+
+ pdesc = container_of(work, struct poll_desc, work);
+ sysfs_notify_dirent(pdesc->value_sd);
+}
+
+static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
+ unsigned long gpio_flags)
+{
+ unsigned long irq_flags;
+ int ret, irq;
+
+ if ((desc->flags & GPIO_TRIGGER_MASK) == gpio_flags)
+ return 0;
+
+ irq = gpio_to_irq(desc - gpio_desc);
+ if (irq < 0)
+ return -EIO;
+
+ if (desc->flags & GPIO_TRIGGER_MASK) {
+ free_irq(irq, &desc->pdesc->work);
+ cancel_work_sync(&desc->pdesc->work);
+ }
+
+ desc->flags &= ~GPIO_TRIGGER_MASK;
+
+ if (!gpio_flags) {
+ ret = 0;
+ goto free_sd;
+ }
+
+ irq_flags = IRQF_SHARED;
+ if (test_bit(FLAG_TRIG_FALL, &gpio_flags))
+ irq_flags |= IRQF_TRIGGER_FALLING;
+ if (test_bit(FLAG_TRIG_RISE, &gpio_flags))
+ irq_flags |= IRQF_TRIGGER_RISING;
+ if (!desc->pdesc) {
+ desc->pdesc = kmalloc(sizeof(struct poll_desc), GFP_KERNEL);
+ if (!desc->pdesc) {
+ ret = -ENOMEM;
+ goto err_out;
+ }
+ desc->pdesc->value_sd = sysfs_get_dirent(dev->kobj.sd,
+ "value");
+ if (!desc->pdesc->value_sd) {
+ ret = -ENODEV;
+ goto free_mem;
+ }
+ INIT_WORK(&desc->pdesc->work, gpio_notify_sysfs);
+ }
+
+ ret = request_irq(irq, gpio_sysfs_irq, irq_flags,
+ "gpiolib", &desc->pdesc->work);
+ if (ret)
+ goto free_sd;
+
+ desc->flags |= gpio_flags;
+ return 0;
+
+free_sd:
+ sysfs_put(desc->pdesc->value_sd);
+free_mem:
+ kfree(desc->pdesc);
+ desc->pdesc = NULL;
+err_out:
+ return ret;
+}
+
+static const struct {
+ const char *name;
+ unsigned long flags;
+} trigger_types[] = {
+ { "none", 0 },
+ { "falling", BIT(FLAG_TRIG_FALL) },
+ { "rising", BIT(FLAG_TRIG_RISE) },
+ { "both", BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE) },
+};
+
+static ssize_t gpio_edge_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ const struct gpio_desc *desc = dev_get_drvdata(dev);
+ ssize_t status;
+
+ mutex_lock(&sysfs_lock);
+
+ if (!test_bit(FLAG_EXPORT, &desc->flags))
+ status = -EIO;
+ else {
+ int i;
+
+ status = 0;
+ for (i = 0; i < ARRAY_SIZE(trigger_types); i++)
+ if ((desc->flags & GPIO_TRIGGER_MASK)
+ == trigger_types[i].flags) {
+ status = sprintf(buf, "%s\n",
+ trigger_types[i].name);
+ break;
+ }
+ }
+
+ mutex_unlock(&sysfs_lock);
+ return status;
+}
+
+static ssize_t gpio_edge_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t size)
+{
+ struct gpio_desc *desc = dev_get_drvdata(dev);
+ ssize_t status;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(trigger_types); i++)
+ if (sysfs_streq(trigger_types[i].name, buf))
+ break;
+
+ mutex_lock(&sysfs_lock);
+
+ if (!test_bit(FLAG_EXPORT, &desc->flags))
+ status = -EIO;
+ else if (i == ARRAY_SIZE(trigger_types))
+ status = -EINVAL;
+ else {
+ status = gpio_setup_irq(desc, dev, trigger_types[i].flags);
+ if (!status)
+ status = size;
+ }
+
+ mutex_unlock(&sysfs_lock);
+
+ return status;
+}
+
+static DEVICE_ATTR(edge, 0644, gpio_edge_show, gpio_edge_store);
+
static const struct attribute *gpio_attrs[] = {
&dev_attr_direction.attr,
&dev_attr_value.attr,
@@ -481,6 +640,14 @@ int gpio_export(unsigned gpio, bool direction_may_change)
else
status = device_create_file(dev,
&dev_attr_value);
+
+ if (!status
+ && gpio_to_irq(gpio) >= 0
+ && (direction_may_change
+ || !test_bit(FLAG_IS_OUT, &desc->flags)))
+ status = device_create_file(dev,
+ &dev_attr_edge);
+
if (status != 0)
device_unregister(dev);
} else
@@ -527,6 +694,7 @@ void gpio_unexport(unsigned gpio)
dev = class_find_device(&gpio_class, NULL, desc, match_export);
if (dev) {
+ gpio_setup_irq(desc, dev, 0);
clear_bit(FLAG_EXPORT, &desc->flags);
put_device(dev);
device_unregister(dev);
--
1.6.1.3
On Wednesday 10 June 2009, Daniel Gl?ckner wrote:
> 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".
Looks pretty clean. Comments from anyone else?
Presumably you tested this on Real Hardware(tm). At one point
I thought sysfs poll() support looked a bit flakey ... was it
acting OK for you in the face of repeated triggers of events?
> Using level triggers is not possible with current sysfs as poll will
> not return if the value did not change. On the other hand edge triggers
> are relative to the last read by the application and not to the start
> of poll. So if there was an event between read and poll, poll will
> return immediately.
>
> Changes compared to v1:
> - use workqueue to call sysfs_notify_dirent as it takes a spinlock
Do you mean "mutex"? IRQ handing code can grab most
spinocks ... so long as they're irq-safe spinlocks.
Either way, if you need a mutex, add a comment saying
why the work_struct is needed.
> - move reconfiguration into separate function
> - allocate work_struct & co. as needed
> - wrap additional element in gpio_desc in #ifdef CONFIG_GPIO_SYSFS
> - rename poll_edge to edge
> - update Documentation/ABI/testing/sysfs-gpio as well
> - refer to poll syscall as "poll(2)"
> - use BIT() and define trigger mask for more readable expressions
> - whitespace changes
Even so, I have a few cosmetic comments. :)
> Signed-off-by: Daniel Gl?ckner <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-gpio | 1 +
> Documentation/gpio.txt | 7 ++
> drivers/gpio/gpiolib.c | 176 +++++++++++++++++++++++++++++++++-
> 3 files changed, 180 insertions(+), 4 deletions(-)
>
>
> Due to the need for a work_struct (16 bytes) per polled pin I created a
> poll_desc structure which also keeps the sysfs_dirent pointer. The memory
> requirement in gpio_desc is still 4 bytes. Is this acceptable?
It's more like "one pointer" not "4 bytes" ... although most
systems wanting this will be using 32-bit embedded CPUs.
So the additional *fixed* overhead is ARCH_NR_GPIOS*sizeof(void*),
or 1 KByte on most systems.
> I have looked into IDRs to store the pointer to the poll_desc structure.
> It appears the interface is not designed to allow allocation of specific
> numbers. One would have to rely on idr_get_new_above to return the number
> passed in. Another possibility would be to use some of the 25 unused bits
> of the flags variable to store the ID.
The reason to want to allocate about log2(ARCH_NR_GPIOS) of those
bits would be to save that 1KByte. This stuff isn't critical path,
and most of that KByte will never be used... few systems will use
more than two GPIOs for such event triggering. An IDR would take
less than 1KB in such a case, I think.
So the call isn't quite straightforward. Maybe other folk have
some feedback.
> Daniel
>
> diff --git a/Documentation/ABI/testing/sysfs-gpio b/Documentation/ABI/testing/sysfs-gpio
> index 8aab809..80f4c94 100644
> --- a/Documentation/ABI/testing/sysfs-gpio
> +++ b/Documentation/ABI/testing/sysfs-gpio
> @@ -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
> diff --git a/Documentation/gpio.txt b/Documentation/gpio.txt
> index 145c25a..68848b5 100644
> --- a/Documentation/gpio.txt
> +++ b/Documentation/gpio.txt
> @@ -524,6 +524,13 @@ and have the following read/write attributes:
> 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:
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 51a8d41..e239975 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1,5 +1,6 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/interrupt.h>
> #include <linux/irq.h>
> #include <linux/spinlock.h>
> #include <linux/device.h>
> @@ -40,6 +41,11 @@
> */
> static DEFINE_SPINLOCK(gpio_lock);
>
> +struct poll_desc {
> + struct work_struct work;
> + struct sysfs_dirent *value_sd;
> +};
> +
> struct gpio_desc {
> struct gpio_chip *chip;
> unsigned long flags;
> @@ -49,10 +55,18 @@ struct gpio_desc {
> #define FLAG_RESERVED 2
> #define FLAG_EXPORT 3 /* protected by sysfs_lock */
> #define FLAG_SYSFS 4 /* exported via /sys/class/gpio/control */
> +#define FLAG_TRIG_FALL 5 /* trigger on falling edge */
> +#define FLAG_TRIG_RISE 6 /* trigger on rising edge */
> +
> +#define GPIO_TRIGGER_MASK (BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE))
>
> #ifdef CONFIG_DEBUG_FS
> const char *label;
> #endif
> +
> +#ifdef CONFIG_GPIO_SYSFS
> + struct poll_desc *pdesc;
> +#endif
> };
> static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
>
> @@ -188,10 +202,10 @@ static DEFINE_MUTEX(sysfs_lock);
> * /value
> * * always readable, subject to hardware behavior
> * * may be writable, as zero/nonzero
> - *
> - * REVISIT there will likely be an attribute for configuring async
> - * notifications, e.g. to specify polling interval or IRQ trigger type
> - * that would for example trigger a poll() on the "value".
> + * /edge
> + * * configures behavior of poll(2) on /value
> + * * available only if pin can generate IRQs on input
> + * * is read/write as "none", "falling", "rising", or "both"
> */
>
> static ssize_t gpio_direction_show(struct device *dev,
> @@ -288,6 +302,151 @@ static ssize_t gpio_value_store(struct device *dev,
> static /*const*/ DEVICE_ATTR(value, 0644,
> gpio_value_show, gpio_value_store);
>
> +static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
> +{
> + struct work_struct *work = priv;
> +
> + schedule_work(work);
> + return IRQ_HANDLED;
> +}
> +
> +static void gpio_notify_sysfs(struct work_struct *work)
> +{
> + struct poll_desc *pdesc;
> +
> + pdesc = container_of(work, struct poll_desc, work);
> + sysfs_notify_dirent(pdesc->value_sd);
> +}
> +
> +static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
> + unsigned long gpio_flags)
> +{
> + unsigned long irq_flags;
> + int ret, irq;
> +
> + if ((desc->flags & GPIO_TRIGGER_MASK) == gpio_flags)
> + return 0;
> +
> + irq = gpio_to_irq(desc - gpio_desc);
> + if (irq < 0)
> + return -EIO;
> +
> + if (desc->flags & GPIO_TRIGGER_MASK) {
> + free_irq(irq, &desc->pdesc->work);
> + cancel_work_sync(&desc->pdesc->work);
> + }
> +
> + desc->flags &= ~GPIO_TRIGGER_MASK;
> +
> + if (!gpio_flags) {
> + ret = 0;
> + goto free_sd;
> + }
> +
> + irq_flags = IRQF_SHARED;
> + if (test_bit(FLAG_TRIG_FALL, &gpio_flags))
> + irq_flags |= IRQF_TRIGGER_FALLING;
> + if (test_bit(FLAG_TRIG_RISE, &gpio_flags))
> + irq_flags |= IRQF_TRIGGER_RISING;
> + if (!desc->pdesc) {
> + desc->pdesc = kmalloc(sizeof(struct poll_desc), GFP_KERNEL);
> + if (!desc->pdesc) {
> + ret = -ENOMEM;
> + goto err_out;
> + }
> + desc->pdesc->value_sd = sysfs_get_dirent(dev->kobj.sd,
> + "value");
> + if (!desc->pdesc->value_sd) {
> + ret = -ENODEV;
> + goto free_mem;
> + }
> + INIT_WORK(&desc->pdesc->work, gpio_notify_sysfs);
> + }
> +
> + ret = request_irq(irq, gpio_sysfs_irq, irq_flags,
> + "gpiolib", &desc->pdesc->work);
> + if (ret)
> + goto free_sd;
> +
> + desc->flags |= gpio_flags;
> + return 0;
> +
> +free_sd:
> + sysfs_put(desc->pdesc->value_sd);
> +free_mem:
> + kfree(desc->pdesc);
> + desc->pdesc = NULL;
> +err_out:
> + return ret;
> +}
> +
> +static const struct {
> + const char *name;
> + unsigned long flags;
> +} trigger_types[] = {
> + { "none", 0 },
> + { "falling", BIT(FLAG_TRIG_FALL) },
> + { "rising", BIT(FLAG_TRIG_RISE) },
> + { "both", BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE) },
> +};
> +
> +static ssize_t gpio_edge_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + const struct gpio_desc *desc = dev_get_drvdata(dev);
> + ssize_t status;
> +
> + mutex_lock(&sysfs_lock);
> +
> + if (!test_bit(FLAG_EXPORT, &desc->flags))
> + status = -EIO;
> + else {
> + int i;
> +
> + status = 0;
> + for (i = 0; i < ARRAY_SIZE(trigger_types); i++)
> + if ((desc->flags & GPIO_TRIGGER_MASK)
> + == trigger_types[i].flags) {
> + status = sprintf(buf, "%s\n",
> + trigger_types[i].name);
> + break;
> + }
> + }
> +
> + mutex_unlock(&sysfs_lock);
> + return status;
> +}
> +
> +static ssize_t gpio_edge_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t size)
> +{
> + struct gpio_desc *desc = dev_get_drvdata(dev);
> + ssize_t status;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(trigger_types); i++)
> + if (sysfs_streq(trigger_types[i].name, buf))
> + break;
It'd be more clear to return -EINVAL for bogus "buf" *here* not
lower down, without taking the mutex. Likely swapping a "goto"
for that "break" would let it be a net code shrink.
> +
> + mutex_lock(&sysfs_lock);
> +
> + if (!test_bit(FLAG_EXPORT, &desc->flags))
> + status = -EIO;
> + else if (i == ARRAY_SIZE(trigger_types))
> + status = -EINVAL;
> + else {
> + status = gpio_setup_irq(desc, dev, trigger_types[i].flags);
> + if (!status)
> + status = size;
> + }
> +
> + mutex_unlock(&sysfs_lock);
> +
> + return status;
> +}
> +
> +static DEVICE_ATTR(edge, 0644, gpio_edge_show, gpio_edge_store);
> +
> static const struct attribute *gpio_attrs[] = {
> &dev_attr_direction.attr,
> &dev_attr_value.attr,
> @@ -481,6 +640,14 @@ int gpio_export(unsigned gpio, bool direction_may_change)
> else
> status = device_create_file(dev,
> &dev_attr_value);
> +
> + if (!status
> + && gpio_to_irq(gpio) >= 0
> + && (direction_may_change
> + || !test_bit(FLAG_IS_OUT, &desc->flags)))
Cosmetic: use *only* tabs to indent. All of them at least
one more level than the "status = ..." body of the test.
> + status = device_create_file(dev,
> + &dev_attr_edge);
> +
> if (status != 0)
> device_unregister(dev);
> } else
> @@ -527,6 +694,7 @@ void gpio_unexport(unsigned gpio)
>
> dev = class_find_device(&gpio_class, NULL, desc, match_export);
> if (dev) {
> + gpio_setup_irq(desc, dev, 0);
> clear_bit(FLAG_EXPORT, &desc->flags);
> put_device(dev);
> device_unregister(dev);
> --
> 1.6.1.3
>
>
On Wed, 2009-07-01 at 16:05 -0700, David Brownell wrote:
> On Wednesday 10 June 2009, Daniel Glöckner wrote:
> > 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".
>
> Looks pretty clean. Comments from anyone else?
FWIW I like it, though packing id in to upper bits of ->flags has a
certain appeal to me.
>
> Presumably you tested this on Real Hardware(tm). At one point
> I thought sysfs poll() support looked a bit flakey ... was it
> acting OK for you in the face of repeated triggers of events?
Has worked OK for me though of course there's no way to know how many
triggers occurred between polls
>
>
> > Using level triggers is not possible with current sysfs as poll will
> > not return if the value did not change. On the other hand edge triggers
> > are relative to the last read by the application and not to the start
> > of poll. So if there was an event between read and poll, poll will
> > return immediately.
> >
> > Changes compared to v1:
> > - use workqueue to call sysfs_notify_dirent as it takes a spinlock
>
> Do you mean "mutex"? IRQ handing code can grab most
> spinocks ... so long as they're irq-safe spinlocks.
> Either way, if you need a mutex, add a comment saying
> why the work_struct is needed.
No, spinlock. sysfs_notify() takes the sysfs mutex but if you've
already got a reference to the dirent and call sysfs_notify_dirent()
directly it just takes the sysfs_open_dirent_lock spinlock.
This can be taken from interrupt if you go through and make a few
s/spin_lock/spin_lock_irqsave/ (and _irqrestore) throughout the sysfs
infrastructure but prolly just moving to workqueue is easier.
>
>
> > - move reconfiguration into separate function
> > - allocate work_struct & co. as needed
> > - wrap additional element in gpio_desc in #ifdef CONFIG_GPIO_SYSFS
> > - rename poll_edge to edge
> > - update Documentation/ABI/testing/sysfs-gpio as well
> > - refer to poll syscall as "poll(2)"
> > - use BIT() and define trigger mask for more readable expressions
> > - whitespace changes
>
> Even so, I have a few cosmetic comments. :)
>
>
> > Signed-off-by: Daniel Glöckner <[email protected]>
> > ---
> > Documentation/ABI/testing/sysfs-gpio | 1 +
> > Documentation/gpio.txt | 7 ++
> > drivers/gpio/gpiolib.c | 176 +++++++++++++++++++++++++++++++++-
> > 3 files changed, 180 insertions(+), 4 deletions(-)
> >
> >
> > Due to the need for a work_struct (16 bytes) per polled pin I created a
> > poll_desc structure which also keeps the sysfs_dirent pointer. The memory
> > requirement in gpio_desc is still 4 bytes. Is this acceptable?
>
> It's more like "one pointer" not "4 bytes" ... although most
> systems wanting this will be using 32-bit embedded CPUs.
>
> So the additional *fixed* overhead is ARCH_NR_GPIOS*sizeof(void*),
> or 1 KByte on most systems.
>
>
> > I have looked into IDRs to store the pointer to the poll_desc structure.
> > It appears the interface is not designed to allow allocation of specific
> > numbers. One would have to rely on idr_get_new_above to return the number
> > passed in.
Which it will unless it's already allocated. But yeah, it don't 'spose
it's documented behaviour.
> Another possibility would be to use some of the 25 unused bits
> > of the flags variable to store the ID.
Not a bad plan methinks.
>
> The reason to want to allocate about log2(ARCH_NR_GPIOS) of those
> bits would be to save that 1KByte. This stuff isn't critical path,
> and most of that KByte will never be used... few systems will use
> more than two GPIOs for such event triggering. An IDR would take
> less than 1KB in such a case, I think.
>
> So the call isn't quite straightforward. Maybe other folk have
> some feedback.
IDR will take less than 1K and it'll be nicely lost in the heap.
Packing in to upper bits of flags will take nothing at all - a nice
trick if you can pull it off.
--Ben.
On Thu, Jul 2, 2009 at 2:05 AM, David Brownell<[email protected]> wrote:
> On Wednesday 10 June 2009, Daniel Glöckner wrote:
>> 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".
>
> Looks pretty clean. Comments from anyone else?
I'd appreciate a kernel side interface to gpio_setup_irq functionality
as well; now gpio_export from kernel won't setup support for poll(2)
nor is there any possibility to do so. Having said that, it can be
done in another patch, another day, I don't want that to be holding
this patch back.
And I guess it might also be argued that anyone wanting to poll(2) can
setup edge triggering from user space when needed. Other thoughts on
that?
BR,
Jani.
On Thursday 02 July 2009, Jani Nikula wrote:
> > Looks pretty clean. ?Comments from anyone else?
>
> I'd appreciate a kernel side interface to gpio_setup_irq functionality
> as well; now gpio_export from kernel won't setup support for poll(2)
> nor is there any possibility to do so.
I don't understand what you mean by this... there's no
file descriptor open at that time, so there's no poll()
that could be done.
And kernel-side functionality can just use existing
interfaces to do whatever it likes...
On Thu, Jul 2, 2009 at 21:57, David Brownell<[email protected]> wrote:
> On Thursday 02 July 2009, Jani Nikula wrote:
>> > Looks pretty clean. Comments from anyone else?
>>
>> I'd appreciate a kernel side interface to gpio_setup_irq functionality
>> as well; now gpio_export from kernel won't setup support for poll(2)
>> nor is there any possibility to do so.
>
> I don't understand what you mean by this... there's no
> file descriptor open at that time, so there's no poll()
> that could be done.
I'm trying to say that if you call gpio_export() to create a sysfs
node, a subsequent poll() in user space won't work because there is no
irq set up, and sysfs_notify_dirent() won't be called. The patch
doesn't provide an interface for setting that up from kernel after
gpio_export() call - it's only possible to set up edge triggering and
sysfs notify by writing to the "edge" node from user space. That may
well be deemed sufficient, at least for now, and can be fixed later if
need be.
BR,
Jani.
On Thursday 02 July 2009, Jani Nikula wrote:
> I'm trying to say that if you call gpio_export() to create a sysfs
> node, a subsequent poll() in user space won't work because there is no
> irq set up, and sysfs_notify_dirent() won't be called. The patch
> doesn't provide an interface for setting that up from kernel after
> gpio_export() call - it's only possible to set up edge triggering and
> sysfs notify by writing to the "edge" node from user space.
OK, now I understand. :)
Yeah, that seems fixable in followups.
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 as poll will
not return if the value did not change. On the other hand edge triggers
are relative to the last read by the application and not to the start
of poll. So if there was an event between read and poll, poll will
return immediately.
Changes compared to v2:
- use idr to store pointer to poll_desc
- use upper bits of gpio_desc.flags to store id for idr
- exit early from gpio_edge_store on bad string
- more whitespace changes
Changes of v2 compared to v1:
- use workqueue to call sysfs_notify_dirent as it takes a spinlock
- move reconfiguration into separate function
- allocate work_struct & co. as needed
- wrap additional element in gpio_desc in #ifdef CONFIG_GPIO_SYSFS
- rename poll_edge to edge
- update Documentation/ABI/testing/sysfs-gpio as well
- refer to poll syscall as "poll(2)"
- use BIT() and define trigger mask for more readable expressions
- whitespace changes
Signed-off-by: Daniel Glöckner <[email protected]>
---
Documentation/ABI/testing/sysfs-gpio | 1 +
Documentation/gpio.txt | 7 +
drivers/gpio/gpiolib.c | 205 +++++++++++++++++++++++++++++++++-
3 files changed, 209 insertions(+), 4 deletions(-)
As requested in previous versions I am now using an idr to reduce the
memory footprint. Currently all 25 unused bits of the flags element are
used to store the id. When new flags are needed, this can be reduced.
For simplicity the code assumes the id is in the upper bits.
I tested v3 on a s6105 that I have sitting on my table.
Former revisions have been tested on an i.MX1 as well.
Daniel
diff --git a/Documentation/ABI/testing/sysfs-gpio b/Documentation/ABI/testing/sysfs-gpio
index 8aab809..80f4c94 100644
--- a/Documentation/ABI/testing/sysfs-gpio
+++ b/Documentation/ABI/testing/sysfs-gpio
@@ -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
diff --git a/Documentation/gpio.txt b/Documentation/gpio.txt
index e4b6985..bcf12fa 100644
--- a/Documentation/gpio.txt
+++ b/Documentation/gpio.txt
@@ -524,6 +524,13 @@ and have the following read/write attributes:
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:
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 51a8d41..73d45b5 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1,5 +1,6 @@
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/interrupt.h>
#include <linux/irq.h>
#include <linux/spinlock.h>
#include <linux/device.h>
@@ -7,6 +8,7 @@
#include <linux/debugfs.h>
#include <linux/seq_file.h>
#include <linux/gpio.h>
+#include <linux/idr.h>
/* Optional implementation infrastructure for GPIO interfaces.
@@ -49,6 +51,12 @@ struct gpio_desc {
#define FLAG_RESERVED 2
#define FLAG_EXPORT 3 /* protected by sysfs_lock */
#define FLAG_SYSFS 4 /* exported via /sys/class/gpio/control */
+#define FLAG_TRIG_FALL 5 /* trigger on falling edge */
+#define FLAG_TRIG_RISE 6 /* trigger on rising edge */
+#define PDESC_ID_SHIFT 7 /* add new flags before this one */
+
+#define GPIO_FLAGS_MASK ((1 << PDESC_ID_SHIFT) - 1)
+#define GPIO_TRIGGER_MASK (BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE))
#ifdef CONFIG_DEBUG_FS
const char *label;
@@ -56,6 +64,15 @@ struct gpio_desc {
};
static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
+#ifdef CONFIG_GPIO_SYSFS
+struct poll_desc {
+ struct work_struct work;
+ struct sysfs_dirent *value_sd;
+};
+
+static struct idr pdesc_idr;
+#endif
+
static inline void desc_set_label(struct gpio_desc *d, const char *label)
{
#ifdef CONFIG_DEBUG_FS
@@ -188,10 +205,10 @@ static DEFINE_MUTEX(sysfs_lock);
* /value
* * always readable, subject to hardware behavior
* * may be writable, as zero/nonzero
- *
- * REVISIT there will likely be an attribute for configuring async
- * notifications, e.g. to specify polling interval or IRQ trigger type
- * that would for example trigger a poll() on the "value".
+ * /edge
+ * * configures behavior of poll(2) on /value
+ * * available only if pin can generate IRQs on input
+ * * is read/write as "none", "falling", "rising", or "both"
*/
static ssize_t gpio_direction_show(struct device *dev,
@@ -288,6 +305,176 @@ static ssize_t gpio_value_store(struct device *dev,
static /*const*/ DEVICE_ATTR(value, 0644,
gpio_value_show, gpio_value_store);
+static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
+{
+ struct work_struct *work = priv;
+
+ schedule_work(work);
+ return IRQ_HANDLED;
+}
+
+static void gpio_notify_sysfs(struct work_struct *work)
+{
+ struct poll_desc *pdesc;
+
+ pdesc = container_of(work, struct poll_desc, work);
+ sysfs_notify_dirent(pdesc->value_sd);
+}
+
+static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
+ unsigned long gpio_flags)
+{
+ struct poll_desc *pdesc;
+ unsigned long irq_flags;
+ int ret, irq, id;
+
+ if ((desc->flags & GPIO_TRIGGER_MASK) == gpio_flags)
+ return 0;
+
+ irq = gpio_to_irq(desc - gpio_desc);
+ if (irq < 0)
+ return -EIO;
+
+ id = desc->flags >> PDESC_ID_SHIFT;
+ pdesc = idr_find(&pdesc_idr, id);
+ if (pdesc) {
+ free_irq(irq, &pdesc->work);
+ cancel_work_sync(&pdesc->work);
+ }
+
+ desc->flags &= ~GPIO_TRIGGER_MASK;
+
+ if (!gpio_flags) {
+ ret = 0;
+ goto free_sd;
+ }
+
+ irq_flags = IRQF_SHARED;
+ if (test_bit(FLAG_TRIG_FALL, &gpio_flags))
+ irq_flags |= IRQF_TRIGGER_FALLING;
+ if (test_bit(FLAG_TRIG_RISE, &gpio_flags))
+ irq_flags |= IRQF_TRIGGER_RISING;
+
+ if (!pdesc) {
+ pdesc = kmalloc(sizeof(struct poll_desc), GFP_KERNEL);
+ if (!pdesc) {
+ ret = -ENOMEM;
+ goto err_out;
+ }
+
+ do {
+ ret = -ENOMEM;
+ if (idr_pre_get(&pdesc_idr, GFP_KERNEL))
+ ret = idr_get_new_above(&pdesc_idr, pdesc, 1,
+ &id);
+ } while (ret == -EAGAIN);
+
+ if (ret)
+ goto free_mem;
+
+ desc->flags &= GPIO_FLAGS_MASK;
+ desc->flags |= (unsigned long)id << PDESC_ID_SHIFT;
+
+ if (desc->flags >> PDESC_ID_SHIFT != id) {
+ ret = -ERANGE;
+ goto free_id;
+ }
+
+ pdesc->value_sd = sysfs_get_dirent(dev->kobj.sd,
+ "value");
+ if (!pdesc->value_sd) {
+ ret = -ENODEV;
+ goto free_id;
+ }
+ INIT_WORK(&pdesc->work, gpio_notify_sysfs);
+ }
+
+ ret = request_irq(irq, gpio_sysfs_irq, irq_flags,
+ "gpiolib", &pdesc->work);
+ if (ret)
+ goto free_sd;
+
+ desc->flags |= gpio_flags;
+ return 0;
+
+free_sd:
+ sysfs_put(pdesc->value_sd);
+free_id:
+ idr_remove(&pdesc_idr, id);
+ desc->flags &= GPIO_FLAGS_MASK;
+free_mem:
+ kfree(pdesc);
+err_out:
+ return ret;
+}
+
+static const struct {
+ const char *name;
+ unsigned long flags;
+} trigger_types[] = {
+ { "none", 0 },
+ { "falling", BIT(FLAG_TRIG_FALL) },
+ { "rising", BIT(FLAG_TRIG_RISE) },
+ { "both", BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE) },
+};
+
+static ssize_t gpio_edge_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ const struct gpio_desc *desc = dev_get_drvdata(dev);
+ ssize_t status;
+
+ mutex_lock(&sysfs_lock);
+
+ if (!test_bit(FLAG_EXPORT, &desc->flags))
+ status = -EIO;
+ else {
+ int i;
+
+ status = 0;
+ for (i = 0; i < ARRAY_SIZE(trigger_types); i++)
+ if ((desc->flags & GPIO_TRIGGER_MASK)
+ == trigger_types[i].flags) {
+ status = sprintf(buf, "%s\n",
+ trigger_types[i].name);
+ break;
+ }
+ }
+
+ mutex_unlock(&sysfs_lock);
+ return status;
+}
+
+static ssize_t gpio_edge_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t size)
+{
+ struct gpio_desc *desc = dev_get_drvdata(dev);
+ ssize_t status;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(trigger_types); i++)
+ if (sysfs_streq(trigger_types[i].name, buf))
+ goto found;
+ return -EINVAL;
+
+found:
+ mutex_lock(&sysfs_lock);
+
+ if (!test_bit(FLAG_EXPORT, &desc->flags))
+ status = -EIO;
+ else {
+ status = gpio_setup_irq(desc, dev, trigger_types[i].flags);
+ if (!status)
+ status = size;
+ }
+
+ mutex_unlock(&sysfs_lock);
+
+ return status;
+}
+
+static DEVICE_ATTR(edge, 0644, gpio_edge_show, gpio_edge_store);
+
static const struct attribute *gpio_attrs[] = {
&dev_attr_direction.attr,
&dev_attr_value.attr,
@@ -481,6 +668,13 @@ int gpio_export(unsigned gpio, bool direction_may_change)
else
status = device_create_file(dev,
&dev_attr_value);
+
+ if (!status && gpio_to_irq(gpio) >= 0 &&
+ (direction_may_change ||
+ !test_bit(FLAG_IS_OUT, &desc->flags)))
+ status = device_create_file(dev,
+ &dev_attr_edge);
+
if (status != 0)
device_unregister(dev);
} else
@@ -527,6 +721,7 @@ void gpio_unexport(unsigned gpio)
dev = class_find_device(&gpio_class, NULL, desc, match_export);
if (dev) {
+ gpio_setup_irq(desc, dev, 0);
clear_bit(FLAG_EXPORT, &desc->flags);
put_device(dev);
device_unregister(dev);
@@ -611,6 +806,8 @@ static int __init gpiolib_sysfs_init(void)
unsigned long flags;
unsigned gpio;
+ idr_init(&pdesc_idr);
+
status = class_register(&gpio_class);
if (status < 0)
return status;
--
1.6.1.3