Re: [PATCH] ili210x: Add support for Ilitek ILI210x based touchscreens

["Henrik Rydberg" <rydberg@euromail.se>]

> > > +static irqreturn_t ili210x_irq(int irq, void *irq_data)
> > > +{
> > > +	struct ili210x *priv = irq_data;
> > > +	struct i2c_client *client = priv->client;
> > > +	struct input_dev *input = priv->input;
> > > +	struct device *dev = &client->dev;
> > > +	struct touchdata touchdata;
> > > +	int rc;
> > > +
> > > +	do {
> > > +		rc = ili210x_read_reg(client, REG_TOUCHDATA, &touchdata,
> > > +				      sizeof(touchdata));
> > > +		if (rc < 0) {
> > > +			dev_err(dev, "Unable to get touchdata, err = %d\n",
> > > +				rc);
> > > +			goto end;
> > > +		}
> > > +
> > > +		ili210x_report_events(input, &touchdata);
> > > +
> > > +		usleep_range(100, 1000);
> > > +		mod_timer(&priv->timer, jiffies + TS_PEN_UP_TIMEOUT);
> > > +	} while (get_pendown_state(priv) && !priv->stopped);
> > 
> > It looks odd to loop in an irq handler, even if it is threaded. What
> > is the pdata->get_pendown_state() doing?
> I agree. The reason of the loop is for edge triggered interrupt.
> 
> On my hardware I don't have support for level triggered irq. I'm working
> with edge triggered interrupts.
> In the pdata structure I give to the driver it set irq_flags to
> IRQF_TRIGGER_FALLING and the get_pendown_state function implemented is the
> following:
> 	static int get_pendown_state(void)
> 	{
> 		return gpio_get_value(GPIO_TOUCHSCREEN_IRQ) ? 0 : 1;
> 	}
> The get_pendown_state() function will look at the irq line to see if it's
> still low meaning there is a finger on the screen.
> If it is the case, we loop, otherwise we exit from the thread and the timer
> will fire after TS_PEN_UP_TIMEOUT.
> 
> On the contrary if working with a level triggered interrupt, i.e. with
> irqflags set to IRQF_TRIGGER_LOW, there is no need to add the
> get_pendown_state function and the thread will exit after each touch read.
> In this case the usleep_range() is even not needed...
> 
> I agree it's maybe not the best solution... Do I've to use something else
> like a workqueue or something else?

I see, tricky. From the code, it looks like you could skip the loop
and start a workqueue there instead. The work function could then poll
touch frames until the gpio line goes high again. This would also give
you a predictable frame rate without the usleep(). The polldev
implementation might be a good template.

Regarding timer vs workqueue, I am not sure how much could be run in
interrupt context here. You could probably skip the threaded irq
either way, if it is only there to start a poll. Dmitry?

Thanks,
Henrik