[PATCH] input: make gpio-keys use IRQF_SHARED

[PATCH] input: make gpio-keys use IRQF_SHARED

[Dmitry Eremin-Solenikov]

There is nothing that disallows gpio-keys to share it's IRQ line
w/ other drivers. Make it use IRQF_SHARED in request_irq().

An example of other driver with which I'd like to share IRQ line
for GPIO buttons is ledtrig-gpio.

Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
 drivers/input/keyboard/gpio_keys.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index efed0c9..9fc2fab 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -147,6 +147,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 		}
 
 		error = request_irq(irq, gpio_keys_isr,
+				    IRQF_SHARED |
 				    IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
 				    button->desc ? button->desc : "gpio_keys",
 				    bdata);
-- 
1.6.3.3

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

[Dmitry Torokhov]

On Wed, Sep 16, 2009 at 07:03:18PM +0400, Dmitry Eremin-Solenikov wrote:
> There is nothing that disallows gpio-keys to share it's IRQ line
> w/ other drivers. Make it use IRQF_SHARED in request_irq().
> 
> An example of other driver with which I'd like to share IRQ line
> for GPIO buttons is ledtrig-gpio.
> 
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> ---
>  drivers/input/keyboard/gpio_keys.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index efed0c9..9fc2fab 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -147,6 +147,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>  		}
>  
>  		error = request_irq(irq, gpio_keys_isr,
> +				    IRQF_SHARED |
>  				    IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>  				    button->desc ? button->desc : "gpio_keys",
>  				    bdata);

How will you determine which device generated the interrupt? Because you
can't return IRQ_HANDLED unconditionally and expect both devices work
reliably.

-- 
Dmitry

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

[Dmitry Eremin-Solenikov]

On Wed, Sep 16, 2009 at 8:28 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Sep 16, 2009 at 07:03:18PM +0400, Dmitry Eremin-Solenikov wrote:
>> There is nothing that disallows gpio-keys to share it's IRQ line
>> w/ other drivers. Make it use IRQF_SHARED in request_irq().
>>
>> An example of other driver with which I'd like to share IRQ line
>> for GPIO buttons is ledtrig-gpio.
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>> ---
>>  drivers/input/keyboard/gpio_keys.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
>> index efed0c9..9fc2fab 100644
>> --- a/drivers/input/keyboard/gpio_keys.c
>> +++ b/drivers/input/keyboard/gpio_keys.c
>> @@ -147,6 +147,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>>               }
>>
>>               error = request_irq(irq, gpio_keys_isr,
>> +                                 IRQF_SHARED |
>>                                   IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>>                                   button->desc ? button->desc : "gpio_keys",
>>                                   bdata);
>
> How will you determine which device generated the interrupt? Because you
> can't return IRQ_HANDLED unconditionally and expect both devices work
> reliably.

It's a single device (gpio pin). However I'd like to be able to attach
several handlers to it.
E.g. one isr is gpio-keys (for reporting event to userspace), another
isr will be from
ledtrig-gpio (controlling the LED). Another can be some kind of
battery driver, etc.

All these drivers will provide different kinds of response for single GPIO pin.

-- 
With best wishes
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

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

[Dmitry Eremin-Solenikov]

On Wed, Sep 16, 2009 at 10:41 PM, Dmitry Eremin-Solenikov
<dbaryshkov@gmail.com> wrote:
> On Wed, Sep 16, 2009 at 8:28 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Wed, Sep 16, 2009 at 07:03:18PM +0400, Dmitry Eremin-Solenikov wrote:
>>> There is nothing that disallows gpio-keys to share it's IRQ line
>>> w/ other drivers. Make it use IRQF_SHARED in request_irq().
>>>
>>> An example of other driver with which I'd like to share IRQ line
>>> for GPIO buttons is ledtrig-gpio.
>>>
>>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>>> ---
>>>  drivers/input/keyboard/gpio_keys.c |    1 +
>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
>>> index efed0c9..9fc2fab 100644
>>> --- a/drivers/input/keyboard/gpio_keys.c
>>> +++ b/drivers/input/keyboard/gpio_keys.c
>>> @@ -147,6 +147,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>>>               }
>>>
>>>               error = request_irq(irq, gpio_keys_isr,
>>> +                                 IRQF_SHARED |
>>>                                   IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>>>                                   button->desc ? button->desc : "gpio_keys",
>>>                                   bdata);
>>
>> How will you determine which device generated the interrupt? Because you
>> can't return IRQ_HANDLED unconditionally and expect both devices work
>> reliably.
>
> It's a single device (gpio pin). However I'd like to be able to attach
> several handlers to it.
> E.g. one isr is gpio-keys (for reporting event to userspace), another
> isr will be from
> ledtrig-gpio (controlling the LED). Another can be some kind of
> battery driver, etc.
>
> All these drivers will provide different kinds of response for single GPIO pin.

