Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
On Fri, 5 Oct 2012 14:20:55 +0200, Drasko DRASKOVIC 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
On Tue, Oct 9, 2012 at 4:22 PM, Drasko DRASKOVIC 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
On Tue, Oct 9, 2012 at 2:00 PM, Linus Walleij wrote: > On Fri, Oct 5, 2012 at 3:16 PM, Drasko DRASKOVIC > 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 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
On Tue, Oct 09, 2012 at 02:00:34PM +0200, Linus Walleij wrote: > On Fri, Oct 5, 2012 at 3:16 PM, Drasko DRASKOVIC > 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 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
On Fri, Oct 5, 2012 at 3:16 PM, Drasko DRASKOVIC 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
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 wrote: > On Fri, Oct 5, 2012 at 2:40 PM, Linus Walleij > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
On Fri, Oct 5, 2012 at 2:40 PM, Linus Walleij 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
On Fri, Oct 5, 2012 at 2:20 PM, Drasko DRASKOVIC 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH][GPIO] Add IRQ edge setter to gpiolib
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 Date: Fri, 5 Oct 2012 11:59:47 +0200 Subject: [PATCH] [PATCH][GPIO] Add IRQ edge setter to gpiolib Signed-off-by: Drasko DRASKOVIC --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-k
Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
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 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
Looping GPIO maintainers... On Fri, Oct 5, 2012 at 12:45 PM, Drasko DRASKOVIC 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 0001-PATCH-GPIO-Add-IRQ-edge-setter-to-gpiolib.patch Description: Binary data
[PATCH][GPIO] Add IRQ edge setter to gpiolib
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 0001-PATCH-GPIO-Add-IRQ-edge-setter-to-gpiolib.patch Description: Binary data