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.