So, what about this patch?

-- 
With best wishes
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

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

[Dmitry Torokhov]

On Fri, Sep 18, 2009 at 03:44:41PM +0400, Dmitry Eremin-Solenikov wrote:
> On Wed, Sep 16, 2009 at 10:41 PM, Dmitry Eremin-Solenikov
> <dbaryshkov@gmail.com> wrote:
> > On Wed, Sep 16, 2009 at 8:28 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> >> On Wed, Sep 16, 2009 at 07:03:18PM +0400, Dmitry Eremin-Solenikov wrote:
> >>> There is nothing that disallows gpio-keys to share it's IRQ line
> >>> w/ other drivers. Make it use IRQF_SHARED in request_irq().
> >>>
> >>> An example of other driver with which I'd like to share IRQ line
> >>> for GPIO buttons is ledtrig-gpio.
> >>>
> >>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> >>> ---
> >>>  drivers/input/keyboard/gpio_keys.c |    1 +
> >>>  1 files changed, 1 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> >>> index efed0c9..9fc2fab 100644
> >>> --- a/drivers/input/keyboard/gpio_keys.c
> >>> +++ b/drivers/input/keyboard/gpio_keys.c
> >>> @@ -147,6 +147,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
> >>>               }
> >>>
> >>>               error = request_irq(irq, gpio_keys_isr,
> >>> +                                 IRQF_SHARED |
> >>>                                   IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> >>>                                   button->desc ? button->desc : "gpio_keys",
> >>>                                   bdata);
> >>
> >> How will you determine which device generated the interrupt? Because you
> >> can't return IRQ_HANDLED unconditionally and expect both devices work
> >> reliably.
> >
> > It's a single device (gpio pin). However I'd like to be able to attach
> > several handlers to it.
> > E.g. one isr is gpio-keys (for reporting event to userspace), another
> > isr will be from
> > ledtrig-gpio (controlling the LED). Another can be some kind of
> > battery driver, etc.
> >
> > All these drivers will provide different kinds of response for single GPIO pin.
> 
> So, what about this patch?
> 

OK, will apply.

-- 
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

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

[Ferenc Wagner]

Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

> On Fri, Sep 18, 2009 at 03:44:41PM +0400, Dmitry Eremin-Solenikov wrote:
>> On Wed, Sep 16, 2009 at 10:41 PM, Dmitry Eremin-Solenikov
>> <dbaryshkov@gmail.com> wrote:
>>> On Wed, Sep 16, 2009 at 8:28 PM, Dmitry Torokhov
>>> <dmitry.torokhov@gmail.com> wrote:
>>>> On Wed, Sep 16, 2009 at 07:03:18PM +0400, Dmitry Eremin-Solenikov wrote:
>>>>> There is nothing that disallows gpio-keys to share it's IRQ line
>>>>> w/ other drivers. Make it use IRQF_SHARED in request_irq().
>>>>>
>>>>> An example of other driver with which I'd like to share IRQ line
>>>>> for GPIO buttons is ledtrig-gpio.
>>>>>
>>>>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>>>>> ---
>>>>>  drivers/input/keyboard/gpio_keys.c |    1 +
>>>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
>>>>> index efed0c9..9fc2fab 100644
>>>>> --- a/drivers/input/keyboard/gpio_keys.c
>>>>> +++ b/drivers/input/keyboard/gpio_keys.c
>>>>> @@ -147,6 +147,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>>>>>               }
>>>>>
>>>>>               error = request_irq(irq, gpio_keys_isr,
>>>>> +                                 IRQF_SHARED |
>>>>>                                   IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>>>>>                                   button->desc ? button->desc : "gpio_keys",
>>>>>                                   bdata);
>>>>
>>>> How will you determine which device generated the interrupt? Because you
>>>> can't return IRQ_HANDLED unconditionally and expect both devices work
>>>> reliably.
>>>
>>> It's a single device (gpio pin). However I'd like to be able to attach
>>> several handlers to it.
>>> E.g. one isr is gpio-keys (for reporting event to userspace), another
>>> isr will be from
>>> ledtrig-gpio (controlling the LED). Another can be some kind of
>>> battery driver, etc.
>>>
>>> All these drivers will provide different kinds of response for single GPIO pin.
>> 
>> So, what about this patch?
>
> OK, will apply.

