[PATCH] Input: atmel_mxt_ts - Fix lost interrupts

[PATCH] Input: atmel_mxt_ts - Fix lost interrupts

[Linus Walleij]

After commit 74d905d2d38a devices requiring the workaround
for edge triggered interrupts stopped working.

This is because the "data" state container defaults to
*not* using the workaround, but the workaround gets used
*before* the check of whether it is needed or not. This
semantic is not obvious from just looking on the patch,
but related to the program flow.

The hardware needs the quirk to be used before even
proceeding to check if the quirk is needed.

This patch makes the quirk be used until we determine
it is *not* needed.

Cc: Andre <andre.muller@web.de>
Cc: Nick Dyer <nick.dyer@itdev.co.uk>
Cc: Jiada Wang <jiada_wang@mentor.com>
Cc: stable@vger.kernel.org
Fixes: 74d905d2d38a ("Input: atmel_mxt_ts - only read messages in mxt_acquire_irq() when necessary")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index e34984388791..f25b2f6038a7 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1297,8 +1297,6 @@ static int mxt_check_retrigen(struct mxt_data *data)
 	int val;
 	struct irq_data *irqd;
 
-	data->use_retrigen_workaround = false;
-
 	irqd = irq_get_irq_data(data->irq);
 	if (!irqd)
 		return -EINVAL;
@@ -1313,8 +1311,10 @@ static int mxt_check_retrigen(struct mxt_data *data)
 		if (error)
 			return error;
 
-		if (val & MXT_COMMS_RETRIGEN)
+		if (val & MXT_COMMS_RETRIGEN) {
+			data->use_retrigen_workaround = false;
 			return 0;
+		}
 	}
 
 	dev_warn(&client->dev, "Enabling RETRIGEN workaround\n");
