| Message ID | 20190422083540.8380-2-daniel@zonque.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] input: touch: eeti: move ISR code to own function | expand |
Hi Daniel, On Mon, Apr 22, 2019 at 10:35:40AM +0200, Daniel Mack wrote: > For systems in which the touch IRQ is acting as wakeup source, the interrupt > controller might not latch the GPIO IRQ during sleep. In such cases, the > interrupt will never occur again after resume, hence the touch screen > appears dead. > > To fix this, call into eeti_ts_read() once to read the hardware status and > to arm the IRQ again. Can you instead make the interrupt level-triggered? Thanks.
Hi Dmitry, On 23/4/2019 5:17 AM, Dmitry Torokhov wrote: > On Mon, Apr 22, 2019 at 10:35:40AM +0200, Daniel Mack wrote: >> For systems in which the touch IRQ is acting as wakeup source, the interrupt >> controller might not latch the GPIO IRQ during sleep. In such cases, the >> interrupt will never occur again after resume, hence the touch screen >> appears dead. >> >> To fix this, call into eeti_ts_read() once to read the hardware status and >> to arm the IRQ again. > > Can you instead make the interrupt level-triggered? The hardware I'm working on doesn't support that unfortunately. In fact, the whole attn-gpio dance is there because of that, and the GPIO descriptor maps to the same pin that also causes the IRQ in my case. Thanks, Daniel
On Tue, Apr 23, 2019 at 06:51:32AM +0200, Daniel Mack wrote: > Hi Dmitry, > > On 23/4/2019 5:17 AM, Dmitry Torokhov wrote: > > On Mon, Apr 22, 2019 at 10:35:40AM +0200, Daniel Mack wrote: > >> For systems in which the touch IRQ is acting as wakeup source, the interrupt > >> controller might not latch the GPIO IRQ during sleep. In such cases, the > >> interrupt will never occur again after resume, hence the touch screen > >> appears dead. > >> > >> To fix this, call into eeti_ts_read() once to read the hardware status and > >> to arm the IRQ again. > > > > Can you instead make the interrupt level-triggered? > > The hardware I'm working on doesn't support that unfortunately. > > In fact, the whole attn-gpio dance is there because of that, and the > GPIO descriptor maps to the same pin that also causes the IRQ in my case. OK, if the interrupt controller is incapable of dealing with level interrupts then we have to do what you propose. Thanks.
On 23/4/2019 10:41 AM, Dmitry Torokhov wrote: > On Tue, Apr 23, 2019 at 06:51:32AM +0200, Daniel Mack wrote: >> Hi Dmitry, >> >> On 23/4/2019 5:17 AM, Dmitry Torokhov wrote: >>> On Mon, Apr 22, 2019 at 10:35:40AM +0200, Daniel Mack wrote: >>>> For systems in which the touch IRQ is acting as wakeup source, the interrupt >>>> controller might not latch the GPIO IRQ during sleep. In such cases, the >>>> interrupt will never occur again after resume, hence the touch screen >>>> appears dead. >>>> >>>> To fix this, call into eeti_ts_read() once to read the hardware status and >>>> to arm the IRQ again. >>> >>> Can you instead make the interrupt level-triggered? >> >> The hardware I'm working on doesn't support that unfortunately. >> >> In fact, the whole attn-gpio dance is there because of that, and the >> GPIO descriptor maps to the same pin that also causes the IRQ in my case. > > OK, if the interrupt controller is incapable of dealing with level > interrupts then we have to do what you propose. So you consider these patches for inclusion then? I'm just asking because I can't see them in your tree yet. Thanks, Daniel
On Sun, Apr 28, 2019 at 09:18:46AM +0200, Daniel Mack wrote: > On 23/4/2019 10:41 AM, Dmitry Torokhov wrote: > > On Tue, Apr 23, 2019 at 06:51:32AM +0200, Daniel Mack wrote: > >> Hi Dmitry, > >> > >> On 23/4/2019 5:17 AM, Dmitry Torokhov wrote: > >>> On Mon, Apr 22, 2019 at 10:35:40AM +0200, Daniel Mack wrote: > >>>> For systems in which the touch IRQ is acting as wakeup source, the interrupt > >>>> controller might not latch the GPIO IRQ during sleep. In such cases, the > >>>> interrupt will never occur again after resume, hence the touch screen > >>>> appears dead. > >>>> > >>>> To fix this, call into eeti_ts_read() once to read the hardware status and > >>>> to arm the IRQ again. > >>> > >>> Can you instead make the interrupt level-triggered? > >> > >> The hardware I'm working on doesn't support that unfortunately. > >> > >> In fact, the whole attn-gpio dance is there because of that, and the > >> GPIO descriptor maps to the same pin that also causes the IRQ in my case. > > > > OK, if the interrupt controller is incapable of dealing with level > > interrupts then we have to do what you propose. > > So you consider these patches for inclusion then? I'm just asking > because I can't see them in your tree yet. I was about to, but now I wonder if we need a mutex in the isr code now, otherwise there is a chance it will be running concurrently when we are resuming if interrupt does latch. Thanks.
On 28/4/2019 7:36 PM, Dmitry Torokhov wrote: > On Sun, Apr 28, 2019 at 09:18:46AM +0200, Daniel Mack wrote: >> On 23/4/2019 10:41 AM, Dmitry Torokhov wrote: >>> On Tue, Apr 23, 2019 at 06:51:32AM +0200, Daniel Mack wrote: >>>> Hi Dmitry, >>>> >>>> On 23/4/2019 5:17 AM, Dmitry Torokhov wrote: >>>>> On Mon, Apr 22, 2019 at 10:35:40AM +0200, Daniel Mack wrote: >>>>>> For systems in which the touch IRQ is acting as wakeup source, the interrupt >>>>>> controller might not latch the GPIO IRQ during sleep. In such cases, the >>>>>> interrupt will never occur again after resume, hence the touch screen >>>>>> appears dead. >>>>>> >>>>>> To fix this, call into eeti_ts_read() once to read the hardware status and >>>>>> to arm the IRQ again. >>>>> >>>>> Can you instead make the interrupt level-triggered? >>>> >>>> The hardware I'm working on doesn't support that unfortunately. >>>> >>>> In fact, the whole attn-gpio dance is there because of that, and the >>>> GPIO descriptor maps to the same pin that also causes the IRQ in my case. >>> >>> OK, if the interrupt controller is incapable of dealing with level >>> interrupts then we have to do what you propose. >> >> So you consider these patches for inclusion then? I'm just asking >> because I can't see them in your tree yet. > > I was about to, but now I wonder if we need a mutex in the isr code now, > otherwise there is a chance it will be running concurrently when we are > resuming if interrupt does latch. Ah, good point. I'll send a v2. Thanks, Daniel
On 28/4/2019 7:36 PM, Dmitry Torokhov wrote: > On Sun, Apr 28, 2019 at 09:18:46AM +0200, Daniel Mack wrote: >> On 23/4/2019 10:41 AM, Dmitry Torokhov wrote: >>> On Tue, Apr 23, 2019 at 06:51:32AM +0200, Daniel Mack wrote: >>>> Hi Dmitry, >>>> >>>> On 23/4/2019 5:17 AM, Dmitry Torokhov wrote: >>>>> On Mon, Apr 22, 2019 at 10:35:40AM +0200, Daniel Mack wrote: >>>>>> For systems in which the touch IRQ is acting as wakeup source, the interrupt >>>>>> controller might not latch the GPIO IRQ during sleep. In such cases, the >>>>>> interrupt will never occur again after resume, hence the touch screen >>>>>> appears dead. >>>>>> >>>>>> To fix this, call into eeti_ts_read() once to read the hardware status and >>>>>> to arm the IRQ again. >>>>> >>>>> Can you instead make the interrupt level-triggered? >>>> >>>> The hardware I'm working on doesn't support that unfortunately. >>>> >>>> In fact, the whole attn-gpio dance is there because of that, and the >>>> GPIO descriptor maps to the same pin that also causes the IRQ in my case. >>> >>> OK, if the interrupt controller is incapable of dealing with level >>> interrupts then we have to do what you propose. >> >> So you consider these patches for inclusion then? I'm just asking >> because I can't see them in your tree yet. > > I was about to, but now I wonder if we need a mutex in the isr code now, > otherwise there is a chance it will be running concurrently when we are > resuming if interrupt does latch. Actually, to address that, all we need to do is call into eeti_ts_read() before enable_irq(). See my v2. Thanks, Daniel
On Sun, Apr 28, 2019 at 09:50:35PM +0200, Daniel Mack wrote: > On 28/4/2019 7:36 PM, Dmitry Torokhov wrote: > > On Sun, Apr 28, 2019 at 09:18:46AM +0200, Daniel Mack wrote: > >> On 23/4/2019 10:41 AM, Dmitry Torokhov wrote: > >>> On Tue, Apr 23, 2019 at 06:51:32AM +0200, Daniel Mack wrote: > >>>> Hi Dmitry, > >>>> > >>>> On 23/4/2019 5:17 AM, Dmitry Torokhov wrote: > >>>>> On Mon, Apr 22, 2019 at 10:35:40AM +0200, Daniel Mack wrote: > >>>>>> For systems in which the touch IRQ is acting as wakeup source, the interrupt > >>>>>> controller might not latch the GPIO IRQ during sleep. In such cases, the > >>>>>> interrupt will never occur again after resume, hence the touch screen > >>>>>> appears dead. > >>>>>> > >>>>>> To fix this, call into eeti_ts_read() once to read the hardware status and > >>>>>> to arm the IRQ again. > >>>>> > >>>>> Can you instead make the interrupt level-triggered? > >>>> > >>>> The hardware I'm working on doesn't support that unfortunately. > >>>> > >>>> In fact, the whole attn-gpio dance is there because of that, and the > >>>> GPIO descriptor maps to the same pin that also causes the IRQ in my case. > >>> > >>> OK, if the interrupt controller is incapable of dealing with level > >>> interrupts then we have to do what you propose. > >> > >> So you consider these patches for inclusion then? I'm just asking > >> because I can't see them in your tree yet. > > > > I was about to, but now I wonder if we need a mutex in the isr code now, > > otherwise there is a chance it will be running concurrently when we are > > resuming if interrupt does latch. > > Actually, to address that, all we need to do is call into eeti_ts_read() > before enable_irq(). See my v2. This is still a bit racy as interrupt may come after you attempted to read but before enable_irq() and then we need interrupt replaying code to work reliably, which, as far as I understand, is not always the case. I suppose we need at least add a comment that we rely on replays. What kind of hardware is that? Is there no chance of making interrupt controller handle level interrupts? Thanks.
On 29/4/2019 3:17 AM, Dmitry Torokhov wrote: > On Sun, Apr 28, 2019 at 09:50:35PM +0200, Daniel Mack wrote: >> On 28/4/2019 7:36 PM, Dmitry Torokhov wrote: >>> On Sun, Apr 28, 2019 at 09:18:46AM +0200, Daniel Mack wrote: >>>> On 23/4/2019 10:41 AM, Dmitry Torokhov wrote: >>>>> On Tue, Apr 23, 2019 at 06:51:32AM +0200, Daniel Mack wrote: >>>>>> Hi Dmitry, >>>>>> >>>>>> On 23/4/2019 5:17 AM, Dmitry Torokhov wrote: >>>>>>> On Mon, Apr 22, 2019 at 10:35:40AM +0200, Daniel Mack wrote: >>>>>>>> For systems in which the touch IRQ is acting as wakeup source, the interrupt >>>>>>>> controller might not latch the GPIO IRQ during sleep. In such cases, the >>>>>>>> interrupt will never occur again after resume, hence the touch screen >>>>>>>> appears dead. >>>>>>>> >>>>>>>> To fix this, call into eeti_ts_read() once to read the hardware status and >>>>>>>> to arm the IRQ again. >>>>>>> >>>>>>> Can you instead make the interrupt level-triggered? >>>>>> >>>>>> The hardware I'm working on doesn't support that unfortunately. >>>>>> >>>>>> In fact, the whole attn-gpio dance is there because of that, and the >>>>>> GPIO descriptor maps to the same pin that also causes the IRQ in my case. >>>>> >>>>> OK, if the interrupt controller is incapable of dealing with level >>>>> interrupts then we have to do what you propose. >>>> >>>> So you consider these patches for inclusion then? I'm just asking >>>> because I can't see them in your tree yet. >>> >>> I was about to, but now I wonder if we need a mutex in the isr code now, >>> otherwise there is a chance it will be running concurrently when we are >>> resuming if interrupt does latch. >> >> Actually, to address that, all we need to do is call into eeti_ts_read() >> before enable_irq(). See my v2. > > This is still a bit racy as interrupt may come after you attempted to > read but before enable_irq() and then we need interrupt replaying code > to work reliably, which, as far as I understand, is not always the case. > I suppose we need at least add a comment that we rely on replays. Ah, of course. Okay, let's just have a mutex for this then. I'll send a v3. > What kind of hardware is that? Is there no chance of making interrupt > controller handle level interrupts? It's a PXA3xx platform, and their interrupt controller lacks such a feature. Thanks, Daniel
diff --git a/drivers/input/touchscreen/eeti_ts.c b/drivers/input/touchscreen/eeti_ts.c index f5724aaa815b..674386f910ba 100644 --- a/drivers/input/touchscreen/eeti_ts.c +++ b/drivers/input/touchscreen/eeti_ts.c @@ -117,6 +117,7 @@ static void eeti_ts_start(struct eeti_ts *eeti) eeti->running = true; wmb(); enable_irq(eeti->client->irq); + eeti_ts_read(eeti); } static void eeti_ts_stop(struct eeti_ts *eeti)
For systems in which the touch IRQ is acting as wakeup source, the interrupt controller might not latch the GPIO IRQ during sleep. In such cases, the interrupt will never occur again after resume, hence the touch screen appears dead. To fix this, call into eeti_ts_read() once to read the hardware status and to arm the IRQ again. Signed-off-by: Daniel Mack <daniel@zonque.org> Reported-by: Sven Neumann <Sven.Neumann@teufel.de> --- drivers/input/touchscreen/eeti_ts.c | 1 + 1 file changed, 1 insertion(+)