linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH][GPIO] Add IRQ edge setter to gpiolib
@ 2012-10-05 10:45 Drasko DRASKOVIC
  2012-10-05 11:09 ` Drasko DRASKOVIC
  0 siblings, 1 reply; 13+ messages in thread
From: Drasko DRASKOVIC @ 2012-10-05 10:45 UTC (permalink / raw)
  To: linux-kernel

[-- Attachment #1: Type: text/plain, Size: 834 bytes --]

Hi all,
please find a patch that adds IRQ edge set-up mechanism to sysfs that
can be called from the kernel.

This functionality can be very useful for embedded systems, as it
permits kernel to do GPIO set-up during boot stage. Configuration
which defines pins behavior is often kept in NVRAM, and during boot
stage these structures can be parsed and executed by the kernel, so
that when user processes already find all sysfs environment ready and
correctly set-up.

While at the present it is possible to export GPIO pins to sysfs (and
correct direction / value), it is not possible to export IRQ
configuration as well, so this must be done in user space (most often
via command line). this patch implements missing functionality, so
that  gpio_sysfs_set_edge() function can be called directly from the
kernel.

Best regards,
Drasko

[-- Attachment #2: 0001-PATCH-GPIO-Add-IRQ-edge-setter-to-gpiolib.patch --]
[-- Type: application/octet-stream, Size: 3518 bytes --]

From eb07313ffb53b8ea080dbcc7400e0ec1a57c836e Mon Sep 17 00:00:00 2001
From: Drasko DRASKOVIC <drasko.draskovic@gmail.com>
Date: Fri, 5 Oct 2012 11:59:47 +0200
Subject: [PATCH] [PATCH][GPIO] Add IRQ edge setter to gpiolib Signed-off-by:
 Drasko DRASKOVIC <drasko.draskovic@gmail.com>

---
 Documentation/gpio.txt     |    3 ++
 drivers/gpio/gpiolib.c     |   70 ++++++++++++++++++++++++++++++++++++++++++++
 include/asm-generic/gpio.h |    5 +++
 3 files changed, 78 insertions(+), 0 deletions(-)

diff --git a/Documentation/gpio.txt b/Documentation/gpio.txt
index e08a883..8db95e1 100644
--- a/Documentation/gpio.txt
+++ b/Documentation/gpio.txt
@@ -712,6 +712,9 @@ requested using gpio_request():
 	/* change the polarity of a GPIO node in sysfs */
 	int gpio_sysfs_set_active_low(unsigned gpio, int value);
 
+	/* change the irq edge of a GPIO node in sysfs */
+	gpio_sysfs_set_edge(unsigned gpio, unsigned int irq_type);
+
 After a kernel driver requests a GPIO, it may only be made available in
 the sysfs interface by gpio_export().  The driver can control whether the
 signal direction may change.  This helps drivers prevent userspace code
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5d6c71e..9b07d67 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -820,6 +820,76 @@ EXPORT_SYMBOL_GPL(gpio_export_link);
 
 
 /**
+ * gpio_sysfs_set_edge - sets irq edge type for an exported GPIO node
+ * @gpio: gpio to set-up edge to, already exported
+ * @irq_type: irq type to be set
+ *
+ * Returns zero on success, else an error.
+ */
+int gpio_sysfs_set_edge(unsigned gpio, unsigned int irq_type)
+{
+	struct gpio_desc        *desc;
+	struct device           *dev = NULL;
+	int                     status = -EINVAL;
+	unsigned long           trigger_flags = 0;
+
+	if (!gpio_is_valid(gpio))
+		goto done;
+
+	mutex_lock(&sysfs_lock);
+
+	desc = &gpio_desc[gpio];
+
+	if (test_bit(FLAG_EXPORT, &desc->flags)) {
+		dev = class_find_device(&gpio_class, NULL, desc, match_export);
+		if (dev == NULL) {
+			status = -ENODEV;
+			goto unlock;
+		}
+	}
+
+	switch (irq_type) {
+		case IRQ_TYPE_NONE:
+			trigger_flags = 0;
+			break;
+		case IRQ_TYPE_EDGE_FALLING:
+			trigger_flags = BIT(FLAG_TRIG_FALL);
+			break;
+		case IRQ_TYPE_EDGE_RISING:
+			trigger_flags = BIT(FLAG_TRIG_RISE);
+			break;
+		case IRQ_TYPE_EDGE_BOTH:
+			trigger_flags = BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE);
+			break;
+		case IRQ_TYPE_LEVEL_HIGH:
+			trigger_flags = 0;
+			break;
+		case IRQ_TYPE_LEVEL_LOW:
+			trigger_flags = 0;
+			break;
+		default:
+			trigger_flags = 0;
+			break;
+	}
+
+	gpio_setup_irq(desc, dev, 0);
+	status = gpio_setup_irq(desc, dev, trigger_flags);
+
+unlock:
+    mutex_unlock(&sysfs_lock);
+
+done:
+    if (status)
+        pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
+
+    return status;
+}
+EXPORT_SYMBOL_GPL(gpio_sysfs_set_edge);
+
+
+
+
+/**
  * gpio_sysfs_set_active_low - set the polarity of gpio sysfs value
  * @gpio: gpio to change
  * @value: non-zero to use active low, i.e. inverted values
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index a9432fc..4827de8 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -252,6 +252,11 @@ static inline int gpio_sysfs_set_active_low(unsigned gpio, int value)
 	return -ENOSYS;
 }
 
+static inline int gpio_sysfs_set_edge(unsigned gpio, unsigned int irq_type)
+{
+	return -ENOSYS;
+}
+
 static inline void gpio_unexport(unsigned gpio)
 {
 }
-- 
1.7.6


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
  2012-10-05 10:45 [PATCH][GPIO] Add IRQ edge setter to gpiolib Drasko DRASKOVIC
@ 2012-10-05 11:09 ` Drasko DRASKOVIC
  2012-10-05 11:50   ` Mark Brown
  0 siblings, 1 reply; 13+ messages in thread
From: Drasko DRASKOVIC @ 2012-10-05 11:09 UTC (permalink / raw)
  To: linux-kernel, grant.likely, linus.walleij

[-- Attachment #1: Type: text/plain, Size: 986 bytes --]

Looping GPIO maintainers...

On Fri, Oct 5, 2012 at 12:45 PM, Drasko DRASKOVIC
<drasko.draskovic@gmail.com> wrote:
> Hi all,
> please find a patch that adds IRQ edge set-up mechanism to sysfs that
> can be called from the kernel.
>
> This functionality can be very useful for embedded systems, as it
> permits kernel to do GPIO set-up during boot stage. Configuration
> which defines pins behavior is often kept in NVRAM, and during boot
> stage these structures can be parsed and executed by the kernel, so
> that when user processes already find all sysfs environment ready and
> correctly set-up.
>
> While at the present it is possible to export GPIO pins to sysfs (and
> correct direction / value), it is not possible to export IRQ
> configuration as well, so this must be done in user space (most often
> via command line). this patch implements missing functionality, so
> that  gpio_sysfs_set_edge() function can be called directly from the
> kernel.
>
> Best regards,
> Drasko

[-- Attachment #2: 0001-PATCH-GPIO-Add-IRQ-edge-setter-to-gpiolib.patch --]
[-- Type: application/octet-stream, Size: 3518 bytes --]

From eb07313ffb53b8ea080dbcc7400e0ec1a57c836e Mon Sep 17 00:00:00 2001
From: Drasko DRASKOVIC <drasko.draskovic@gmail.com>
Date: Fri, 5 Oct 2012 11:59:47 +0200
Subject: [PATCH] [PATCH][GPIO] Add IRQ edge setter to gpiolib Signed-off-by:
 Drasko DRASKOVIC <drasko.draskovic@gmail.com>

---
 Documentation/gpio.txt     |    3 ++
 drivers/gpio/gpiolib.c     |   70 ++++++++++++++++++++++++++++++++++++++++++++
 include/asm-generic/gpio.h |    5 +++
 3 files changed, 78 insertions(+), 0 deletions(-)

diff --git a/Documentation/gpio.txt b/Documentation/gpio.txt
index e08a883..8db95e1 100644
--- a/Documentation/gpio.txt
+++ b/Documentation/gpio.txt
@@ -712,6 +712,9 @@ requested using gpio_request():
 	/* change the polarity of a GPIO node in sysfs */
 	int gpio_sysfs_set_active_low(unsigned gpio, int value);
 
+	/* change the irq edge of a GPIO node in sysfs */
+	gpio_sysfs_set_edge(unsigned gpio, unsigned int irq_type);
+
 After a kernel driver requests a GPIO, it may only be made available in
 the sysfs interface by gpio_export().  The driver can control whether the
 signal direction may change.  This helps drivers prevent userspace code
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5d6c71e..9b07d67 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -820,6 +820,76 @@ EXPORT_SYMBOL_GPL(gpio_export_link);
 
 
 /**
+ * gpio_sysfs_set_edge - sets irq edge type for an exported GPIO node
+ * @gpio: gpio to set-up edge to, already exported
+ * @irq_type: irq type to be set
+ *
+ * Returns zero on success, else an error.
+ */
+int gpio_sysfs_set_edge(unsigned gpio, unsigned int irq_type)
+{
+	struct gpio_desc        *desc;
+	struct device           *dev = NULL;
+	int                     status = -EINVAL;
+	unsigned long           trigger_flags = 0;
+
+	if (!gpio_is_valid(gpio))
+		goto done;
+
+	mutex_lock(&sysfs_lock);
+
+	desc = &gpio_desc[gpio];
+
+	if (test_bit(FLAG_EXPORT, &desc->flags)) {
+		dev = class_find_device(&gpio_class, NULL, desc, match_export);
+		if (dev == NULL) {
+			status = -ENODEV;
+			goto unlock;
+		}
+	}
+
+	switch (irq_type) {
+		case IRQ_TYPE_NONE:
+			trigger_flags = 0;
+			break;
+		case IRQ_TYPE_EDGE_FALLING:
+			trigger_flags = BIT(FLAG_TRIG_FALL);
+			break;
+		case IRQ_TYPE_EDGE_RISING:
+			trigger_flags = BIT(FLAG_TRIG_RISE);
+			break;
+		case IRQ_TYPE_EDGE_BOTH:
+			trigger_flags = BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE);
+			break;
+		case IRQ_TYPE_LEVEL_HIGH:
+			trigger_flags = 0;
+			break;
+		case IRQ_TYPE_LEVEL_LOW:
+			trigger_flags = 0;
+			break;
+		default:
+			trigger_flags = 0;
+			break;
+	}
+
+	gpio_setup_irq(desc, dev, 0);
+	status = gpio_setup_irq(desc, dev, trigger_flags);
+
+unlock:
+    mutex_unlock(&sysfs_lock);
+
+done:
+    if (status)
+        pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
+
+    return status;
+}
+EXPORT_SYMBOL_GPL(gpio_sysfs_set_edge);
+
+
+
+
+/**
  * gpio_sysfs_set_active_low - set the polarity of gpio sysfs value
  * @gpio: gpio to change
  * @value: non-zero to use active low, i.e. inverted values
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index a9432fc..4827de8 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -252,6 +252,11 @@ static inline int gpio_sysfs_set_active_low(unsigned gpio, int value)
 	return -ENOSYS;
 }
 
+static inline int gpio_sysfs_set_edge(unsigned gpio, unsigned int irq_type)
+{
+	return -ENOSYS;
+}
+
 static inline void gpio_unexport(unsigned gpio)
 {
 }
-- 
1.7.6


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
  2012-10-05 11:09 ` Drasko DRASKOVIC
