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@xxxxxxxxx> 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. -- Dmitry