I still think it's dangerous.  But I am not a kernel hacker and a very
beginner on this whole field, so please let me explain why, and please
correct my conclusions as needed.

I met this problem playing with an embedded platform (brcm47xx), where
I wanted to employ the gpio_keys driver.  Registering the gpio_keys
platform device wasn't enough, because the gpio_keys module still
couldn't be loaded as the serial console already used the same
interrupt line of the Sonics Silicon Backplane.  (Also, it used
shared, but level triggered interrupts, so simply adding in
IRQF_SHARED wouldn't have helped either, besides breaking both
devices).  Anyway, such platforms may have several GPIO buttons,
typically using the same interrupt line, so there must be a way to
tell them apart, which looks impossible without some GPIO access to
acknowledge (change polarity of) the correct button.  So I plan to put
forward some changes in this area, like switching to level triggered
interrupts to facilitate sharing.  I've got some proof of concept
code, but still have to learn the ropes.

Apart from this, adding in IRQF_SHARED like above for being able to
attach several handlers looks fishy big time.  Would the rest be
invoked at all once you returned IRQ_HANDLED from the first (it's a
question of fact in Linux interrupt handling, I just down't know, but
would guess yes, for level triggered interrupts at least)?  Is this
common practice?
-- 
Thanks,
Feri.
--
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

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

[Ferenc Wagner]

Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

> On Wed, Sep 16, 2009 at 07:03:18PM +0400, Dmitry Eremin-Solenikov wrote:
>> There is nothing that disallows gpio-keys to share it's IRQ line
>> w/ other drivers. Make it use IRQF_SHARED in request_irq().
>> 
>> An example of other driver with which I'd like to share IRQ line
>> for GPIO buttons is ledtrig-gpio.
>> 
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>> ---
>>  drivers/input/keyboard/gpio_keys.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>> 
>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
>> index efed0c9..9fc2fab 100644
>> --- a/drivers/input/keyboard/gpio_keys.c
>> +++ b/drivers/input/keyboard/gpio_keys.c
>> @@ -147,6 +147,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>>  		}
>>  
>>  		error = request_irq(irq, gpio_keys_isr,
>> +				    IRQF_SHARED |
>>  				    IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>>  				    button->desc ? button->desc : "gpio_keys",
>>  				    bdata);
>
> How will you determine which device generated the interrupt? Because you
> can't return IRQ_HANDLED unconditionally and expect both devices work
> reliably.

It would be possible, but commit
da0d03fe6cecde837f113a8a587f5a872d0fade0 states that:

    The gpio_get_value function may sleep, so it should not be called in a
    timer function.

But I don't see why it could sleep, is that really the case?  Because
that makes acknowledging the interrupt very hard.  Actually, I need
and tested interrupt sharing.  What are the "cansleep wrappers" in
asm-generic/gpio.h if these function can sleep as well?  I'd be
grateful for some explanation.  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.
-- 
Regards,
Feri.
--
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

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

[Dmitry Torokhov]

On Tue, Sep 22, 2009 at 05:14:22PM +0200, Ferenc Wagner wrote:
> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> 
> > On Wed, Sep 16, 2009 at 07:03:18PM +0400, Dmitry Eremin-Solenikov wrote:
> >> There is nothing that disallows gpio-keys to share it's IRQ line
> >> w/ other drivers. Make it use IRQF_SHARED in request_irq().
> >> 
> >> An example of other driver with which I'd like to share IRQ line
> >> for GPIO buttons is ledtrig-gpio.
> >> 
> >> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> >> ---
> >>  drivers/input/keyboard/gpio_keys.c |    1 +
> >>  1 files changed, 1 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> >> index efed0c9..9fc2fab 100644
> >> --- a/drivers/input/keyboard/gpio_keys.c
> >> +++ b/drivers/input/keyboard/gpio_keys.c
> >> @@ -147,6 +147,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
> >>  		}
> >>  
> >>  		error = request_irq(irq, gpio_keys_isr,
> >> +				    IRQF_SHARED |
> >>  				    IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> >>  				    button->desc ? button->desc : "gpio_keys",
> >>  				    bdata);
> >
> > How will you determine which device generated the interrupt? Because you
> > can't return IRQ_HANDLED unconditionally and expect both devices work
> > reliably.
> 
> It would be possible, but commit
> da0d03fe6cecde837f113a8a587f5a872d0fade0 states that:
> 
>     The gpio_get_value function may sleep, so it should not be called in a
>     timer function.
> 
> 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't sleep.

