Re: [PATCH 1/2]input - wacom_w8001: Support pen or touch only devices

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

Hi Ping,

On Wed, Dec 08, 2010 at 05:17:13PM -0800, Ping Cheng wrote:
> Not all penabled devices support touch. The same idea is true for
> touch devices. Setting up the devices according to the specific
> querying results.
> 
> Signed-off-by: Ping Cheng <pingc@wacom.com>
> ---
>  drivers/input/touchscreen/wacom_w8001.c |   39 ++++++++++++++++++++++++------
>  1 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
> index 361d00a..90b92e8 100644
> --- a/drivers/input/touchscreen/wacom_w8001.c
> +++ b/drivers/input/touchscreen/wacom_w8001.c
> @@ -37,6 +37,7 @@ MODULE_LICENSE("GPL");
>  
>  #define W8001_QUERY_PACKET	0x20
>  
> +#define W8001_CMD_STOP		'0'
>  #define W8001_CMD_START		'1'
>  #define W8001_CMD_QUERY		'*'
>  #define W8001_CMD_TOUCHQUERY	'%'
> @@ -279,24 +280,46 @@ static int w8001_setup(struct w8001 *w8001)
>  	struct w8001_coord coord;
>  	int error;
>  
> -	error = w8001_command(w8001, W8001_CMD_QUERY, true);
> +	error = w8001_command(w8001, W8001_CMD_STOP, false);

Isn't there any response from the device to W8001_CMD_STOP?

>  	if (error)
>  		return error;
>  
> -	parse_data(w8001->response, &coord);
> +	mdelay(250);	/* wait 250ms before querying the device */
>  
> -	input_set_abs_params(dev, ABS_X, 0, coord.x, 0, 0);
> -	input_set_abs_params(dev, ABS_Y, 0, coord.y, 0, 0);
> -	input_set_abs_params(dev, ABS_PRESSURE, 0, coord.pen_pressure, 0, 0);
> -	input_set_abs_params(dev, ABS_TILT_X, 0, coord.tilt_x, 0, 0);
> -	input_set_abs_params(dev, ABS_TILT_Y, 0, coord.tilt_y, 0, 0);
> +	/* penabled? */
> +	error = w8001_command(w8001, W8001_CMD_QUERY, true);
> +	if (!error) {
> +		dev->keybit[BIT_WORD(BTN_TOOL_PEN)] |= BIT_MASK(BTN_TOOL_PEN);
> +		dev->keybit[BIT_WORD(BTN_TOOL_RUBBER)] |= BIT_MASK(BTN_TOOL_RUBBER);
> +		dev->keybit[BIT_WORD(BTN_STYLUS)] |= BIT_MASK(BTN_STYLUS);
> +		dev->keybit[BIT_WORD(BTN_STYLUS2)] |= BIT_MASK(BTN_STYLUS2);

Could we start switching to __set_bit(BTN_TOOL_PEN, dev->keybit) style?
The original way was somewhat OK when setting several buttons at once
but I feel __set_bit() form is much safer.

> +		parse_data(w8001->response, &coord);
> +
> +		input_set_abs_params(dev, ABS_X, 0, coord.x, 0, 0);
> +		input_set_abs_params(dev, ABS_Y, 0, coord.y, 0, 0);
> +		input_set_abs_params(dev, ABS_PRESSURE, 0, coord.pen_pressure, 0, 0);
> +		if (coord.tilt_x && coord.tilt_y)
> +		{

Formatting: opening brace goes on the same as the statement.

> +			input_set_abs_params(dev, ABS_TILT_X, 0, coord.tilt_x, 0, 0);
> +			input_set_abs_params(dev, ABS_TILT_Y, 0, coord.tilt_y, 0, 0);
> +		}
> +	}
>  
> +	/* touch enabled? */
>  	error = w8001_command(w8001, W8001_CMD_TOUCHQUERY, true);
> -	if (!error) {
> +
> +	/* Some non-touch devices may reply to the touch query. But their
> +	 * second byte is empty, which indicates touch is not supported.
> +	 */
> +	if (!error && w8001->response[1]) {
>  		struct w8001_touch_query touch;
>  
>  		parse_touchquery(w8001->response, &touch);
>  
> +		input_set_abs_params(dev, ABS_X, 0, touch.x, 0, 0);
> +		input_set_abs_params(dev, ABS_Y, 0, touch.y, 0, 0);
> +		dev->keybit[BIT_WORD(BTN_TOOL_FINGER)] |= BIT_MASK(BTN_TOOL_FINGER);
> +
>  		switch (touch.sensor_id) {
>  		case 0:
>  		case 2:
> -- 
> 1.7.2.3
> 

Thanks.

-- 
Dmitry