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

From: Linus Walleij <linus.walleij@linaro.org>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Linux Input <linux-input@vger.kernel.org>,
	Andre <andre.muller@web.de>, Nick Dyer <nick.dyer@itdev.co.uk>,
	Jiada Wang <jiada_wang@mentor.com>,
	stable <stable@vger.kernel.org>
Subject: Re: [PATCH] Input: atmel_mxt_ts - Fix lost interrupts
Date: Sun, 29 Nov 2020 22:13:06 +0100	[thread overview]
Message-ID: <CACRpkdY8r5_EYAtWLL2vZQ8ULf6rG-VfgDe=7oveJQwiRXxTNg@mail.gmail.com> (raw)
In-Reply-To: <20201129025328.GH2034289@dtor-ws>

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