Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib

2012-11-30 Thread Grant Likely
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

2012-11-30 Thread Grant Likely
On Fri, 5 Oct 2012 14:20:55 +0200, Drasko DRASKOVIC 
drasko.drasko...@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.
--
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

2012-10-11 Thread Linus Walleij
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

2012-10-11 Thread Linus Walleij
On Tue, Oct 9, 2012 at 4:22 PM, Drasko DRASKOVIC
drasko.drasko...@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
--
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

2012-10-09 Thread Drasko DRASKOVIC
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

2012-10-09 Thread Russell King - ARM Linux
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

2012-10-09 Thread Linus Walleij
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

2012-10-09 Thread Linus Walleij
On Fri, Oct 5, 2012 at 3:16 PM, Drasko DRASKOVIC
drasko.drasko...@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
--
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

2012-10-09 Thread Russell King - ARM Linux
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.drasko...@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.
--
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

2012-10-09 Thread Drasko DRASKOVIC
On Tue, Oct 9, 2012 at 2:00 PM, Linus Walleij linus.wall...@linaro.org wrote:
 On Fri, Oct 5, 2012 at 3:16 PM, Drasko DRASKOVIC
 drasko.drasko...@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
--
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

2012-10-07 Thread Drasko DRASKOVIC
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

2012-10-07 Thread Drasko DRASKOVIC
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.drasko...@gmail.com wrote:
 On Fri, Oct 5, 2012 at 2:40 PM, Linus Walleij linus.wall...@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
--
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

2012-10-05 Thread Drasko DRASKOVIC
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

2012-10-05 Thread Linus Walleij
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

2012-10-05 Thread Drasko DRASKOVIC
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(_lock);
+
+   desc = _desc[gpio];
+
+   if (test_bit(FLAG_EXPORT, >flags)) {
+   dev = class_find_device(_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(_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-kernel" in
the body of a message to majord...@vger.kern

Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib

2012-10-05 Thread Drasko DRASKOVIC
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

2012-10-05 Thread Mark Brown
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

2012-10-05 Thread Drasko DRASKOVIC
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

2012-10-05 Thread Drasko DRASKOVIC
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

2012-10-05 Thread Drasko DRASKOVIC
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


Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib

2012-10-05 Thread Drasko DRASKOVIC
Looping GPIO maintainers...

On Fri, Oct 5, 2012 at 12:45 PM, Drasko DRASKOVIC
drasko.drasko...@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


0001-PATCH-GPIO-Add-IRQ-edge-setter-to-gpiolib.patch
Description: Binary data


Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib

2012-10-05 Thread Mark Brown
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

2012-10-05 Thread Drasko DRASKOVIC
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
broo...@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.
--
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

2012-10-05 Thread Drasko DRASKOVIC
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.drasko...@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.drasko...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel

Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib

2012-10-05 Thread Linus Walleij
On Fri, Oct 5, 2012 at 2:20 PM, Drasko DRASKOVIC
drasko.drasko...@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
--
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

2012-10-05 Thread Drasko DRASKOVIC
On Fri, Oct 5, 2012 at 2:40 PM, Linus Walleij linus.wall...@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
--
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/