Re: [PATCH 1/2] atmel_tsadcc: Device driver for AT91SAM9RL Touchscreen

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Justin,

On Fri, Apr 25, 2008 at 02:56:37PM -0400, Justin Waters wrote:
> The atmel tsadcc is a touchscreen/adc controller found on the AT91SAM9RL SoC.
> This driver provides basic support for this controller for use as a
> touchscreen.  Some future improvements include suspend/resume capabilities and
> debugfs support.
> 
> Signed-off-by: Justin Waters <justin.waters@timesys.com>

Thank you for the patch. Some comments:

> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 35d4097..3302e27 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -18,4 +18,5 @@ obj-$(CONFIG_TOUCHSCREEN_USB_COMPOSITE)	+= usbtouchscreen.o
>  obj-$(CONFIG_TOUCHSCREEN_PENMOUNT)	+= penmount.o
>  obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT)	+= touchright.o
>  obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN)	+= touchwin.o
> +obj-$(CONFIG_TOUCHSCREEN_ATMEL_TSADCC)	+= atmel_tsadcc.o

If it was sorted alphabetically it would help me.

> +
> +		/* Pen remains down */
> +		atmel_tsadcc_getevent(ts);
> +
> +		input_report_abs(ts->input, ABS_X, ts->event.x);
> +		input_report_abs(ts->input, ABS_Y, ts->event.y);
> +		input_report_abs(ts->input, ABS_PRESSURE, 7500);
> +

The device does not really support pressure reading, please don't try
to provide fake reports. Signal touch with BTN_TOUCH.


> +static int __devexit atmel_tsadcc_remove(struct platform_device *pdev)
> +{
> +	struct atmel_tsadcc *ts = dev_get_drvdata(&pdev->dev);
> +
> +	input_unregister_device(ts->input);
> +
> +	free_irq(ts->irq, ts);

You should free IRQ first, otherwise IRQ may fire just was device is
unregistered and bad things may happed.

Also please run checkpatch on the driver since there are some
formatting and whitespace issues (note that I dont particularly care
about 80 column limit as long as code is readable).

Thank you. 

-- 
Dmitry