Re: [PATCH] gpiolib: allow poll on gpio value
- From: Ben Nizette <bn@xxxxxxxxxxxxxxx>
- Date: Sat, 06 Jun 2009 16:51:42 +1000
On Fri, 2009-06-05 at 23:01 -0700, David Brownell wrote:
On Friday 05 June 2009, Ben Nizette wrote:
Hey, good stuff Daniel. There's a fair few common features missing but
they can be added at a later date.
- Ability to honour rising and falling filters even if the hardware only
supports both-edge (as lots of gpio interrupts do)
I suspect that's inadvisable. Userspace code will need to know
what trigger model it's using, yes? Lies are doubleplusungood.
Well if the user wants to know that poll(2) will report each time value
transitions 0 -> 1 for example then why should it care whether that
filtering has come straight from the interrupt controller or some piece
of code has simply discarded the 1 -> 0 transitions for it?
Doing this in the kernel is a good idea IMO - the probability of the
gpio changing value between and interrupt and it's handler is much lower
than between interrupt and userspace reading "value".
Of course document that pulses smaller than interrupt latency may be
missed - a limitation which I think is perfectly fair.
- Support for polling the gpio at some interval for gpios which don't
support irqs at all
I had that thought. Units ... seconds? Milliseconds mapped to HZ?
Could come later.
Certainly could come later. ms are as good a choice as any, just so
long as root can set a lower bound on polling period.
- Debounce support
Software? Hardware capabilties vary *widely* ... three cases that
come quickly to mind: (a) twl4030 fixed 30 msec delays, (b) at91
and avr32 "deglitch" filter, just syncs to a clock that's likely
from 30-100 MHz, (c) omap2/omap3, up to 255 cycles of 32 KiHz clock
but appplies entire GPIO banks, (d) DaVinci, no hardware support.
I can imagine a standard software filter option, but that would
need to be a separate sysfs mechanism since it wouldn't always
be desired. (And separate patch, if needed.)
For hardware options ... do that by hardware-specific sysfs hooks
if they're really needed.
Oh gods, software for sure. Yeah hardware debounce support is something
which I'm not game to tackle.
I guess debouncing can be done by userspace though. In fact it almost
is done to some approximation simply by the fact that by the time the
user actually gets around to reacting the pin's probably settled anyway.
- Reporting of number of changes since last read
Feels a more than bit overkilled by now. ;)
UIO does it and I've found it useful there. I've also had requests to
implement this in my old gpio sysfs notification implementation (now
sadly neglected). Apart from anything else it leads to easy
implementation for things like robust event counters - a common use case
for this kind of thing.
These are all things which exist in many out-of-tree or
platform-specific implementations of this kind of thing and until
they're there I reckon people will largely stick with what they've got.
But that's really their problem of course, this is still valuable.
Regarding the code itself, not much but:
On Fri, 2009-06-05 at 16:36 +0200, Daniel Glöckner wrote:
+ "poll_edge" ... reads as either "none", "rising", "falling", or
IMO this is misleading, sounds like you're polling the gpio.
So, just name the sysfs attribute "edge"?
Sure. Or notification_edge though it's a mouthful. "poll" is just so
horribly overloaded in this field.
+
+ struct sysfs_dirent *value_sd;
};
No CONFIG_ option to turn all this off?
What's the .text and .data impact of this? Sure it's going to be small,
but to some people (especially those likely to care about gpio) 1k
of .data is something worth being able to turn off.
I think it's probably OK to have this covered by the current
GPIO_SYSFS flag.
Fair 'nuff. AFAICT this code is covered by that option already but the
value_sd field still needs to be wrapped.
Using an IDR keyed to the gpio value and just allocating your useful
data structures when poll_edge != "none" would help too.
Can do that without an IDR, I think...
Sure. If you're dynamically allocating these data structures though you
need some way to keep track of them. Either point to them with a field
in struct gpio_desc (which of course misses the point), chain them
together in a list or tree or $(EXOTIC_DATA_STRUCTURE) or stick them in
an IDR. IDRs just happen to be light, easy, fast and suit this
integer-to-pointer case.
--Ben.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
- References:
- [PATCH] gpiolib: allow poll on gpio value
- From: Daniel Glöckner
- Re: [PATCH] gpiolib: allow poll on gpio value
- From: Ben Nizette
- Re: [PATCH] gpiolib: allow poll on gpio value
- From: David Brownell
- [PATCH] gpiolib: allow poll on gpio value
- Prev by Date: zaurus needs generic pxa suspend/resume routines
- Next by Date: open_posix_testsuite: STOP + CONT + wait hang?
- Previous by thread: Re: [PATCH] gpiolib: allow poll on gpio value
- Next by thread: Re: [PATCH] gpiolib: allow poll on gpio value
- Index(es):
Relevant Pages
|