@ 2012-10-05 11:50   ` Mark Brown
  2012-10-05 12:15     ` Drasko DRASKOVIC
  0 siblings, 1 reply; 13+ messages in thread
From: Mark Brown @ 2012-10-05 11:50 UTC (permalink / raw)
  To: Drasko DRASKOVIC; +Cc: linux-kernel, grant.likely, linus.walleij

On Fri, Oct 05, 2012 at 01:09:13PM +0200, Drasko DRASKOVIC wrote:
> Looping GPIO maintainers...

Please follow the patch submission process in
Documentation/SubmittingPatches - the main thing here is to not send the
patch as an attachment.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
  2012-10-05 11:50   ` Mark Brown
@ 2012-10-05 12:15     ` Drasko DRASKOVIC
  0 siblings, 0 replies; 13+ messages in thread
From: Drasko DRASKOVIC @ 2012-10-05 12:15 UTC (permalink / raw)
  To: Mark Brown; +Cc: linux-kernel, grant.likely, linus.walleij

Hi Mark,
thanks.

I'll re-send it as a plain text in a separate e-mail.

BR,
Drasko

On Fri, Oct 5, 2012 at 1:50 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, Oct 05, 2012 at 01:09:13PM +0200, Drasko DRASKOVIC wrote:
>> Looping GPIO maintainers...
>
> Please follow the patch submission process in
> Documentation/SubmittingPatches - the main thing here is to not send the
> patch as an attachment.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
  2012-10-05 12:20 Drasko DRASKOVIC
  2012-10-05 12:40 ` Linus Walleij
@ 2012-11-30 11:30 ` Grant Likely
  1 sibling, 0 replies; 13+ messages in thread