>  Because
> that makes acknowledging the interrupt very hard.  Actually, I need
> and tested interrupt sharing.  What are the "cansleep wrappers" in
> asm-generic/gpio.h if these function can sleep as well?  I'd be
> grateful for some explanation.  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.

-- 
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

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

[Ferenc Wagner]

Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

> On Tue, Sep 22, 2009 at 05:14:22PM +0200, Ferenc Wagner wrote:
>> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
>> 
>>> On Wed, Sep 16, 2009 at 07:03:18PM +0400, Dmitry Eremin-Solenikov wrote:
>>>> There is nothing that disallows gpio-keys to share it's IRQ line
>>>> w/ other drivers. Make it use IRQF_SHARED in request_irq().
>>>> 
>>>> An example of other driver with which I'd like to share IRQ line
>>>> for GPIO buttons is ledtrig-gpio.
>>>> 
>>>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>>>> ---
>>>>  drivers/input/keyboard/gpio_keys.c |    1 +
>>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>> 
>>>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
>>>> index efed0c9..9fc2fab 100644
>>>> --- a/drivers/input/keyboard/gpio_keys.c
>>>> +++ b/drivers/input/keyboard/gpio_keys.c
>>>> @@ -147,6 +147,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
>>>>  		}
>>>>  
>>>>  		error = request_irq(irq, gpio_keys_isr,
>>>> +				    IRQF_SHARED |
>>>>  				    IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>>>>  				    button->desc ? button->desc : "gpio_keys",
>>>>  				    bdata);
>>>
>>> How will you determine which device generated the interrupt? Because you
>>> can't return IRQ_HANDLED unconditionally and expect both devices work
>>> reliably.
>> 
>> It would be possible, but commit
>> da0d03fe6cecde837f113a8a587f5a872d0fade0 states that:
>> 
>>     The gpio_get_value function may sleep, so it should not be called in a
>>     timer function.
>> 
>> 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't sleep.

I assume you mean "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.  But level
triggering is needed for sharing.  Maybe we could make this
configurable on a per-button basis?  Or by a module parameter?
Anyway, a gpio_cansleep test should go into the module initialisation
routine.

>> 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.
-- 
Thanks,
Feri.
--
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

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

[Dmitry Torokhov]

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:
> >> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> >> 
> >>> On Wed, Sep 16, 2009 at 07:03:18PM +0400, Dmitry Eremin-Solenikov wrote:
> >>>> There is nothing that disallows gpio-keys to share it's IRQ line
> >>>> w/ other drivers. Make it use IRQF_SHARED in request_irq().
> >>>> 
> >>>> An example of other driver with which I'd like to share IRQ line
> >>>> for GPIO buttons is ledtrig-gpio.
> >>>> 
> >>>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> >>>> ---
> >>>>  drivers/input/keyboard/gpio_keys.c |    1 +
> >>>>  1 files changed, 1 insertions(+), 0 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> >>>> index efed0c9..9fc2fab 100644
> >>>> --- a/drivers/input/keyboard/gpio_keys.c
> >>>> +++ b/drivers/input/keyboard/gpio_keys.c
> >>>> @@ -147,6 +147,7 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
> >>>>  		}
> >>>>  
> >>>>  		error = request_irq(irq, gpio_keys_isr,
> >>>> +				    IRQF_SHARED |
> >>>>  				    IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> >>>>  				    button->desc ? button->desc : "gpio_keys",
> >>>>  				    bdata);
> >>>
> >>> How will you determine which device generated the interrupt? Because you
> >>> can't return IRQ_HANDLED unconditionally and expect both devices work
> >>> reliably.
> >> 
> >> It would be possible, but commit
> >> da0d03fe6cecde837f113a8a587f5a872d0fade0 states that:
> >> 
> >>     The gpio_get_value function may sleep, so it should not be called in a
> >>     timer function.
> >> 
> >> 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't sleep.
> 
> I assume you mean "can sleep".

