[PATCH 1/2] input: touch: eeti: move ISR code to own function

[PATCH 1/2] input: touch: eeti: move ISR code to own function

[Daniel Mack]

Move the ISR handling code to its own function and change the logic to bail
immediately in case of .running is false or if the attn_gpio is available
but unasserted.

This allows us to call the function at any time to check for the state of
attn_gpio.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/input/touchscreen/eeti_ts.c | 30 +++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/input/touchscreen/eeti_ts.c b/drivers/input/touchscreen/eeti_ts.c
index 7fe41965c5d1..f5724aaa815b 100644
--- a/drivers/input/touchscreen/eeti_ts.c
+++ b/drivers/input/touchscreen/eeti_ts.c
@@ -75,14 +75,19 @@ static void eeti_ts_report_event(struct eeti_ts *eeti, u8 *buf)
 	input_sync(eeti->input);
 }
 
-static irqreturn_t eeti_ts_isr(int irq, void *dev_id)
+static void eeti_ts_read(struct eeti_ts *eeti)
 {
-	struct eeti_ts *eeti = dev_id;
-	int len;
-	int error;
+	int len, error;
 	char buf[6];
 
-	do {
+	for (;;) {
+		if (!eeti->running)
+			break;
+
+		if (eeti->attn_gpio &&
+		    gpiod_get_value_cansleep(eeti->attn_gpio) == 0)
+			break;
+
 		len = i2c_master_recv(eeti->client, buf, sizeof(buf));
 		if (len != sizeof(buf)) {
 			error = len < 0 ? len : -EIO;
@@ -92,12 +97,17 @@ static irqreturn_t eeti_ts_isr(int irq, void *dev_id)
 			break;
 		}
 
-		if (buf[0] & 0x80) {
-			/* Motion packet */
+		/* Motion packet */
+		if (buf[0] & 0x80)
 			eeti_ts_report_event(eeti, buf);
-		}
-	} while (eeti->running &&
-		 eeti->attn_gpio && gpiod_get_value_cansleep(eeti->attn_gpio));
+	}
+}
+
+static irqreturn_t eeti_ts_isr(int irq, void *dev_id)
+{
+	struct eeti_ts *eeti = dev_id;
+
+	eeti_ts_read(eeti);
 
 	return IRQ_HANDLED;
 }
-- 
2.20.1

[PATCH 2/2] input: touch: eeti: read hardware state once after wakeup

[Daniel Mack]

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(+)

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

Re: [PATCH 2/2] input: touch: eeti: read hardware state once after wakeup

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH 2/2] input: touch: eeti: read hardware state once after wakeup

[Daniel Mack]

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

Re: [PATCH 2/2] input: touch: eeti: read hardware state once after wakeup

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH 2/2] input: touch: eeti: read hardware state once after wakeup

[Daniel Mack]

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

Re: [PATCH 2/2] input: touch: eeti: read hardware state once after wakeup

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH 2/2] input: touch: eeti: read hardware state once after wakeup

[Daniel Mack]

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

Re: [PATCH 2/2] input: touch: eeti: read hardware state once after wakeup

[Daniel Mack]

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

Re: [PATCH 2/2] input: touch: eeti: read hardware state once after wakeup

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH 2/2] input: touch: eeti: read hardware state once after wakeup

[Daniel Mack]

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