From: Grant Likely @ 2012-11-30 11:30 UTC (permalink / raw)
  To: Drasko DRASKOVIC, linux-kernel; +Cc: linus.walleij

On Fri, 5 Oct 2012 14:20:55 +0200, Drasko DRASKOVIC <drasko.draskovic@gmail.com> wrote:
> Hi all,
> please find a patch that adds IRQ edge set-up mechanism to sysfs that
> can be called from the kernel.
> 
> This functionality can be very useful for embedded systems, as it
> permits kernel to do GPIO set-up during boot stage. Configuration
> which defines pins behavior is often kept in NVRAM, and during boot
> stage these structures can be parsed and executed by the kernel, so
> that when user processes already find all sysfs environment ready and
> correctly set-up.
> 
> While at the present it is possible to export GPIO pins to sysfs (and
> correct direction / value), it is not possible to export IRQ
> configuration as well, so this must be done in user space (most often
> via command line). this patch implements missing functionality, so
> that  gpio_sysfs_set_edge() function can be called directly from the
> kernel.

This really seems like the wrong place to be doing this. If the GPIO
needs to be used with a particular configuration for an IRQ, then the
device tree or platform data on non-dt should be given that information.
I do not feel good about exporting anonymnous IRQ configuration to
userspace.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
  2012-10-09 14:22       ` Drasko DRASKOVIC
@ 2012-10-11 16:26         ` Linus Walleij
  0 siblings, 0 replies; 13+ messages in thread