Yes.

> 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 ifrastructure.

> But level
> triggering is needed for sharing.

I believe that both level and edge-triggered interrupts can be shared.

>  Maybe we could make this
> configurable on a per-button basis?  Or by a module parameter?
> Anyway, a gpio_cansleep test should go into the module initialisation
> routine.
> 
> >> 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.

-- 
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

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

[Ferenc Wagner]

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.
>>>> 
>>>> 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?
-- 
Thanks,
Feri.
--
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

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

[Ferenc Wagner]

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?

>>>>> 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.
-- 
Thanks,
Feri.
--
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

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

[Dmitry Torokhov]

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

Re: gpio_get_value in atomic context (was: make gpio-keys use IRQF_SHARED)

[Ferenc Wagner]

Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

> On Mon, Oct 12, 2009 at 07:09:50PM +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.

Let's make Arve aware of this then (even though it may not affect
Android in practice).
-- 
Feri.

Re: gpio_get_value in atomic context (was: make gpio-keys use IRQF_SHARED)

[Arve Hjønnevåg]

2009/10/14 Ferenc Wagner <wferi@niif.hu>:
> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
>
>> On Mon, Oct 12, 2009 at 07:09:50PM +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.
>
> Let's make Arve aware of this then (even though it may not affect
> Android in practice).

If gpio_get_value may sleep, then what is gpio_get_value_cansleep for?
The gpio documentation claims gpio_get_value and gpio_set_value are
safe to use from interrupt context, but a call to gpio_cansleep is
probably needed in probe.

The matrix driver also need to change the direction of the gpio from
the timer, so it would be useful to have a standard way to check if
this is allowed as well.

-- 
Arve Hjønnevåg
--
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

Re: gpio_get_value in atomic context (was: make gpio-keys use IRQF_SHARED)

[David Brownell]

On Wednesday 14 October 2009, Arve Hjønnevåg wrote:
> 2009/10/14 Ferenc Wagner <wferi@niif.hu>:
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> >
> >> On Mon, Oct 12, 2009 at 07:09:50PM +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.
> >
> > Let's make Arve aware of this then (even though it may not affect
> > Android in practice).
> 
> If gpio_get_value may sleep, then what is gpio_get_value_cansleep for?

To tell if this is one of the GPIOs for which it may sleep.

Whether the GPIO is atomic vs cansleep is an attribute of the
GPIO itself, not the routine it's called with.


> The gpio documentation claims gpio_get_value and gpio_set_value are
> safe to use from interrupt context, but a call to gpio_cansleep is
> probably needed in probe.

They're safe to use there ... if that GPIO is safe.  Yes, make sure
you know the answer before you call gpio_get_value() instead of
the always-safe gpio_get_value_cansleep().


> The matrix driver also need to change the direction of the gpio from
> the timer, so it would be useful to have a standard way to check if
> this is allowed as well.

If gpio_{set,get}_value() can sleep, so can gpio_direction_*().

- Dave


--
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

Re: gpio_get_value in atomic context

[Ferenc Wagner]

David Brownell <david-b@pacbell.net> writes:

> On Wednesday 14 October 2009, Arve Hjønnevåg wrote:
>
>>> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
>>>
>>>> On Mon, Oct 12, 2009 at 07:09:50PM +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.
>> 
>> If gpio_get_value may sleep, then what is gpio_get_value_cansleep for?
>
> To tell if this is one of the GPIOs for which it may sleep.

Isn't that gpio_cansleep()?

> Whether the GPIO is atomic vs cansleep is an attribute of the
> GPIO itself, not the routine it's called with.

My reading of Documentation/gpio.txt implies that actually it is,
because gpio_get_value() is ignored for GPIOs which can sleep:

    Other than the fact that these (gpio_[sg]et_value_cansleep) calls
    might sleep, and will not be ignored for GPIOs that can't be
    accessed from IRQ handlers, these calls act the same as the
    spinlock-safe calls.

>> The gpio documentation claims gpio_get_value and gpio_set_value are
>> safe to use from interrupt context, but a call to gpio_cansleep is
>> probably needed in probe.
>
> They're safe to use there ... if that GPIO is safe.  Yes, make sure
> you know the answer before you call gpio_get_value() instead of
> the always-safe gpio_get_value_cansleep().

