| Message ID | 20201128123720.929948-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Input: atmel_mxt_ts - Fix lost interrupts | expand |
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.
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
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; > 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. Ah, I see. Thank you.
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, Andre
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.
On Tue, Dec 1, 2020 at 7:31 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > 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. Sure I sent a new version like this! It's certainly more elegant and it works fine for me too. Yours, Linus Walleij
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; - 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);
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. 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(-)