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

Hi Mark,

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

Some of the WM831x series of PMICs from Wolfson Microelectronics include
a resistive touchscreen controller. Implement support for these controllers
within the input API.

Platform data is supported to allow configuration of system parameters such
as selection between four and five wire touchscreens and for specification
of optional direct to CPU IRQs for sample availability and for pen down.
Use of this feature for at least the data IRQ is strongly recommended.

Thanks to Julien Boibessot for extensive testing and detailed feedback.

Signed-off-by: Mark Brown <redacted>
Tested-by: Julien Boibessot <redacted>

Just a couple small concerns here:

+
+struct wm831x_ts {
+	struct input_dev *input_dev;
+	struct wm831x *wm831x;
+	int data_irq;
+	int pd_irq;

Maybe make irqs unsigned?

+
+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);

Why nosync?

+
+	ret = request_threaded_irq(wm831x_ts->data_irq, NULL,
+				   wm831x_ts_data_irq,
+				   IRQF_ONESHOT,
+				   "Touchscreen data", wm831x_ts);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to request data IRQ %d: %d\n",
+			wm831x_ts->data_irq, ret);
+		goto err_alloc;
+	}
+	disable_irq(wm831x_ts->data_irq);
+
+	ret = request_threaded_irq(wm831x_ts->pd_irq, NULL,
+				   wm831x_ts_pen_down_irq, IRQF_ONESHOT,
+				   "Touchscreen pen down", wm831x_ts);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to request pen down IRQ %d: %d\n",
+			wm831x_ts->pd_irq, ret);
+		goto err_data_irq;
+	}
+
+	platform_set_drvdata(pdev, wm831x_ts);
+
+	input_dev = input_allocate_device();

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?

I'd feel safer if we allocated input device earlier, together with the
touchscreen structure.

Thanks.

-- 
Dmitry