Re: [PATCH] input: make gpio-keys use IRQF_SHARED

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Mon, Oct 12, 2009 at 07:09:50PM +0200, Ferenc Wagner wrote:
> Ferenc Wagner <wferi@niif.hu> writes:
> 
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> >
> >> On Tue, Sep 22, 2009 at 09:06:05PM +0200, Ferenc Wagner wrote:
> >>
> >>> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> >>> 
> >>>> On Tue, Sep 22, 2009 at 05:14:22PM +0200, Ferenc Wagner wrote:
> >>>>
> >>>>>     The gpio_get_value function may sleep, so it should not be
> >>>>>     called in a timer function.
> 
> So is drivers/staging/dream/gpio_input.c in error, too?
> 

I guess so. Initially gpio method did not sleep but that has changed.

> >>>>> But I don't see why it could sleep, is that really the case?
> >>>>
> >>>> There are things like i2c gpio extenders that require access to
> >>>> slow buses and can sleep.
> >>>
> >>> Please read my other reply in this thread before the following.
> >>> All this seems to mean that using level triggered interrupts on
> >>> such devices is impossible, unless we find a way to acknowledge the
> >>> interrupt without GPIO access.
> >>
> >> You probably want to look into threaded interrupt handlers and
> >> IRQF_ONESHOT. These can't be shared though, so it looks like you
> >> need nested IRQ handlers infrastructure.
> >
> > This sounds like a job for the irqchip setup code, if I understand
> > http://lkml.org/lkml/2009/8/15/133 correctly, that is, no driver
> > business.  Doesn't that apply only when the irqchip itself is on a
> > slow bus?  I find IRQF_ONESHOT more relevant, and sharing such a beast
> > would be possible in principle, although a little complicated, as
> > http://lkml.org/lkml/2009/8/15/131 asserts.  But still, how would the
> > somewhat more latency-sensitive serial port on the same interrupt line
> > tolerate its interrupt staying masked for a considerable period?  Even
> > if there was no hardware which shared interrupts between slow and fast
> > devices (which I hope), a driver blindly using oneshot interrupts
> > would unnecessarily add the scheduling delay to the masked period
> > instead of acknowledging and unmasking the line from hardirq context.
> > Please correct me if I got these wrong.
> >
> > On the other hand, querying gpio_cansleep could be used to avoid this,
> > and I can't conceive how the IRQ core could find out and do what's
> > best for the driver in such cases.
> >
> >>> But level triggering is needed for sharing.
> >>
> >> I believe that both level and edge-triggered interrupts can be shared.
> >
> > Sure, but all parties must agree on the trigger type, and level
> > triggering seems to be the norm (I've read
> > http://lkml.org/lkml/1998/8/7/30 on the unreliability of shared edge
> > triggered interrupts, but I don't buy Linus' argument, because the
> > system should keep asking the devices until none of them needs
> > servicing -- ineffective, but reliable).  Anyway, in my (and therefore
> > the most important) case the serial console grabs the interrupt first,
> > and although it's willing to share it, it uses level triggering, so
> > I've got no choice.
> >
> >>>>> Also, commit 57ffe9d539e0eb741bb9ca8f2834d210e70ee2e3 removed the
> >>>>> possibility of telling apart different keys, so that should be
> >>>>> reverted during the process.  I already asked Uwe Kleine-König
> >>>>> about the whys, but didn't get a reply.
> >>>>
> >>>> I don't see why you say that... You request IRQ per button and you get
> >>>> that button structure as argument in the interrupt handler.
> >>> 
> >>> In practice, several buttons often share a single IRQ line, possibly
> >>> even with other hardware, like the serial port in my case (as
> >>> described in my other reply).  So generally you need the full platform
> >>> data for all GPIO buttons in the handler, to find out which generated
> >>> the interrupt.
> >>
> >> Your interrupt handler will get called for every button on that IRQ line
> >> and you can query button state I think.
> >
> > Well, yes, if I register the handler once for each button.  Is that
> > really preferable?  It didn't occur to me as the current code does not
> > use shared interrupts, so it's out of question.  Sigh, the generic
> > GPIO interface is already rather inefficient, accessing only a single
> > bit per call (cf. the first block comment in tosakbd.c)...
> > Or did I get you wrong again?
> 
> I'd much appreciate some education on the above points as well, if
> your time permits.

I think it simplifies thigs a bit. After all we are not talking about
device delivering interrupts constantly at a very high rate so
simplicity may be preferable over ultimate performance here.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html