From: Drasko DRASKOVIC <drasko.draskovic@gmail.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>,
Grant Likely <grant.likely@secretlab.ca>,
Thomas Gleixner <tglx@linutronix.de>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH][GPIO] Add IRQ edge setter to gpiolib
Date: Tue, 9 Oct 2012 16:22:05 +0200 [thread overview]
Message-ID: <CAEk6gTD-ko5MXteB3DOTUkF1bJSj9Yga3wh0JTKD=D+YWjy6jQ@mail.gmail.com> (raw)
In-Reply-To: <CACRpkdb+txD9hfhs7pfnzmDuU59VXEdzJc86z=tdy2yqYBCqOg@mail.gmail.com>
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
next prev parent reply other threads:[~2012-10-09 14:22 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-10-05 12:20 [PATCH][GPIO] Add IRQ edge setter to gpiolib 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 [this message]
2012-10-11 16:26 ` Linus Walleij
2012-11-30 11:30 ` Grant Likely
-- strict thread matches above, loose matches on Subject: below --
2012-10-05 10:45 Drasko DRASKOVIC
2012-10-05 11:09 ` Drasko DRASKOVIC
2012-10-05 11:50 ` Mark Brown
2012-10-05 12:15 ` Drasko DRASKOVIC
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAEk6gTD-ko5MXteB3DOTUkF1bJSj9Yga3wh0JTKD=D+YWjy6jQ@mail.gmail.com' \
--to=drasko.draskovic@gmail.com \
--cc=grant.likely@secretlab.ca \
--cc=linus.walleij@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@arm.linux.org.uk \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).