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@xxxxxx>
> Cc: Nick Dyer <nick.dyer@xxxxxxxxxxx>
> Cc: Jiada Wang <jiada_wang@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 74d905d2d38a ("Input: atmel_mxt_ts - only read messages in mxt_acquire_irq() when necessary")
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> 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
