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 <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
> Tested-by: Julien Boibessot <julien.boibessot@xxxxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
