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

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

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