@@ -3117,6 +3117,7 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	data = devm_kzalloc(&client->dev, sizeof(struct mxt_data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
+	data->use_retrigen_workaround = true;
 
 	snprintf(data->phys, sizeof(data->phys), "i2c-%u-%04x/input0",
 		 client->adapter->nr, client->addr);
-- 
2.26.2

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

[Dmitry Torokhov]

Hi Linus,

On Sat, Nov 28, 2020 at 01:37:20PM +0100, Linus Walleij wrote:
> After commit 74d905d2d38a devices requiring the workaround
> for edge triggered interrupts stopped working.
> 
> This is because the "data" state container defaults to
> *not* using the workaround, but the workaround gets used
> *before* the check of whether it is needed or not. This
> semantic is not obvious from just looking on the patch,
> but related to the program flow.
> 
> The hardware needs the quirk to be used before even
> proceeding to check if the quirk is needed.
> 
> This patch makes the quirk be used until we determine
> it is *not* needed.

Thank you very much for root-causing the issue!

> 
> Cc: Andre <andre.muller@web.de>
> Cc: Nick Dyer <nick.dyer@itdev.co.uk>
> Cc: Jiada Wang <jiada_wang@mentor.com>
> Cc: stable@vger.kernel.org
> Fixes: 74d905d2d38a ("Input: atmel_mxt_ts - only read messages in mxt_acquire_irq() when necessary")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index e34984388791..f25b2f6038a7 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -1297,8 +1297,6 @@ static int mxt_check_retrigen(struct mxt_data *data)
>  	int val;
>  	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.

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.

>  	irqd = irq_get_irq_data(data->irq);
>  	if (!irqd)
>  		return -EINVAL;
> @@ -1313,8 +1311,10 @@ static int mxt_check_retrigen(struct mxt_data *data)
>  		if (error)
>  			return error;
>  
> -		if (val & MXT_COMMS_RETRIGEN)
> +		if (val & MXT_COMMS_RETRIGEN) {
> +			data->use_retrigen_workaround = false;
>  			return 0;
> +		}
>  	}
>  
>  	dev_warn(&client->dev, "Enabling RETRIGEN workaround\n");
> @@ -3117,6 +3117,7 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	data = devm_kzalloc(&client->dev, sizeof(struct mxt_data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
> +	data->use_retrigen_workaround = true;
>  
>  	snprintf(data->phys, sizeof(data->phys), "i2c-%u-%04x/input0",
>  		 client->adapter->nr, client->addr);
> -- 
> 2.26.2
> 

By the way, does your touchscreen work if you change interrupt trigger
to level in DTS?

Thanks.

-- 
Dmitry

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

[Linus Walleij]

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.

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.

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.

Yours,
Linus Walleij

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

[Dmitry Torokhov]

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

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

[Andre Muller]

On 30/11/2020 09.01, Dmitry Torokhov wrote:
> 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;

This swap also fixes the driver for me. So both Linus' and your approach work here.

The log says
[    0.212647] atmel_mxt_ts i2c-ATML0000:01: Family: 164 Variant: 17 Firmware V1.0.AA Objects: 32
[    0.212804] atmel_mxt_ts i2c-ATML0000:01: Enabling RETRIGEN workaround
[    0.228469] atmel_mxt_ts i2c-ATML0000:01: Direct firmware load for maxtouch.cfg failed with error -2
[    0.229979] atmel_mxt_ts i2c-ATML0000:01: Touchscreen size X960Y540
[    0.230023] input: Atmel maXTouch Touchpad as /devices/pci0000:00/INT3432:00/i2c-0/i2c-ATML0000:01/input/input4
[    0.236080] atmel_mxt_ts i2c-ATML0001:01: Family: 164 Variant: 13 Firmware V1.0.AA Objects: 41
[    0.236478] atmel_mxt_ts i2c-ATML0001:01: Enabling RETRIGEN workaround
[    0.256034] atmel_mxt_ts i2c-ATML0001:01: Direct firmware load for maxtouch.cfg failed with error -2
[    0.257608] atmel_mxt_ts i2c-ATML0001:01: Touchscreen size X2559Y1699
[    0.257642] input: Atmel maXTouch Touchscreen as /devices/pci0000:00/INT3433:00/i2c-1/i2c-ATML0001:01/input/input5

Thank you,
Andre

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

[Dmitry Torokhov]

On Mon, Nov 30, 2020 at 09:18:45PM +0100, Andre Muller wrote:
> On 30/11/2020 09.01, Dmitry Torokhov wrote:
> > 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;
> 
> This swap also fixes the driver for me. So both Linus' and your approach work here.
> 
> The log says
> [    0.212647] atmel_mxt_ts i2c-ATML0000:01: Family: 164 Variant: 17 Firmware V1.0.AA Objects: 32
> [    0.212804] atmel_mxt_ts i2c-ATML0000:01: Enabling RETRIGEN workaround
> [    0.228469] atmel_mxt_ts i2c-ATML0000:01: Direct firmware load for maxtouch.cfg failed with error -2
> [    0.229979] atmel_mxt_ts i2c-ATML0000:01: Touchscreen size X960Y540
> [    0.230023] input: Atmel maXTouch Touchpad as /devices/pci0000:00/INT3432:00/i2c-0/i2c-ATML0000:01/input/input4
> [    0.236080] atmel_mxt_ts i2c-ATML0001:01: Family: 164 Variant: 13 Firmware V1.0.AA Objects: 41
> [    0.236478] atmel_mxt_ts i2c-ATML0001:01: Enabling RETRIGEN workaround
> [    0.256034] atmel_mxt_ts i2c-ATML0001:01: Direct firmware load for maxtouch.cfg failed with error -2
> [    0.257608] atmel_mxt_ts i2c-ATML0001:01: Touchscreen size X2559Y1699
> [    0.257642] input: Atmel maXTouch Touchscreen as /devices/pci0000:00/INT3433:00/i2c-1/i2c-ATML0001:01/input/input5

Thank you for testing Andre!

Linus, I think I prefer swapping around calls as that makes the logic on
mxt_check_retrigen() simpler. Could you please update your patch to swap
the calls as I would like to credit you for the fix.

Thanks.

-- 
Dmitry

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

[Linus Walleij]

On Tue, Dec 1, 2020 at 7:31 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> Linus, I think I prefer swapping around calls as that makes the logic on
> mxt_check_retrigen() simpler. Could you please update your patch to swap
> the calls as I would like to credit you for the fix.

Sure I sent a new version like this! It's certainly more elegant and
it works fine for me too.

Yours,
Linus Walleij