| Message ID | 20181213151442.27854-4-marex@denx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | input: touchscreen: ili210x: Add ILI2511 support | expand |
On Thu, Dec 13, 2018 at 7:15 AM Marek Vasut <marex@denx.de> wrote: > > Get rid of the workqueue, just spawn a threaded IRQ and handle > all the touchscreen readouts in the handler thread. So we reliably get interrupt on release? Also this probably means that clients have to use level interrupts.... Thanks.
On 12/13/2018 08:08 PM, Dmitry Torokhov wrote: > On Thu, Dec 13, 2018 at 7:15 AM Marek Vasut <marex@denx.de> wrote: >> >> Get rid of the workqueue, just spawn a threaded IRQ and handle >> all the touchscreen readouts in the handler thread. > > So we reliably get interrupt on release? What am I missing here , can you elaborate a bit ? > Also this probably means that clients have to use level interrupts.... The interrupt line of the ILI2511 is indeed level triggered (pen/finger down means IRQ line goes down, pen/finger up means it goes up).
On Thu, Dec 13, 2018 at 4:44 PM Marek Vasut <marex@denx.de> wrote: > > On 12/13/2018 08:08 PM, Dmitry Torokhov wrote: > > On Thu, Dec 13, 2018 at 7:15 AM Marek Vasut <marex@denx.de> wrote: > >> > >> Get rid of the workqueue, just spawn a threaded IRQ and handle > >> all the touchscreen readouts in the handler thread. > > > > So we reliably get interrupt on release? > > What am I missing here , can you elaborate a bit ? If I'm reading the old code correctly it would be OK even if interrupt was only delivered on initial touch, and then we'd be simply polling the device... > > > Also this probably means that clients have to use level interrupts.... > > The interrupt line of the ILI2511 is indeed level triggered (pen/finger > down means IRQ line goes down, pen/finger up means it goes up). So what happens when pen goes up just as you are leaving the ISR? The controller would clear interrupt, but you already reported touch and would not report release. The controller needs to make sure that it keeps the line low even after you removed pen/finger, until you read controller state once again so you can reliably report touch release. Does this make sense? Thanks.
On 12/14/2018 02:17 AM, Dmitry Torokhov wrote: > On Thu, Dec 13, 2018 at 4:44 PM Marek Vasut <marex@denx.de> wrote: >> >> On 12/13/2018 08:08 PM, Dmitry Torokhov wrote: >>> On Thu, Dec 13, 2018 at 7:15 AM Marek Vasut <marex@denx.de> wrote: >>>> >>>> Get rid of the workqueue, just spawn a threaded IRQ and handle >>>> all the touchscreen readouts in the handler thread. >>> >>> So we reliably get interrupt on release? >> >> What am I missing here , can you elaborate a bit ? > > If I'm reading the old code correctly it would be OK even if interrupt > was only delivered on initial touch, and then we'd be simply polling > the device... Sure, although that's not how the controller behaves, so do we care about retaining that behavior ? >> >>> Also this probably means that clients have to use level interrupts.... >> >> The interrupt line of the ILI2511 is indeed level triggered (pen/finger >> down means IRQ line goes down, pen/finger up means it goes up). > > So what happens when pen goes up just as you are leaving the ISR? The > controller would clear interrupt, but you already reported touch and > would not report release. The controller needs to make sure that it > keeps the line low even after you removed pen/finger, until you read > controller state once again so you can reliably report touch release. > > Does this make sense? Yeah, that makes sense, so we probably want to retain the workqueue afterall ? Or is there a better option ?
diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c index 7ba93de712432..1102ee560bf4d 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 @@ -43,9 +42,6 @@ struct firmware_version { struct ili210x { struct i2c_client *client; struct input_dev *input; - bool (*get_pendown_state)(void); - unsigned int poll_period; - struct delayed_work dwork; }; static int ili210x_read_reg(struct i2c_client *client, u8 reg, void *buf, @@ -102,20 +98,9 @@ static void ili210x_report_events(struct input_dev *input, input_sync(input); } -static bool get_pendown_state(const struct ili210x *priv) -{ - bool state = false; - - if (priv->get_pendown_state) - state = priv->get_pendown_state(); - - return state; -} - -static void ili210x_work(struct work_struct *work) +static irqreturn_t ili210x_irq(int irq, void *irq_data) { - struct ili210x *priv = container_of(work, struct ili210x, - dwork.work); + struct ili210x *priv = irq_data; struct i2c_client *client = priv->client; struct touchdata touchdata; int error; @@ -125,21 +110,10 @@ static void ili210x_work(struct work_struct *work) if (error) { dev_err(&client->dev, "Unable to get touchdata, err = %d\n", error); - return; + return IRQ_HANDLED; } - ili210x_report_events(priv->input, &touchdata); - - if ((touchdata.status & 0xf3) || get_pendown_state(priv)) - schedule_delayed_work(&priv->dwork, - msecs_to_jiffies(priv->poll_period)); -} - -static irqreturn_t ili210x_irq(int irq, void *irq_data) -{ - struct ili210x *priv = irq_data; - - schedule_delayed_work(&priv->dwork, 0); + ili210x_report_events(priv, &touchdata); return IRQ_HANDLED; } @@ -224,8 +198,6 @@ static int ili210x_i2c_probe(struct i2c_client *client, priv->client = client; priv->input = input; - priv->poll_period = DEFAULT_POLL_PERIOD; - INIT_DELAYED_WORK(&priv->dwork, ili210x_work); /* Setup input device */ input->name = "ILI210x Touchscreen"; @@ -248,19 +220,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); @@ -279,19 +251,12 @@ 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; } 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); return 0;
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> To: linux-input@vger.kernel.org --- drivers/input/touchscreen/ili210x.c | 51 +++++------------------------ 1 file changed, 8 insertions(+), 43 deletions(-)