Re: [PATCH] Input: wm831x - Add driver for Wolfson WM831x PMIC touchscreen controllers

On Tue, Jan 25, 2011 at 11:50:19AM -0800, Dmitry Torokhov wrote:

On Tue, Jan 25, 2011 at 07:34:23PM +0000, Mark Brown wrote:

quoted

On Tue, Jan 25, 2011 at 09:07:29AM -0800, Dmitry Torokhov wrote:

quoted

On Tue, Jan 25, 2011 at 02:09:16PM +0000, Mark Brown wrote:

quoted

quoted

+	int data_irq;
+	int pd_irq;

quoted

Maybe make irqs unsigned?

I guess.

quoted

quoted

+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;
+
+	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_nosync(wm831x_ts->data_irq);

quoted

Why nosync?

Cut'n'paste from the pen up handling in the data IRQ (which obviously
does need to be nosync due to the fact that it runs from IRQ context).

quoted

quoted

+	input_dev = input_allocate_device();

quoted

Are we 100% sure it is impossible for the 2 irqs above to fire up before
we get here? Like if we happen to have hardware in half-active state
while binding?

It's spectacularly unlikely - in the sort of hardware this device is
useful for nothing below Linux would realistically want to talk to the
touchscreen and since the IRQs are threaded they can't be shared.

Would you please move input allocation still - even if it works fine
with your device the code might be used as an example by other
authors... and there should be no penalties for your driver if you
allocate just a tad earlier.

Ah, you did already. Thank you.

-- 
Dmitry