From: Linus Walleij @ 2012-10-11 16:26 UTC (permalink / raw)
  To: Drasko DRASKOVIC
  Cc: Russell King - ARM Linux, Grant Likely, Thomas Gleixner, linux-kernel

On Tue, Oct 9, 2012 at 4:22 PM, Drasko DRASKOVIC
<drasko.draskovic@gmail.com> wrote:
> [Me]
>> So can you explain exactly why userspace want to configure
>> GPIO pins in interrupt mode, when there is no way whatsoever
>> for userspace to handle these IRQs?
>
> Maybe I understand something wrong, but explicit configuration of GPIO
> interrupt trigger type visible in sysfs is possible __only__ from the
> userspace today, and that is exactly limitation I am addressing.

So this is the real problem is it not?

So if we consider this:

static irqreturn_t my_callback(int irq, void *dev_id)
{
        return IRQ_HANDLED;
}

int my_gpio, my_irq, ret;

my_gpio = 17; /* For some reason I like this pin */
ret = gpio_request(my_gpio);
ret = gpio_direction_input(my_gpio);
my_irq = gpio_to_irq(my_gpio);
ret = request_any_context_irq(my_irq,
    my_callback,
    IRQF_TRIGGER_FALLING,
    "my gpio thing");

(some error handling removed, based on drivers/mmc/host/mmci.c)

What is wrong with this picture? Do you mean that the problem is
that the trigger type I just set up for the IRQ connected to the pin
is not reflected in GPIO sysfs?

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
  2012-10-09 12:00     ` Linus Walleij
  2012-10-09 12:27       ` Russell King - ARM Linux
@ 2012-10-09 14:22       ` Drasko DRASKOVIC
  2012-10-11 16:26         ` Linus Walleij
  1 sibling, 1 reply; 13+ messages in thread
From: Drasko DRASKOVIC @ 2012-10-09 14:22 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Russell King - ARM Linux, Grant Likely, Thomas Gleixner, linux-kernel

On Tue, Oct 9, 2012 at 2:00 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Oct 5, 2012 at 3:16 PM, Drasko DRASKOVIC
> <drasko.draskovic@gmail.com> wrote:
>> [Me]
>>> If I understand correctly the below more or less exports
>>> struct irq_chip to userspace,
>>> trying to hide it by instead exposing a property of the
>>> containing struct gpio_chip and it worries me.
>>
>> No, it should not.
>
> You are exporting all of the defines from irq.h,
> IRQ_TYPE_NONE, IRQ_TYPE_EDGE_FALLING, etc
> to userspace. These are defined in <linux/irq.h> and that file
> has this comment on top:
>
> /*
>  * Please do not include this file in generic code.  There is currently
>  * no requirement for any architecture to implement anything held
>  * within this file.
>  *
>  * Thanks. --rmk
>  */
> And that comment is even only about generic *KERNEL* code,
> userspace is way, way more than that.

Yes, this seem to be true... I was looking for something similar to
gpio_edge_store(), but it is indeed static device attribute.

Would it then be more appropriate to pass char buffer as a param, and
"descriptively" precise the edge type ("rising", "falling", "both"),
as it is now done with GPIO device from command line ?
Or there can be some more elegant way to pass this information without
the need to include <linux/irq.h> or any other kernel's internal
structs?

One approach can be to add set_irq_type fnc pointer as a member to
gpio_chip structure... Gpiolib can then call directly this callback
upon chip export.

>
>> It operates only on already exported gpiochip
>> (similar to gpio_export_link()).
>> It just helps exported GPIO be configured in "interrupt" and not in
>> "normal" mode.
>
> So can you explain exactly why userspace want to configure
> GPIO pins in interrupt mode, when there is no way whatsoever
> for userspace to handle these IRQs?

Maybe I understand something wrong, but explicit configuration of GPIO
interrupt trigger type visible in sysfs is possible __only__ from the
userspace today, and that is exactly limitation I am addressing.

The only way known to me to set-up for example GPIO_X's interrupt
trigger edge to "rising" is something like this :
> echo "rising" > /sys/class/gpio/gpioX/edge
but kernel obviously can not (should not, at least) R/W a file.

To clarify, of course that kernel module can always call internal
.set_type callback of static-to-module irq_chip. However, this kind of
set-up will not at all be visible in userspace sysfs device attribute
"edge", which can even be not aligned to real HW set-up by the module.
I.e. we can imagine that kernel module sets up IRQ edge to "rising",
and sys (after creation) will say that edge is "none" - because it has
to be explicitly changed from userspace.

