[V2,05/10] input: touchscreen: ili210x: Convert to threaded IRQ
diff mbox series

Message ID 20181220204305.28807-6-marex@denx.de
State New
Headers show
Series
  • input: touchscreen: ili210x: Add ILI2511 support
Related show

Commit Message

Marek Vasut Dec. 20, 2018, 8:43 p.m. UTC
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(-)

Comments

Dmitry Torokhov Dec. 21, 2018, 1:44 a.m. UTC | #1
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.
Marek Vasut Dec. 21, 2018, 2:30 a.m. UTC | #2
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.
>
Dmitry Torokhov Dec. 21, 2018, 8:30 a.m. UTC | #3
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.
Marek Vasut Dec. 21, 2018, 8:42 p.m. UTC | #4
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.
>
Dmitry Torokhov Dec. 22, 2018, 1:03 a.m. UTC | #5
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.
Marek Vasut Dec. 22, 2018, 2:02 a.m. UTC | #6
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.

Patch
diff mbox series

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);