[PATCH v2] Input: atmel_mxt_ts - Fix lost interrupts

[PATCH v2] 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. It is determined as not needed when
we either have a level-triggered interrupt or the
firmware claims that it has enabled retrigging.

Cc: Andre Müller <andre.muller@web.de>
Cc: Nick Dyer <nick.dyer@itdev.co.uk>
Cc: Jiada Wang <jiada_wang@mentor.com>
Cc: stable@vger.kernel.org
Reported-by: Andre Müller <andre.muller@web.de>
Fixes: 74d905d2d38a ("Input: atmel_mxt_ts - only read messages in mxt_acquire_irq() when necessary")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Explicitly disable the retrig workaround also if the
  IRQ descriptor says we have a level triggered interrupt.
- Drop the second explicit assigning of "true" to the
  use_retrigen_workaround bool, it is already enabled.
- Augment debug text to say that we leave it enabled
  rather than enable it.
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index e34984388791..c822db8dbd02 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1297,14 +1297,18 @@ 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;
 
-	if (irqd_is_level_type(irqd))
+	if (irqd_is_level_type(irqd)) {
+		/*
+		 * We don't need the workaround if we have level trigged
+		 * interrupts. This will just work fine.
+		 */
+		data->use_retrigen_workaround = false;
 		return 0;
+	}
 
 	if (data->T18_address) {
 		error = __mxt_read_reg(client,
@@ -1313,12 +1317,13 @@ 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");
-	data->use_retrigen_workaround = true;
+	dev_warn(&client->dev, "Leaving RETRIGEN workaround enabled\n");
 	return 0;
 }
 
@@ -3117,6 +3122,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 v2] Input: atmel_mxt_ts - Fix lost interrupts

[Andre Muller]

The patch works for me:

[    0.205866] atmel_mxt_ts i2c-ATML0000:01: Family: 164 Variant: 17 Firmware V1.0.AA Objects: 32
[    0.221638] atmel_mxt_ts i2c-ATML0000:01: Leaving RETRIGEN workaround enabled
[    0.221692] atmel_mxt_ts i2c-ATML0000:01: Direct firmware load for maxtouch.cfg failed with error -2
[    0.223165] atmel_mxt_ts i2c-ATML0000:01: Touchscreen size X960Y540
[    0.223206] input: Atmel maXTouch Touchpad as /devices/pci0000:00/INT3432:00/i2c-0/i2c-ATML0000:01/input/input4
[    0.229177] atmel_mxt_ts i2c-ATML0001:01: Family: 164 Variant: 13 Firmware V1.0.AA Objects: 41
[    0.248909] atmel_mxt_ts i2c-ATML0001:01: Leaving RETRIGEN workaround enabled
[    0.248992] atmel_mxt_ts i2c-ATML0001:01: Direct firmware load for maxtouch.cfg failed with error -2
[    0.250396] atmel_mxt_ts i2c-ATML0001:01: Touchscreen size X2559Y1699

Tested-by: Andre Müller <andre.muller@web.de>

Thank you!

On 29/11/2020 22.24, 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. It is determined as not needed when
> we either have a level-triggered interrupt or the
> firmware claims that it has enabled retrigging.
>
> Cc: Andre Müller <andre.muller@web.de>
> Cc: Nick Dyer <nick.dyer@itdev.co.uk>
> Cc: Jiada Wang <jiada_wang@mentor.com>
> Cc: stable@vger.kernel.org
> Reported-by: Andre Müller <andre.muller@web.de>
> Fixes: 74d905d2d38a ("Input: atmel_mxt_ts - only read messages in mxt_acquire_irq() when necessary")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Explicitly disable the retrig workaround also if the
>    IRQ descriptor says we have a level triggered interrupt.
> - Drop the second explicit assigning of "true" to the
>    use_retrigen_workaround bool, it is already enabled.
> - Augment debug text to say that we leave it enabled
>    rather than enable it.
> ---
>   drivers/input/touchscreen/atmel_mxt_ts.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index e34984388791..c822db8dbd02 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -1297,14 +1297,18 @@ 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;
>
> -	if (irqd_is_level_type(irqd))
> +	if (irqd_is_level_type(irqd)) {
> +		/*
> +		 * We don't need the workaround if we have level trigged
> +		 * interrupts. This will just work fine.
> +		 */
> +		data->use_retrigen_workaround = false;
>   		return 0;
> +	}
>
>   	if (data->T18_address) {
>   		error = __mxt_read_reg(client,
> @@ -1313,12 +1317,13 @@ 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");
> -	data->use_retrigen_workaround = true;
> +	dev_warn(&client->dev, "Leaving RETRIGEN workaround enabled\n");
>   	return 0;
>   }
>
> @@ -3117,6 +3122,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);
>