It is because sysfs' gpiolib holds edge information internally (within
gpio_desc.flags, static to gpiolib.c) , and can be discorelation
between what is really set-up by the driver in the background. Usually
they are aligned, as one will set-up edge type via command line (or
userspace program), and then gpiolib updated flags and calls driver's
set_type callback.

However, when driver module sets-up edge on it's own behalf, this
change is not updated in gpiolib, and upon boot we can end up with HW
adn IRQ that has "rising" edge type, but "cat"-ing
/sys/class/gpio/gpioX/edge would give "none".


So, finally - either we pass via gpiolib to set-up sysfs visible IRQ
edge type, or give kernel update gpiolib's internal flags (vey bad).

I hope this clarifies a little my motivation of defining IRQ edge type
via sysfs from kernel.

BR,
Drasko

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
  2012-10-09 12:00     ` Linus Walleij
@ 2012-10-09 12:27       ` Russell King - ARM Linux
  2012-10-09 14:22       ` Drasko DRASKOVIC
  1 sibling, 0 replies; 13+ messages in thread
From: Russell King - ARM Linux @ 2012-10-09 12:27 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Drasko DRASKOVIC, Grant Likely, Thomas Gleixner, linux-kernel

On Tue, Oct 09, 2012 at 02:00:34PM +0200, Linus Walleij wrote:
> On Fri, Oct 5, 2012 at 3:16 PM, Drasko DRASKOVIC
> <drasko.draskovic@gmail.com> wrote:
> > [Me]
> >> If I understand correctly the below more or less exports
> >> struct irq_chip to userspace,
> >> trying to hide it by instead exposing a property of the
> >> containing struct gpio_chip and it worries me.
> >
> > No, it should not.
> 
> You are exporting all of the defines from irq.h,
> IRQ_TYPE_NONE, IRQ_TYPE_EDGE_FALLING, etc
> to userspace. These are defined in <linux/irq.h> and that file
> has this comment on top:
> 
> /*
>  * Please do not include this file in generic code.  There is currently
>  * no requirement for any architecture to implement anything held
>  * within this file.
>  *
>  * Thanks. --rmk
>  */
> 
> And that comment is even only about generic *KERNEL* code,
> userspace is way, way more than that.

It is always worth remembering this simple fact:

  If you export something from the kernel to userspace, it becomes part
  of the kernel's userspace API.  Userspace APIs are more stable than
  the kernel, some suggest that the userspace API should be maintained
  for 10 years.  Are you willing to set in stone for years part of the
  kernel internal structures without putting a lot of thought into how
  you export the data?

If you haven't spent a significant amount of time thinking about how you're
going to export something - and thinking about whether it should even be
exported, then you haven't done enough to outweigh the pain of having it
exported.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
  2012-10-05 13:16   ` Drasko DRASKOVIC
  2012-10-07 23:47     ` Drasko DRASKOVIC
@ 2012-10-09 12:00     ` Linus Walleij
  2012-10-09 12:27       ` Russell King - ARM Linux
  2012-10-09 14:22       ` Drasko DRASKOVIC
  1 sibling, 2 replies; 13+ messages in thread
From: Linus Walleij @ 2012-10-09 12:00 UTC (permalink / raw)
  To: Drasko DRASKOVIC, Russell King - ARM Linux
  Cc: Grant Likely, Thomas Gleixner, linux-kernel

On Fri, Oct 5, 2012 at 3:16 PM, Drasko DRASKOVIC
<drasko.draskovic@gmail.com> wrote:
> [Me]
>> If I understand correctly the below more or less exports
>> struct irq_chip to userspace,
>> trying to hide it by instead exposing a property of the
>> containing struct gpio_chip and it worries me.
>
> No, it should not.

You are exporting all of the defines from irq.h,
IRQ_TYPE_NONE, IRQ_TYPE_EDGE_FALLING, etc
to userspace. These are defined in <linux/irq.h> and that file
has this comment on top:

/*
 * Please do not include this file in generic code.  There is currently
 * no requirement for any architecture to implement anything held
 * within this file.
 *
 * Thanks. --rmk
 */

And that comment is even only about generic *KERNEL* code,
userspace is way, way more than that.

> It operates only on already exported gpiochip
> (similar to gpio_export_link()).
> It just helps exported GPIO be configured in "interrupt" and not in
> "normal" mode.

