| Message ID | 20181220204305.28807-6-marex@denx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | input: touchscreen: ili210x: Add ILI2511 support | expand |
Hi Marek, On Thu, Dec 20, 2018 at 09:43:00PM +0100, Marek Vasut wrote: > Get rid of the workqueue, just spawn a threaded IRQ and handle Not really... > @@ -279,7 +275,6 @@ static int ili210x_i2c_remove(struct i2c_client *client) > struct ili210x *priv = i2c_get_clientdata(client); > > sysfs_remove_group(&client->dev.kobj, &ili210x_attr_group); > - free_irq(priv->client->irq, priv); Now this is unsafe, as interrupt is released after work is cancelled and interrupt may schedule work again. If we can prove that it works, I liked your original change better. Can you try addind sleep of let's say 5 seconds to the interrupt thread just before it returns and see if you get release events reported. The idea is to verify the sequence: - chip raises interrupt line - ISR reads chip state - in response chip makes intterrupt line inactive? - finger leaves the surface - hopefully chip activates interrupt line again - ISR returns - ISR fires again, reports release. > cancel_delayed_work_sync(&priv->dwork); > input_unregister_device(priv->input); > Thanks.
On 12/21/2018 02:44 AM, Dmitry Torokhov wrote: > Hi Marek, Hi, > On Thu, Dec 20, 2018 at 09:43:00PM +0100, Marek Vasut wrote: >> Get rid of the workqueue, just spawn a threaded IRQ and handle > > Not really... Fixed >> @@ -279,7 +275,6 @@ static int ili210x_i2c_remove(struct i2c_client *client) >> struct ili210x *priv = i2c_get_clientdata(client); >> >> sysfs_remove_group(&client->dev.kobj, &ili210x_attr_group); >> - free_irq(priv->client->irq, priv); > > Now this is unsafe, as interrupt is released after work is cancelled and > interrupt may schedule work again. > > If we can prove that it works, I liked your original change better. Can > you try addind sleep of let's say 5 seconds to the interrupt thread > just before it returns and see if you get release events reported. The > idea is to verify the sequence: > > - chip raises interrupt line > - ISR reads chip state > - in response chip makes intterrupt line inactive? No, the IRQ line stays active as long as there's touch event happening (someone has a finger on the touch panel). > - finger leaves the surface > - hopefully chip activates interrupt line again > - ISR returns > - ISR fires again, reports release. > > > >> cancel_delayed_work_sync(&priv->dwork); >> input_unregister_device(priv->input); >> > > Thanks. >
On Fri, Dec 21, 2018 at 03:30:36AM +0100, Marek Vasut wrote: > On 12/21/2018 02:44 AM, Dmitry Torokhov wrote: > > Hi Marek, > > Hi, > > > On Thu, Dec 20, 2018 at 09:43:00PM +0100, Marek Vasut wrote: > >> Get rid of the workqueue, just spawn a threaded IRQ and handle > > > > Not really... > > Fixed > > >> @@ -279,7 +275,6 @@ static int ili210x_i2c_remove(struct i2c_client *client) > >> struct ili210x *priv = i2c_get_clientdata(client); > >> > >> sysfs_remove_group(&client->dev.kobj, &ili210x_attr_group); > >> - free_irq(priv->client->irq, priv); > > > > Now this is unsafe, as interrupt is released after work is cancelled and > > interrupt may schedule work again. > > > > If we can prove that it works, I liked your original change better. Can > > you try addind sleep of let's say 5 seconds to the interrupt thread > > just before it returns and see if you get release events reported. The > > idea is to verify the sequence: > > > > - chip raises interrupt line > > - ISR reads chip state > > - in response chip makes intterrupt line inactive? > > No, the IRQ line stays active as long as there's touch event happening > (someone has a finger on the touch panel). Sorry for asking the same question over and over, but have you verified that the touch controller definitely does not raise interrupt on release (the procedure that I outlined above should help determining that)? Just want to understand exactly the controller behavior. Thanks.
On 12/21/2018 09:30 AM, Dmitry Torokhov wrote: > On Fri, Dec 21, 2018 at 03:30:36AM +0100, Marek Vasut wrote: >> On 12/21/2018 02:44 AM, Dmitry Torokhov wrote: >>> Hi Marek, >> >> Hi, >> >>> On Thu, Dec 20, 2018 at 09:43:00PM +0100, Marek Vasut wrote: >>>> Get rid of the workqueue, just spawn a threaded IRQ and handle >>> >>> Not really... >> >> Fixed >> >>>> @@ -279,7 +275,6 @@ static int ili210x_i2c_remove(struct i2c_client *client) >>>> struct ili210x *priv = i2c_get_clientdata(client); >>>> >>>> sysfs_remove_group(&client->dev.kobj, &ili210x_attr_group); >>>> - free_irq(priv->client->irq, priv); >>> >>> Now this is unsafe, as interrupt is released after work is cancelled and >>> interrupt may schedule work again. >>> >>> If we can prove that it works, I liked your original change better. Can >>> you try addind sleep of let's say 5 seconds to the interrupt thread >>> just before it returns and see if you get release events reported. The >>> idea is to verify the sequence: >>> >>> - chip raises interrupt line >>> - ISR reads chip state >>> - in response chip makes intterrupt line inactive? >> >> No, the IRQ line stays active as long as there's touch event happening >> (someone has a finger on the touch panel). > > Sorry for asking the same question over and over, but have you verified > that the touch controller definitely does not raise interrupt on release > (the procedure that I outlined above should help determining that)? No problem. I had a scope connected to the IRQ line, the line goes LOW on touch-down and high on touch-release. > Just want to understand exactly the controller behavior. Sure > Thanks. >
On Fri, Dec 21, 2018 at 09:42:54PM +0100, Marek Vasut wrote: > On 12/21/2018 09:30 AM, Dmitry Torokhov wrote: > > On Fri, Dec 21, 2018 at 03:30:36AM +0100, Marek Vasut wrote: > >> On 12/21/2018 02:44 AM, Dmitry Torokhov wrote: > >>> Hi Marek, > >> > >> Hi, > >> > >>> On Thu, Dec 20, 2018 at 09:43:00PM +0100, Marek Vasut wrote: > >>>> Get rid of the workqueue, just spawn a threaded IRQ and handle > >>> > >>> Not really... > >> > >> Fixed > >> > >>>> @@ -279,7 +275,6 @@ static int ili210x_i2c_remove(struct i2c_client *client) > >>>> struct ili210x *priv = i2c_get_clientdata(client); > >>>> > >>>> sysfs_remove_group(&client->dev.kobj, &ili210x_attr_group); > >>>> - free_irq(priv->client->irq, priv); > >>> > >>> Now this is unsafe, as interrupt is released after work is cancelled and > >>> interrupt may schedule work again. > >>> > >>> If we can prove that it works, I liked your original change better. Can > >>> you try addind sleep of let's say 5 seconds to the interrupt thread > >>> just before it returns and see if you get release events reported. The > >>> idea is to verify the sequence: > >>> > >>> - chip raises interrupt line > >>> - ISR reads chip state > >>> - in response chip makes intterrupt line inactive? > >> > >> No, the IRQ line stays active as long as there's touch event happening > >> (someone has a finger on the touch panel). > > > > Sorry for asking the same question over and over, but have you verified > > that the touch controller definitely does not raise interrupt on release > > (the procedure that I outlined above should help determining that)? > > No problem. I had a scope connected to the IRQ line, the line goes LOW > on touch-down and high on touch-release. OK, so we definitely need a work or a timer to handle release. Thanks.
On 12/22/2018 02:03 AM, Dmitry Torokhov wrote: > On Fri, Dec 21, 2018 at 09:42:54PM +0100, Marek Vasut wrote: >> On 12/21/2018 09:30 AM, Dmitry Torokhov wrote: >>> On Fri, Dec 21, 2018 at 03:30:36AM +0100, Marek Vasut wrote: >>>> On 12/21/2018 02:44 AM, Dmitry Torokhov wrote: >>>>> Hi Marek, >>>> >>>> Hi, >>>> >>>>> On Thu, Dec 20, 2018 at 09:43:00PM +0100, Marek Vasut wrote: >>>>>> Get rid of the workqueue, just spawn a threaded IRQ and handle >>>>> >>>>> Not really... >>>> >>>> Fixed >>>> >>>>>> @@ -279,7 +275,6 @@ static int ili210x_i2c_remove(struct i2c_client *client) >>>>>> struct ili210x *priv = i2c_get_clientdata(client); >>>>>> >>>>>> sysfs_remove_group(&client->dev.kobj, &ili210x_attr_group); >>>>>> - free_irq(priv->client->irq, priv); >>>>> >>>>> Now this is unsafe, as interrupt is released after work is cancelled and >>>>> interrupt may schedule work again. >>>>> >>>>> If we can prove that it works, I liked your original change better. Can >>>>> you try addind sleep of let's say 5 seconds to the interrupt thread >>>>> just before it returns and see if you get release events reported. The >>>>> idea is to verify the sequence: >>>>> >>>>> - chip raises interrupt line >>>>> - ISR reads chip state >>>>> - in response chip makes intterrupt line inactive? >>>> >>>> No, the IRQ line stays active as long as there's touch event happening >>>> (someone has a finger on the touch panel). >>> >>> Sorry for asking the same question over and over, but have you verified >>> that the touch controller definitely does not raise interrupt on release >>> (the procedure that I outlined above should help determining that)? >> >> No problem. I had a scope connected to the IRQ line, the line goes LOW >> on touch-down and high on touch-release. > > OK, so we definitely need a work or a timer to handle release. Right.
diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c index 322241472b9f..04d75d4ecb20 100644 --- a/drivers/input/touchscreen/ili210x.c +++ b/drivers/input/touchscreen/ili210x.c @@ -5,7 +5,6 @@ #include <linux/input.h> #include <linux/input/mt.h> #include <linux/delay.h> -#include <linux/workqueue.h> #define MAX_TOUCHES 2 #define DEFAULT_POLL_PERIOD 20 @@ -237,19 +236,19 @@ static int ili210x_i2c_probe(struct i2c_client *client, i2c_set_clientdata(client, priv); - error = request_irq(client->irq, ili210x_irq, 0, - client->name, priv); + error = devm_request_threaded_irq(dev, client->irq, NULL, ili210x_irq, + IRQF_ONESHOT, client->name, priv); if (error) { dev_err(dev, "Unable to request touchscreen IRQ, err: %d\n", error); - goto err_free_mem; + return error; } error = sysfs_create_group(&dev->kobj, &ili210x_attr_group); if (error) { dev_err(dev, "Unable to create sysfs attributes, err: %d\n", error); - goto err_free_irq; + return error; } error = input_register_device(priv->input); @@ -268,9 +267,6 @@ static int ili210x_i2c_probe(struct i2c_client *client, err_remove_sysfs: sysfs_remove_group(&dev->kobj, &ili210x_attr_group); -err_free_irq: - free_irq(client->irq, priv); -err_free_mem: return error; } @@ -279,7 +275,6 @@ static int ili210x_i2c_remove(struct i2c_client *client) struct ili210x *priv = i2c_get_clientdata(client); sysfs_remove_group(&client->dev.kobj, &ili210x_attr_group); - free_irq(priv->client->irq, priv); cancel_delayed_work_sync(&priv->dwork); input_unregister_device(priv->input);
Get rid of the workqueue, just spawn a threaded IRQ and handle all the touchscreen readouts in the handler thread. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Cc: Henrik Rydberg <rydberg@bitmath.org> Cc: Olivier Sobrie <olivier@sobrie.be> Cc: Philipp Puschmann <pp@emlix.com> To: linux-input@vger.kernel.org --- V2: Retain workqueue --- drivers/input/touchscreen/ili210x.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)