* [PATCH 1/2] Input: wm831x-ts - Ensure the controller is in a known state on open @ 2011-03-14 22:45 Mark Brown 2011-03-14 22:45 ` [PATCH 2/2] Input: wm831x-ts - Fix races with IRQ management Mark Brown 2011-03-16 6:31 ` [PATCH 1/2] Input: wm831x-ts - Ensure the controller is in a known state on open Dmitry Torokhov 0 siblings, 2 replies; 6+ messages in thread From: Mark Brown @ 2011-03-14 22:45 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-input, patches, Mark Brown Explicitly set all the enable bits when opening the device just in case something left the device in an unexpected state. Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> --- drivers/input/touchscreen/wm831x-ts.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/drivers/input/touchscreen/wm831x-ts.c b/drivers/input/touchscreen/wm831x-ts.c index 3db0c29..6ae054f 100644 --- a/drivers/input/touchscreen/wm831x-ts.c +++ b/drivers/input/touchscreen/wm831x-ts.c @@ -167,7 +167,9 @@ static int wm831x_ts_input_open(struct input_dev *idev) struct wm831x *wm831x = wm831x_ts->wm831x; wm831x_set_bits(wm831x, WM831X_TOUCH_CONTROL_1, - WM831X_TCH_ENA, WM831X_TCH_ENA); + WM831X_TCH_ENA | WM831X_TCH_CVT_ENA | + WM831X_TCH_X_ENA | WM831X_TCH_Y_ENA | + WM831X_TCH_Z_ENA, WM831X_TCH_ENA); wm831x_set_bits(wm831x, WM831X_TOUCH_CONTROL_1, WM831X_TCH_CVT_ENA, WM831X_TCH_CVT_ENA); -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] Input: wm831x-ts - Fix races with IRQ management 2011-03-14 22:45 [PATCH 1/2] Input: wm831x-ts - Ensure the controller is in a known state on open Mark Brown @ 2011-03-14 22:45 ` Mark Brown 2011-03-18 11:20 ` Mark Brown 2011-03-16 6:31 ` [PATCH 1/2] Input: wm831x-ts - Ensure the controller is in a known state on open Dmitry Torokhov 1 sibling, 1 reply; 6+ messages in thread From: Mark Brown @ 2011-03-14 22:45 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-input, patches, Mark Brown If the WM831x pen down and data IRQs run in parallel it is possible for the data and pen down IRQs to deadlock themselves as one is part way through disabling its operation while the other is part way through enabling. Fix this by always disabling the pen down interrupt while data is active and vice versa. This also fixes an issue when using the built in IRQs due to enable_irq() not being valid from interrupt context on an interrupt controller with bus operations like the built in IRQ controller - this issue may also have affected other interrupt controllers. Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> --- This revision reworks the close() implementation, hopefully more robustly, which should address the main thrust of your comments previously. As I said in my previous e-mail the restrictions on enable_irq() seem reasonable to me, the thing that's nasty here is that we need to enable and disable the IRQs at all. drivers/input/touchscreen/wm831x-ts.c | 60 ++++++++++++++++++++++++++++++-- 1 files changed, 56 insertions(+), 4 deletions(-) diff --git a/drivers/input/touchscreen/wm831x-ts.c b/drivers/input/touchscreen/wm831x-ts.c index 6ae054f..3f825cb 100644 --- a/drivers/input/touchscreen/wm831x-ts.c +++ b/drivers/input/touchscreen/wm831x-ts.c @@ -68,8 +68,29 @@ struct wm831x_ts { unsigned int pd_irq; bool pressure; bool pen_down; + struct work_struct pd_data_work; + struct work_struct data_pd_work; }; +static void wm831x_pd_data(struct work_struct *work) +{ + struct wm831x_ts *wm831x_ts = + container_of(work, struct wm831x_ts, pd_data_work); + + disable_irq(wm831x_ts->pd_irq); + enable_irq(wm831x_ts->data_irq); + dev_dbg(wm831x_ts->wm831x->dev, "IRQ PD->DATA done\n"); +} + +static void wm831x_data_pd(struct work_struct *work) +{ + struct wm831x_ts *wm831x_ts = + container_of(work, struct wm831x_ts, data_pd_work); + + enable_irq(wm831x_ts->pd_irq); + dev_dbg(wm831x_ts->wm831x->dev, "IRQ DATA->PD done\n"); +} + static irqreturn_t wm831x_ts_data_irq(int irq, void *irq_data) { struct wm831x_ts *wm831x_ts = irq_data; @@ -110,7 +131,10 @@ static irqreturn_t wm831x_ts_data_irq(int irq, void *irq_data) } if (!wm831x_ts->pen_down) { + /* Switch from data to pen down */ + dev_dbg(wm831x->dev, "IRQ DATA->PD\n"); disable_irq_nosync(wm831x_ts->data_irq); + schedule_work(&wm831x_ts->data_pd_work); /* Don't need data any more */ wm831x_set_bits(wm831x, WM831X_TOUCH_CONTROL_1, @@ -156,7 +180,10 @@ static irqreturn_t wm831x_ts_pen_down_irq(int irq, void *irq_data) WM831X_TCHPD_EINT, WM831X_TCHPD_EINT); wm831x_ts->pen_down = true; - enable_irq(wm831x_ts->data_irq); + + /* Switch from pen down to data */ + dev_dbg(wm831x->dev, "IRQ PD->DATA\n"); + schedule_work(&wm831x_ts->pd_data_work); return IRQ_HANDLED; } @@ -182,13 +209,36 @@ static void wm831x_ts_input_close(struct input_dev *idev) struct wm831x_ts *wm831x_ts = input_get_drvdata(idev); struct wm831x *wm831x = wm831x_ts->wm831x; + /* Shut the controller down, disabling all other functionality */ + wm831x_set_bits(wm831x, WM831X_TOUCH_CONTROL_1, + WM831X_TCH_ENA, 0); + + /* Make sure any pending IRQs are done, the above will prevent + * new ones firing. + */ + synchronize_irq(wm831x_ts->data_irq); + synchronize_irq(wm831x_ts->pd_irq); + + /* Make sure the IRQ completion work is quiesced */ + flush_work_sync(&wm831x_ts->data_pd_work); + flush_work_sync(&wm831x_ts->pd_data_work); + + /* If we ended up with the pen down then make sure we revert back + * to pen detection state for the next time we start up. + */ + if (wm831x_ts->pen_down) { + disable_irq(wm831x_ts->data_irq); + enable_irq(wm831x_ts->pd_irq); + wm831x_ts->pen_down = false; + } + + /* Be nice to the next user and make sure everything is + * flagged as disabled. + */ wm831x_set_bits(wm831x, WM831X_TOUCH_CONTROL_1, WM831X_TCH_ENA | WM831X_TCH_CVT_ENA | WM831X_TCH_X_ENA | WM831X_TCH_Y_ENA | WM831X_TCH_Z_ENA, 0); - - if (wm831x_ts->pen_down) - disable_irq(wm831x_ts->data_irq); } static __devinit int wm831x_ts_probe(struct platform_device *pdev) @@ -212,6 +262,8 @@ static __devinit int wm831x_ts_probe(struct platform_device *pdev) wm831x_ts->wm831x = wm831x; wm831x_ts->input_dev = input_dev; + INIT_WORK(&wm831x_ts->pd_data_work, wm831x_pd_data); + INIT_WORK(&wm831x_ts->data_pd_work, wm831x_data_pd); /* * If we have a direct IRQ use it, otherwise use the interrupt -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] Input: wm831x-ts - Fix races with IRQ management 2011-03-14 22:45 ` [PATCH 2/2] Input: wm831x-ts - Fix races with IRQ management Mark Brown @ 2011-03-18 11:20 ` Mark Brown 2011-03-25 7:30 ` Dmitry Torokhov 0 siblings, 1 reply; 6+ messages in thread From: Mark Brown @ 2011-03-18 11:20 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-input, patches On Mon, Mar 14, 2011 at 10:45:01PM +0000, Mark Brown wrote: > This revision reworks the close() implementation, hopefully more > robustly, which should address the main thrust of your comments > previously. As I said in my previous e-mail the restrictions on > enable_irq() seem reasonable to me, the thing that's nasty here is that > we need to enable and disable the IRQs at all. It'd be good to get something for this into 2.6.39. At the very least this version of the patch avoids issues while the device is open and keeps any issues in close() which is an improvement. If this approach isn't going to be OK please let me know so I can look into alternatives. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] Input: wm831x-ts - Fix races with IRQ management 2011-03-18 11:20 ` Mark Brown @ 2011-03-25 7:30 ` Dmitry Torokhov 2011-03-25 10:49 ` Mark Brown 0 siblings, 1 reply; 6+ messages in thread From: Dmitry Torokhov @ 2011-03-25 7:30 UTC (permalink / raw) To: Mark Brown; +Cc: linux-input, patches On Fri, Mar 18, 2011 at 11:20:44AM +0000, Mark Brown wrote: > On Mon, Mar 14, 2011 at 10:45:01PM +0000, Mark Brown wrote: > > > This revision reworks the close() implementation, hopefully more > > robustly, which should address the main thrust of your comments > > previously. As I said in my previous e-mail the restrictions on > > enable_irq() seem reasonable to me, the thing that's nasty here is that > > we need to enable and disable the IRQs at all. > > It'd be good to get something for this into 2.6.39. At the very least > this version of the patch avoids issues while the device is open and > keeps any issues in close() which is an improvement. If this approach > isn't going to be OK please let me know so I can look into alternatives. Mark, Sorry for the delay. I am just not comfortable with a touchcsreen requiring essentially 2 threads to operate (2 IRQ threads, 2 works). I was thinking about converting to a thread with a state machine and 2 non-threaded IRQs disabling themselves and waking the thread up. Do you think something like that would work? Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] Input: wm831x-ts - Fix races with IRQ management 2011-03-25 7:30 ` Dmitry Torokhov @ 2011-03-25 10:49 ` Mark Brown 0 siblings, 0 replies; 6+ messages in thread From: Mark Brown @ 2011-03-25 10:49 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-input, patches On Fri, Mar 25, 2011 at 12:30:32AM -0700, Dmitry Torokhov wrote: > I am just not comfortable with a touchcsreen requiring essentially 2 > threads to operate (2 IRQ threads, 2 works). I was thinking about > converting to a thread with a state machine and 2 non-threaded IRQs > disabling themselves and waking the thread up. Do you think something > like that would work? You can't use non-threaded IRQs as the chip is accessed over I2C or SPI so when using the built in interrupts you're in thread context before you've decoded the IRQ. Given the need to ack the IRQ to stop it trying to do something it doesn't seem worth trying to move the chip interaction out of the IRQ handlers there - it'd just be performance pain and you'd still need to worry about which IRQs are enabled at any given moment since you'd need to disable the IRQs if you wanted to defer handling them. Combining the two work items to do the IRQ enable handovers should be doable, though. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] Input: wm831x-ts - Ensure the controller is in a known state on open 2011-03-14 22:45 [PATCH 1/2] Input: wm831x-ts - Ensure the controller is in a known state on open Mark Brown 2011-03-14 22:45 ` [PATCH 2/2] Input: wm831x-ts - Fix races with IRQ management Mark Brown @ 2011-03-16 6:31 ` Dmitry Torokhov 1 sibling, 0 replies; 6+ messages in thread From: Dmitry Torokhov @ 2011-03-16 6:31 UTC (permalink / raw) To: Mark Brown; +Cc: linux-input, patches On Mon, Mar 14, 2011 at 10:45:00PM +0000, Mark Brown wrote: > Explicitly set all the enable bits when opening the device just in case > something left the device in an unexpected state. > Applied, thank you Mark. -- Dmitry ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-03-25 10:49 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-03-14 22:45 [PATCH 1/2] Input: wm831x-ts - Ensure the controller is in a known state on open Mark Brown 2011-03-14 22:45 ` [PATCH 2/2] Input: wm831x-ts - Fix races with IRQ management Mark Brown 2011-03-18 11:20 ` Mark Brown 2011-03-25 7:30 ` Dmitry Torokhov 2011-03-25 10:49 ` Mark Brown 2011-03-16 6:31 ` [PATCH 1/2] Input: wm831x-ts - Ensure the controller is in a known state on open Dmitry Torokhov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.