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

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

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