On Tue, Mar 06, 2012 at 10:25:02AM +0100, Henrik Rydberg wrote:
> > > > +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?
I propose to change the handling of the interrupt by the code below.
#define POLL_PERIOD msecs_to_jiffies(1)
static void ili210x_work(struct work_struct *work)
{
struct ili210x *priv = container_of(work, struct ili210x,
dwork.work);
struct input_dev *input = priv->input;
struct i2c_client *client = priv->client;
struct device *dev = &client->dev;
struct touchdata touchdata;
int rc;
rc = ili210x_read_reg(client, REG_TOUCHDATA, &touchdata,
sizeof(touchdata));
if (rc < 0) {
dev_err(dev, "Unable to get touchdata, err = %d\n",
rc);
return;
}
ili210x_report_events(input, &touchdata);
if ((touchdata.status & 0xf3) || get_pendown_state(priv))
schedule_delayed_work(&priv->dwork, POLL_PERIOD);
}
static irqreturn_t ili210x_irq(int irq, void *irq_data)
{
struct ili210x *priv = irq_data;
schedule_delayed_work(&priv->dwork, 0);
return IRQ_HANDLED;
}
It removes the timer and handle the case of level triggered and edge
triggered interrupts.
What do you think about that ?
Thanks,
--
Olivier Sobrie
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