So can you explain exactly why userspace want to configure
GPIO pins in interrupt mode, when there is no way whatsoever
for userspace to handle these IRQs?

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
  2012-10-05 13:16   ` Drasko DRASKOVIC
@ 2012-10-07 23:47     ` Drasko DRASKOVIC
  2012-10-09 12:00     ` Linus Walleij
  1 sibling, 0 replies; 13+ messages in thread
From: Drasko DRASKOVIC @ 2012-10-07 23:47 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Grant Likely, Thomas Gleixner, linux-kernel

Hi Grant, Thomas,
do you see any potential problems with this patch?

It adds sysfs interface to change / set-up trigger edge from the
kernel (currently not possible).

BR,
Drasko


On Fri, Oct 5, 2012 at 3:16 PM, Drasko DRASKOVIC
<drasko.draskovic@gmail.com> wrote:
> On Fri, Oct 5, 2012 at 2:40 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Fri, Oct 5, 2012 at 2:20 PM, Drasko DRASKOVIC
>> Why not put the above text into the patch commit blurb?
>
> OK, I can add this easily. Makes sense.
>
>
>> If I understand correctly the below more or less exports
>> struct irq_chip to userspace,
>> trying to hide it by instead exposing a property of the
>> containing struct gpio_chip and it worries me.
>
> No, it should not. It operates only on already exported gpiochip
> (similar to gpio_export_link()).
> It just helps exported GPIO be configured in "interrupt" and not in
> "normal" mode.
>
>>
>> One possible comment is that you shouldn't
>> add this to gpiolib, if you want to mess around with the
>> irq_chip you should create sysfs entries for the irq_chip
>> per se, then we can have a symbolic link from the
>> gpio_chip to its irq_chip in sysfs, and you can follow that
>> to change trigger flanks, right?
>
> I did not found any correlation between kernel space irq_chip and
> gpiolib's internal stuctures describing interrupt.
>
> This patch implementation seems like reasonable solution to me, but
> still leaves possibility for someone to configure GPIO interrupt mode
> internally (within driver) different than it is declared lately to
> sysfs by calling my function...
>
> Otherwise, a pointer to an edge set-up kernel function can be added to
> the gpio_chip structure, but I think it will be slightly more
> complicated, and will fall back to the same thing.
>
>>
>> Part of me doesn't like it when userspace is messing
>> around with this kind of thing. Why can't this be set
>> up from the kernel itself by some jam table?
>>
>> I can understand it if this is some lab board with GPIO
>> or so, if it's some embedded GPIO controller within
>> a laptop or something it surely should be in kernelspace.
>
> Yes, it is intended for embedded devices, where most (if not all) of
> GPIO configuration should be done by the kernel, but also this
> configuration should be appropriately exported to sysfs, so that
> user-space applications could start using it right after the boot.
>
>
>> So please detail your usecase a bit... what is the code
>> daemon etc in userspace poking around at these pins?
>> What is is doing and why?
>
> Right now, sysfs exposes interface like this for GPIO IRQ type configuration :
>
> # cat /sys/class/gpio/gpioX/edge
> none
> # echo rising > /sys/class/gpio/gpioX/edge
>
> This example puts GPIO pin X into "interrupt" mode, and declares it's
> "type" i.e. that it triggers on rising edge.
>
> In the world of embedded, system configuratio which tells which GPIO
> pins should be configured in "interrupt" mode (and what is their
> trigger type) is kept in some format usually known only to the kernel
> driver. We have no need to export this format to the user space, so
> that rc scripts for example would know to parse GPIO configuration and
> execute operations I mentioned above.
>
> To avoid that this is done from the user space, a function accesible
> to kernel is needed.
>
> BR,
> Drasko

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
  2012-10-05 12:40 ` Linus Walleij
@ 2012-10-05 13:16   ` Drasko DRASKOVIC
  2012-10-07 23:47     ` Drasko DRASKOVIC
  2012-10-09 12:00     ` Linus Walleij
  0 siblings, 2 replies; 13+ messages in thread
From: Drasko DRASKOVIC @ 2012-10-05 13:16 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Grant Likely, Thomas Gleixner, linux-kernel

On Fri, Oct 5, 2012 at 2:40 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Oct 5, 2012 at 2:20 PM, Drasko DRASKOVIC
> Why not put the above text into the patch commit blurb?

OK, I can add this easily. Makes sense.


> If I understand correctly the below more or less exports
> struct irq_chip to userspace,
> trying to hide it by instead exposing a property of the
> containing struct gpio_chip and it worries me.

No, it should not. It operates only on already exported gpiochip
(similar to gpio_export_link()).
It just helps exported GPIO be configured in "interrupt" and not in
"normal" mode.

>
> One possible comment is that you shouldn't
> add this to gpiolib, if you want to mess around with the
> irq_chip you should create sysfs entries for the irq_chip
> per se, then we can have a symbolic link from the
> gpio_chip to its irq_chip in sysfs, and you can follow that
> to change trigger flanks, right?

I did not found any correlation between kernel space irq_chip and
gpiolib's internal stuctures describing interrupt.

This patch implementation seems like reasonable solution to me, but
still leaves possibility for someone to configure GPIO interrupt mode
internally (within driver) different than it is declared lately to
sysfs by calling my function...

Otherwise, a pointer to an edge set-up kernel function can be added to
the gpio_chip structure, but I think it will be slightly more
complicated, and will fall back to the same thing.

>
> Part of me doesn't like it when userspace is messing
> around with this kind of thing. Why can't this be set
> up from the kernel itself by some jam table?
>
> I can understand it if this is some lab board with GPIO
> or so, if it's some embedded GPIO controller within
> a laptop or something it surely should be in kernelspace.

Yes, it is intended for embedded devices, where most (if not all) of
GPIO configuration should be done by the kernel, but also this
configuration should be appropriately exported to sysfs, so that
user-space applications could start using it right after the boot.


> So please detail your usecase a bit... what is the code
> daemon etc in userspace poking around at these pins?
> What is is doing and why?

Right now, sysfs exposes interface like this for GPIO IRQ type configuration :

# cat /sys/class/gpio/gpioX/edge
none
# echo rising > /sys/class/gpio/gpioX/edge

This example puts GPIO pin X into "interrupt" mode, and declares it's
"type" i.e. that it triggers on rising edge.

In the world of embedded, system configuratio which tells which GPIO
pins should be configured in "interrupt" mode (and what is their
trigger type) is kept in some format usually known only to the kernel
driver. We have no need to export this format to the user space, so
that rc scripts for example would know to parse GPIO configuration and
execute operations I mentioned above.

To avoid that this is done from the user space, a function accesible
to kernel is needed.

BR,
Drasko

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
  2012-10-05 12:20 Drasko DRASKOVIC
@ 2012-10-05 12:40 ` Linus Walleij
  2012-10-05 13:16   ` Drasko DRASKOVIC
  2012-11-30 11:30 ` Grant Likely
  1 sibling, 1 reply; 13+ messages in thread
