| Message ID | 20200331105051.58896-32-jiada_wang@mentor.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | atmel_mxt_ts misc | expand |
31.03.2020 13:50, Jiada Wang пишет: > From: Nick Dyer <nick.dyer@itdev.co.uk> > > The path of enabling the IRQ in the probe function is not safe in level > triggered operation, if it was already powered up and there is a message > waiting on the device (eg finger down) because the object table has not yet > been read. This forces the ISR into a hard loop. > > Delay enabling the interrupt until it is first needed. > > - if (data->use_retrigen_workaround) { > + /* Presence of data->irq means IRQ initialised */ > + data->irq = data->client->irq; IIRC, IRQ=0 could be a valid interrupt since this is a "virtual" interrupt number. ... > init_completion(&data->chg_completion); > @@ -3826,26 +3842,22 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) > return error; > } What about just to tell the IRQ core not to enable the interrupt handling during of the devm_request_threaded_irq()? To achieve that, add this line here: irq_set_status_flags(client->irq, IRQ_NOAUTOEN); > - error = devm_request_threaded_irq(&client->dev, client->irq, > - NULL, mxt_interrupt, IRQF_ONESHOT, > - client->name, data); ... Then the interrupt will be requested in the disabled state and it will be enabled only after the first enable_irq() invocation.
Hi Dmitry On 2020/04/11 7:38, Dmitry Osipenko wrote: > 31.03.2020 13:50, Jiada Wang пишет: >> From: Nick Dyer <nick.dyer@itdev.co.uk> >> >> The path of enabling the IRQ in the probe function is not safe in level >> triggered operation, if it was already powered up and there is a message >> waiting on the device (eg finger down) because the object table has not yet >> been read. This forces the ISR into a hard loop. >> >> Delay enabling the interrupt until it is first needed. > > >> >> - if (data->use_retrigen_workaround) { >> + /* Presence of data->irq means IRQ initialised */ >> + data->irq = data->client->irq; > > IIRC, IRQ=0 could be a valid interrupt since this is a "virtual" > interrupt number. > > ... >> init_completion(&data->chg_completion); >> @@ -3826,26 +3842,22 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) >> return error; >> } > > What about just to tell the IRQ core not to enable the interrupt > handling during of the devm_request_threaded_irq()? > > To achieve that, add this line here: > > irq_set_status_flags(client->irq, IRQ_NOAUTOEN); > >> - error = devm_request_threaded_irq(&client->dev, client->irq, >> - NULL, mxt_interrupt, IRQF_ONESHOT, >> - client->name, data); > ... > > Then the interrupt will be requested in the disabled state and it will > be enabled only after the first enable_irq() invocation. > thanks, I think your solution makes more sense, I will replace with your suggested solution in next version Thanks, Jiada
15.04.2020 17:44, Wang, Jiada пишет: .. > thanks, I think your solution makes more sense, > I will replace with your suggested solution in next version Please feel free to CC me on the next version.
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c index 62f11afb26ae..c26470bcb38b 100644 --- a/drivers/input/touchscreen/atmel_mxt_ts.c +++ b/drivers/input/touchscreen/atmel_mxt_ts.c @@ -1448,9 +1448,24 @@ static int mxt_acquire_irq(struct mxt_data *data) { int error; - enable_irq(data->irq); + if (!data->irq) { + error = devm_request_threaded_irq(&data->client->dev, + data->client->irq, + NULL, mxt_interrupt, + IRQF_ONESHOT, + data->client->name, data); + if (error) { + dev_err(&data->client->dev, "Error requesting irq\n"); + return error; + } - if (data->use_retrigen_workaround) { + /* Presence of data->irq means IRQ initialised */ + data->irq = data->client->irq; + } else { + enable_irq(data->irq); + } + + if (data->object_table && data->use_retrigen_workaround) { error = mxt_process_messages_until_invalid(data); if (error) return error; @@ -3370,7 +3385,9 @@ static int mxt_load_fw(struct device *dev) goto release_firmware; } - enable_irq(data->irq); + ret = mxt_acquire_irq(data); + if (ret) + goto release_firmware; /* Poll after 0.1s if no interrupt received */ schedule_delayed_work(&data->flash->work, msecs_to_jiffies(100)); @@ -3798,7 +3815,6 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) client->adapter->nr, client->addr); data->client = client; - data->irq = client->irq; i2c_set_clientdata(client, data); init_completion(&data->chg_completion); @@ -3826,26 +3842,22 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) return error; } - error = devm_request_threaded_irq(&client->dev, client->irq, - NULL, mxt_interrupt, IRQF_ONESHOT, - client->name, data); - if (error) { - dev_err(&client->dev, "Failed to register interrupt\n"); - return error; - } - if (data->suspend_mode == MXT_SUSPEND_REGULATOR) { + error = mxt_acquire_irq(data); + if (error) + return error; + error = mxt_probe_regulators(data); if (error) return error; + + disable_irq(data->irq); } else if (data->reset_gpio) { msleep(MXT_RESET_GPIO_TIME); gpiod_set_value(data->reset_gpio, 1); msleep(MXT_RESET_INVALID_CHG); } - disable_irq(data->irq); - error = mxt_initialize(data); if (error) return error;