| Message ID | 20121223095450.GA17407@core.coreip.homeip.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Dec 23, 2012 at 01:54:50AM -0800, Dmitry Torokhov wrote: > This is not 100% equivalent transformation as now we schedule first and > disable IRQ later... Anyway, I think the driver shoudl be converted to > threaded IRQ instead. Mark, does the patch below make any sense to you? I'm a bit nervous about the fact that currently both the pen down IRQ and the coordinate read are pushed through a single workqueue so are serialised but after your patch they'll be split into the IRQ thread and the workqueue. It *should* be fine but I'd have to sit there and study it to convince myself that it's safe.
Hello, Dmitry. On Sun, Dec 23, 2012 at 01:54:50AM -0800, Dmitry Torokhov wrote: > This is not 100% equivalent transformation as now we schedule first and > disable IRQ later... Anyway, I think the driver shoudl be converted to > threaded IRQ instead. Mark, does the patch below make any sense to you? Yeah, I think the conversion is actually broken. There isn't anything which prevents work item execution racing against irq disabling. I agree the right thing to do is using threaded irq handler. Thanks!
On Mon, Dec 24, 2012 at 04:18:27PM +0000, Mark Brown wrote: > On Sun, Dec 23, 2012 at 01:54:50AM -0800, Dmitry Torokhov wrote: > > > This is not 100% equivalent transformation as now we schedule first and > > disable IRQ later... Anyway, I think the driver shoudl be converted to > > threaded IRQ instead. Mark, does the patch below make any sense to you? > > I'm a bit nervous about the fact that currently both the pen down IRQ > and the coordinate read are pushed through a single workqueue so are > serialised but after your patch they'll be split into the IRQ thread and > the workqueue. It *should* be fine but I'd have to sit there and study > it to convince myself that it's safe. Mark, Did yo have a chance to review the patch? Thanks!
On Sat, Mar 09, 2013 at 03:53:36PM -0800, Dmitry Torokhov wrote: > On Mon, Dec 24, 2012 at 04:18:27PM +0000, Mark Brown wrote: > > I'm a bit nervous about the fact that currently both the pen down IRQ > > and the coordinate read are pushed through a single workqueue so are > > serialised but after your patch they'll be split into the IRQ thread and > > the workqueue. It *should* be fine but I'd have to sit there and study > > it to convince myself that it's safe. > Mark, > Did yo have a chance to review the patch? > Thanks! Sort of. I'd be much happier keeping them serialised, it's too much work on legacy code to convince myself otherwise.
diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c index 5dbe73a..bf5eddf 100644 --- a/drivers/input/touchscreen/wm97xx-core.c +++ b/drivers/input/touchscreen/wm97xx-core.c @@ -289,11 +289,12 @@ void wm97xx_set_suspend_mode(struct wm97xx *wm, u16 mode) EXPORT_SYMBOL_GPL(wm97xx_set_suspend_mode); /* - * Handle a pen down interrupt. + * Codec PENDOWN irq handler + * */ -static void wm97xx_pen_irq_worker(struct work_struct *work) +static irqreturn_t wm97xx_pen_interrupt(int irq, void *dev_id) { - struct wm97xx *wm = container_of(work, struct wm97xx, pen_event_work); + struct wm97xx *wm = dev_id; int pen_was_down = wm->pen_is_down; /* do we need to enable the touch panel reader */ @@ -347,27 +348,6 @@ static void wm97xx_pen_irq_worker(struct work_struct *work) if (!wm->pen_is_down && wm->mach_ops->acc_enabled) wm->mach_ops->acc_pen_up(wm); - wm->mach_ops->irq_enable(wm, 1); -} - -/* - * Codec PENDOWN irq handler - * - * We have to disable the codec interrupt in the handler because it - * can take up to 1ms to clear the interrupt source. We schedule a task - * in a work queue to do the actual interaction with the chip. The - * interrupt is then enabled again in the slow handler when the source - * has been cleared. - */ -static irqreturn_t wm97xx_pen_interrupt(int irq, void *dev_id) -{ - struct wm97xx *wm = dev_id; - - if (!work_pending(&wm->pen_event_work)) { - wm->mach_ops->irq_enable(wm, 0); - queue_work(wm->ts_workq, &wm->pen_event_work); - } - return IRQ_HANDLED; } @@ -378,12 +358,9 @@ static int wm97xx_init_pen_irq(struct wm97xx *wm) { u16 reg; - /* If an interrupt is supplied an IRQ enable operation must also be - * provided. */ - BUG_ON(!wm->mach_ops->irq_enable); - - if (request_irq(wm->pen_irq, wm97xx_pen_interrupt, IRQF_SHARED, - "wm97xx-pen", wm)) { + if (request_threaded_irq(wm->pen_irq, NULL, wm97xx_pen_interrupt, + IRQF_SHARED | IRQF_ONESHOT, + "wm97xx-pen", wm)) { dev_err(wm->dev, "Failed to register pen down interrupt, polling"); wm->pen_irq = 0; @@ -502,7 +479,6 @@ static int wm97xx_ts_input_open(struct input_dev *idev) wm->codec->dig_enable(wm, 1); INIT_DELAYED_WORK(&wm->ts_reader, wm97xx_ts_reader); - INIT_WORK(&wm->pen_event_work, wm97xx_pen_irq_worker); wm->ts_reader_min_interval = HZ >= 100 ? HZ / 100 : 1; if (wm->ts_reader_min_interval < 1) @@ -553,10 +529,6 @@ static void wm97xx_ts_input_close(struct input_dev *idev) wm->pen_is_down = 0; - /* Balance out interrupt disables/enables */ - if (cancel_work_sync(&wm->pen_event_work)) - wm->mach_ops->irq_enable(wm, 1); - /* ts_reader rearms itself so we need to explicitly stop it * before we destroy the workqueue. */ diff --git a/include/linux/wm97xx.h b/include/linux/wm97xx.h index fd98bb9..12f793d 100644 --- a/include/linux/wm97xx.h +++ b/include/linux/wm97xx.h @@ -280,7 +280,6 @@ struct wm97xx { unsigned long ts_reader_min_interval; /* Minimum interval */ unsigned int pen_irq; /* Pen IRQ number in use */ struct workqueue_struct *ts_workq; - struct work_struct pen_event_work; u16 acc_slot; /* AC97 slot used for acc touch data */ u16 acc_rate; /* acc touch data rate */ unsigned pen_is_down:1; /* Pen is down */