From: Linus Walleij @ 2012-10-05 12:40 UTC (permalink / raw)
  To: Drasko DRASKOVIC, Grant Likely, Thomas Gleixner; +Cc: linux-kernel

On Fri, Oct 5, 2012 at 2:20 PM, Drasko DRASKOVIC
<drasko.draskovic@gmail.com> wrote:

> please find a patch that adds IRQ edge set-up mechanism to sysfs that
> can be called from the kernel.
>
> This functionality can be very useful for embedded systems, as it
> permits kernel to do GPIO set-up during boot stage. Configuration
> which defines pins behavior is often kept in NVRAM, and during boot
> stage these structures can be parsed and executed by the kernel, so
> that when user processes already find all sysfs environment ready and
> correctly set-up.
>
> While at the present it is possible to export GPIO pins to sysfs (and
> correct direction / value), it is not possible to export IRQ
> configuration as well, so this must be done in user space (most often
> via command line). this patch implements missing functionality, so
> that  gpio_sysfs_set_edge() function can be called directly from the
> kernel.

Why not put the above text into the patch commit blurb?

I really won't touch this unless I get a comment from Grant
and/or Thomas Gleixner about it.

The common GPIO sysfs is hairy enough as it is, and
not universally liked.

If I understand correctly the below more or less exports
struct irq_chip to userspace,
trying to hide it by instead exposing a property of the
containing struct gpio_chip and it worries me.

One possible comment is that you shouldn't
add this to gpiolib, if you want to mess around with the
irq_chip you should create sysfs entries for the irq_chip
per se, then we can have a symbolic link from the
gpio_chip to its irq_chip in sysfs, and you can follow that
to change trigger flanks, right?

Part of me doesn't like it when userspace is messing
around with this kind of thing. Why can't this be set
up from the kernel itself by some jam table?

I can understand it if this is some lab board with GPIO
or so, if it's some embedded GPIO controller within
a laptop or something it surely should be in kernelspace.

So please detail your usecase a bit... what is the code
daemon etc in userspace poking around at these pins?
What is is doing and why?

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH][GPIO] Add IRQ edge setter to gpiolib
@ 2012-10-05 12:20 Drasko DRASKOVIC
  2012-10-05 12:40 ` Linus Walleij
  2012-11-30 11:30 ` Grant Likely
  0 siblings, 2 replies; 13+ messages in thread
From: Drasko DRASKOVIC @ 2012-10-05 12:20 UTC (permalink / raw)
  To: linux-kernel; +Cc: grant.likely, linus.walleij

Hi all,
please find a patch that adds IRQ edge set-up mechanism to sysfs that
can be called from the kernel.

This functionality can be very useful for embedded systems, as it
permits kernel to do GPIO set-up during boot stage. Configuration
which defines pins behavior is often kept in NVRAM, and during boot
stage these structures can be parsed and executed by the kernel, so
that when user processes already find all sysfs environment ready and
correctly set-up.

While at the present it is possible to export GPIO pins to sysfs (and
correct direction / value), it is not possible to export IRQ
configuration as well, so this must be done in user space (most often
via command line). this patch implements missing functionality, so
that  gpio_sysfs_set_edge() function can be called directly from the
kernel.

Best regards,
Drasko


---


>From eb07313ffb53b8ea080dbcc7400e0ec1a57c836e Mon Sep 17 00:00:00 2001
From: Drasko DRASKOVIC <drasko.draskovic@gmail.com>
Date: Fri, 5 Oct 2012 11:59:47 +0200
Subject: [PATCH] [PATCH][GPIO] Add IRQ edge setter to gpiolib Signed-off-by:
 Drasko DRASKOVIC <drasko.draskovic@gmail.com>

