[PATCH 1/2] Input: wm831x-ts - Ensure the controller is in a known state on open

[PATCH 1/2] Input: wm831x-ts - Ensure the controller is in a known state on open

[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

[PATCH 2/2] Input: wm831x-ts - Fix races with IRQ management

[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

Re: [PATCH 1/2] Input: wm831x-ts - Ensure the controller is in a known state on open

[Dmitry Torokhov]

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

Re: [PATCH 2/2] Input: wm831x-ts - Fix races with IRQ management

[Mark Brown]

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.

Re: [PATCH 2/2] Input: wm831x-ts - Fix races with IRQ management

[Dmitry Torokhov]

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

Re: [PATCH 2/2] Input: wm831x-ts - Fix races with IRQ management

[Mark Brown]

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.