Always-safe, meaning never ignored?  They surely aren't safe to use in
atomic context...

>> The matrix driver also need to change the direction of the gpio from
>> the timer, so it would be useful to have a standard way to check if
>> this is allowed as well.
>
> If gpio_{set,get}_value() can sleep, so can gpio_direction_*().

As above, in my reading gpio_[sg]et_value() cannot sleep, but are
ignored for the sleepy GPIOs.  Also, from the documentation:

    Also, using these calls for GPIOs that can't safely be accessed
    without sleeping is an error.

I'd deeply appreciate some further clarification of the above, and
possibly on the problems outlined in the thread
http://thread.gmane.org/gmane.linux.kernel.input/8775/focus=9153.  Is it
possible to use level triggered shared IRQs for GPIOs which can sleep?
If yes, how?
-- 
Thanks,
Feri.
--
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

Re: gpio_get_value in atomic context

[Ferenc Wagner]

David Brownell <david-b@pacbell.net> writes:

> On Wednesday 14 October 2009, Arve Hjønnevåg wrote:
>
>>> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
>>>
>>>> On Mon, Oct 12, 2009 at 07:09:50PM +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.
>> 
>> If gpio_get_value may sleep, then what is gpio_get_value_cansleep for?
>
> To tell if this is one of the GPIOs for which it may sleep.

Isn't that gpio_cansleep()?

> Whether the GPIO is atomic vs cansleep is an attribute of the
> GPIO itself, not the routine it's called with.

My reading of Documentation/gpio.txt implies that actually it is,
because gpio_get_value() is ignored for GPIOs which can sleep:

    Other than the fact that these (gpio_[sg]et_value_cansleep) calls
    might sleep, and will not be ignored for GPIOs that can't be
    accessed from IRQ handlers, these calls act the same as the
    spinlock-safe calls.

>> The gpio documentation claims gpio_get_value and gpio_set_value are
>> safe to use from interrupt context, but a call to gpio_cansleep is
>> probably needed in probe.
>
> They're safe to use there ... if that GPIO is safe.  Yes, make sure
> you know the answer before you call gpio_get_value() instead of
> the always-safe gpio_get_value_cansleep().

Always-safe, meaning never ignored?  They surely aren't safe to use in
atomic context...

>> The matrix driver also need to change the direction of the gpio from
>> the timer, so it would be useful to have a standard way to check if
>> this is allowed as well.
>
> If gpio_{set,get}_value() can sleep, so can gpio_direction_*().

As above, in my reading gpio_[sg]et_value() cannot sleep, but are
ignored for the sleepy GPIOs.  Also, from the documentation:

    Also, using these calls for GPIOs that can't safely be accessed
    without sleeping is an error.

I'd deeply appreciate some further clarification of the above, and
possibly on the problems outlined in the thread
http://thread.gmane.org/gmane.linux.kernel.input/8775/focus=9153.  Is it
possible to use level triggered shared IRQs for GPIOs which can sleep?
If yes, how?

Using a nested handler would be a possibility, if I could assume that
the irqchips belonging to sleepy GPIOs are always sleepy as well
(ie. their dispatchers run in a separate thread with the IRQ disabled).
Is that the case by any chance?
-- 
Thanks,
Feri.
--
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

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

[Ferenc Wagner]

Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

> On Mon, Oct 12, 2009 at 07:09:50PM +0200, Ferenc Wagner wrote:
>
>> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
>>
>>> On Tue, Sep 22, 2009 at 09:06:05PM +0200, Ferenc Wagner wrote:
>>>
>>>> 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.

I'd appreciate hearing your opinion on the above, too.

>> 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.

And on this...

>>>> 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.

This scheme depends on triggering on the falling edge or otherwise
polling until the line goes inactive again, so that new interrupts can
be registered.  Sounds not so good.  But anyway, it's mostly academic
pondering, as we're dealing with level-triggered interrupts now.

>>>>> On Tue, Sep 22, 2009 at 05:14:22PM +0200, Ferenc Wagner wrote:
>>>>>
>>>>>> 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)...
>
> 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.

I see, but I also feel like the simplification is at most negligible.
The very same code which installs the one-button-handlers could be
used in a single handler to test each gpio in turn.  And we don't go
against common practice.
-- 
Thanks,
Feri.
--
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