---
 Documentation/gpio.txt     |    3 ++
 drivers/gpio/gpiolib.c     |   70 ++++++++++++++++++++++++++++++++++++++++++++
 include/asm-generic/gpio.h |    5 +++
 3 files changed, 78 insertions(+), 0 deletions(-)

diff --git a/Documentation/gpio.txt b/Documentation/gpio.txt
index e08a883..8db95e1 100644
--- a/Documentation/gpio.txt
+++ b/Documentation/gpio.txt
@@ -712,6 +712,9 @@ requested using gpio_request():
 	/* change the polarity of a GPIO node in sysfs */
 	int gpio_sysfs_set_active_low(unsigned gpio, int value);

+	/* change the irq edge of a GPIO node in sysfs */
+	gpio_sysfs_set_edge(unsigned gpio, unsigned int irq_type);
+
 After a kernel driver requests a GPIO, it may only be made available in
 the sysfs interface by gpio_export().  The driver can control whether the
 signal direction may change.  This helps drivers prevent userspace code
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5d6c71e..9b07d67 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -820,6 +820,76 @@ EXPORT_SYMBOL_GPL(gpio_export_link);


 /**
+ * gpio_sysfs_set_edge - sets irq edge type for an exported GPIO node
+ * @gpio: gpio to set-up edge to, already exported
+ * @irq_type: irq type to be set
+ *
+ * Returns zero on success, else an error.
+ */
+int gpio_sysfs_set_edge(unsigned gpio, unsigned int irq_type)
+{
+	struct gpio_desc        *desc;
+	struct device           *dev = NULL;
+	int                     status = -EINVAL;
+	unsigned long           trigger_flags = 0;
+
+	if (!gpio_is_valid(gpio))
+		goto done;
+
+	mutex_lock(&sysfs_lock);
+
+	desc = &gpio_desc[gpio];
+
+	if (test_bit(FLAG_EXPORT, &desc->flags)) {
+		dev = class_find_device(&gpio_class, NULL, desc, match_export);
+		if (dev == NULL) {
+			status = -ENODEV;
+			goto unlock;
+		}
+	}
+
+	switch (irq_type) {
+		case IRQ_TYPE_NONE:
+			trigger_flags = 0;
+			break;
+		case IRQ_TYPE_EDGE_FALLING:
+			trigger_flags = BIT(FLAG_TRIG_FALL);
+			break;
+		case IRQ_TYPE_EDGE_RISING:
+			trigger_flags = BIT(FLAG_TRIG_RISE);
+			break;
+		case IRQ_TYPE_EDGE_BOTH:
+			trigger_flags = BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE);
+			break;
+		case IRQ_TYPE_LEVEL_HIGH:
+			trigger_flags = 0;
+			break;
+		case IRQ_TYPE_LEVEL_LOW:
+			trigger_flags = 0;
+			break;
+		default:
+			trigger_flags = 0;
+			break;
+	}
+
+	gpio_setup_irq(desc, dev, 0);
+	status = gpio_setup_irq(desc, dev, trigger_flags);
+
+unlock:
+    mutex_unlock(&sysfs_lock);
+
+done:
+    if (status)
+        pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
+
+    return status;
+}
+EXPORT_SYMBOL_GPL(gpio_sysfs_set_edge);
+
+
+
+
+/**
  * gpio_sysfs_set_active_low - set the polarity of gpio sysfs value
  * @gpio: gpio to change
  * @value: non-zero to use active low, i.e. inverted values
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index a9432fc..4827de8 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -252,6 +252,11 @@ static inline int
gpio_sysfs_set_active_low(unsigned gpio, int value)
 	return -ENOSYS;
 }

+static inline int gpio_sysfs_set_edge(unsigned gpio, unsigned int irq_type)
+{
+	return -ENOSYS;
+}
+
 static inline void gpio_unexport(unsigned gpio)
 {
 }
-- 
1.7.6

^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2012-11-30 11:30 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-05 10:45 [PATCH][GPIO] Add IRQ edge setter to gpiolib Drasko DRASKOVIC
2012-10-05 11:09 ` Drasko DRASKOVIC
2012-10-05 11:50   ` Mark Brown
2012-10-05 12:15     ` Drasko DRASKOVIC
2012-10-05 12:20 Drasko DRASKOVIC
2012-10-05 12:40 ` Linus Walleij
2012-10-05 13:16   ` Drasko DRASKOVIC
2012-10-07 23:47     ` Drasko DRASKOVIC
2012-10-09 12:00     ` Linus Walleij
2012-10-09 12:27       ` Russell King - ARM Linux
2012-10-09 14:22       ` Drasko DRASKOVIC
2012-10-11 16:26         ` Linus Walleij
2012-11-30 11:30 ` Grant Likely

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).