Re: [PATCH] Input: atmel_mxt_ts - Fix lost interrupts

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

On Sun, Nov 29, 2020 at 10:13:06PM +0100, Linus Walleij wrote:
> On Sun, Nov 29, 2020 at 3:53 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Sat, Nov 28, 2020 at 01:37:20PM +0100, Linus Walleij wrote:
> 
> > > @@ -1297,8 +1297,6 @@ static int mxt_check_retrigen(struct mxt_data *data)
> > >       int val;is
> > >       struct irq_data *irqd;
> > >
> > > -     data->use_retrigen_workaround = false;
> > > -
> >
> > So this will result in data->use_retrigen_workaround staying "true" for
> > level interrupts, which is not needed, as with those we will never lose
> > an edge. So I think your patch can be reduced to simply setting
> > data->use_retrigen_workaround to true in mxt_probe() and leaving
> > mxt_check_retrigen() without any changes.
> 
> I did that first but then I realized that since there is an
> errorpath in mxt_check_retrigen() and it starts by disabling
> the workaround so in an error occurs in
> __mxt_read_reg() it will be left disabled.

If __mxt_read_reg() fails then we will bail out and leave the device not
operable, so leaving the workaround disabled does not change anything.

> 
> But I see that I fail to account for the level-trigging
> case where it should disable the workaround and
> bail out so I anyway need to revise the patch.
> 
> > However I wonder if it would not be better to simply call
> > mxt_check_retrigen() before calling mxt_acquire_irq() in mxt_probe()
> > instead of after.
> 
> I don't fully understand this driver, but it seems the information
> whether retrigen is available only appears after talking to the
> device a bit, checking firmware and "objects" available on the
> device and then it may already be too late.

No, because the workaround is checked only in mxt_acquire_irq() which is
called immediately preceding the check for RETRIGEN. And since
__mxt_read_reg() is a wrapper around i2c_transfer() and does not need
IRQs to be enalbed, we can move stuff around. Could you please check if
the following works for you:

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index dde364dfb79d..2b3fff0822fe 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -2185,11 +2185,11 @@ static int mxt_initialize(struct mxt_data *data)
 		msleep(MXT_FW_RESET_TIME);
 	}
 
-	error = mxt_acquire_irq(data);
+	error = mxt_check_retrigen(data);
 	if (error)
 		return error;
 
-	error = mxt_check_retrigen(data);
+	error = mxt_acquire_irq(data);
 	if (error)
 		return error;
 

> Someone who knows the device better might be able to
> contribute here :/
> 
> > By the way, does your touchscreen work if you change interrupt trigger
> > to level in DTS?
> 
> Nope. This happens:
> [    1.824610] atmel_mxt_ts 3-004a: Failed to register interrupt
> [    1.830583] atmel_mxt_ts: probe of 3-004a failed with error -22
> 
> And that in turn is because this connected to a Nomadik
> GPIO controller which is one of those GPIO controllers
> that only support edge triggered interrupts and does not
> support level interrupts. So it needs to be edge triggered on
> this platform.

Ah, I see. Thank you.

-- 
Dmitry