Re: [PATCH 3/4 v3] input - wacom : support one finger touch the touchscreen way

["Henrik Rydberg" <rydberg@euromail.se>]

Hi Ping,

I think it is getting there, just a couple of things inline.

On Tue, Mar 01, 2011 at 04:18:43PM -0800, Ping Cheng wrote:
> There are two types of 1FGT devices supported in wacom_wac.c.
> Changing them to follow the existing touchscreen format, i.e.,
> only report BTN_TOUCH as a valid tool type.
> 
> Touch data will be ignored if pen is in proximity. This requires
> a touch up event sent if touch was down when pen comes in. The
> touch up event should be sent before any pen events are emitted.
> Otherwise, two pointers would race for the cursor.
> 
> However, we can not send a touch up inside wacom_tpc_pen since
> pen and touch are on different logical port. That is why we
> have to check if touch is up before sending pen events.
> 
> Reviewed-by: Chris Bagwell <chris@cnpbagwell.com>
> Signed-off-by: Ping Cheng <pingc@wacom.com>
> ---
> v3: Simplify the pen and touch logical flow to avoid manual state
> change.
> ---
>  drivers/input/tablet/wacom_wac.c |  130 ++++++++++++++-----------------------
>  drivers/input/tablet/wacom_wac.h |    1 +
>  2 files changed, 50 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/input/tablet/wacom_wac.c b/drivers/input/tablet/wacom_wac.c
> index 15bab99..ebbb375 100644
> --- a/drivers/input/tablet/wacom_wac.c
> +++ b/drivers/input/tablet/wacom_wac.c
> @@ -675,51 +675,37 @@ static int wacom_intuos_irq(struct wacom_wac *wacom)
>  	return 1;
>  }
>  
> -static void wacom_tpc_touch_out(struct wacom_wac *wacom, int idx)
> -{
> -	struct input_dev *input = wacom->input;
> -	int finger = idx + 1;
> -
> -	input_report_abs(input, ABS_X, 0);
> -	input_report_abs(input, ABS_Y, 0);
> -	input_report_abs(input, ABS_MISC, 0);
> -	input_report_key(input, wacom->tool[finger], 0);
> -	if (!idx)
> -		input_report_key(input, BTN_TOUCH, 0);
> -	input_event(input, EV_MSC, MSC_SERIAL, finger);
> -	input_sync(input);
> -}
> -
> -static void wacom_tpc_touch_in(struct wacom_wac *wacom, size_t len)
> +static int wacom_tpc_single_touch(struct wacom_wac *wacom, size_t len)
>  {
>  	char *data = wacom->data;
>  	struct input_dev *input = wacom->input;
> +	bool prox = 0;

Initialization not needed. Alternatively, the else-clause below can be dropped.

> +	int x = 0, y = 0;
>  
> -	wacom->tool[1] = BTN_TOOL_DOUBLETAP;
> -	wacom->id[0] = TOUCH_DEVICE_ID;
> +	if (!wacom->shared->stylus_in_proximity) {
> +		if (len == WACOM_PKGLEN_TPC1FG) {
> +			prox = data[0] & 0x01;
> +			x = get_unaligned_le16(&data[1]);
> +			y = get_unaligned_le16(&data[3]);
> +		} else { /* with capacity */
> +			prox = data[1] & 0x01;
> +			x = le16_to_cpup((__le16 *)&data[2]);
> +			y = le16_to_cpup((__le16 *)&data[4]);
> +		}
> +	} else
> +		/* force touch out when pen is in prox */
> +		prox = 0;
>  
> -	if (len != WACOM_PKGLEN_TPC1FG) {
> +	if (prox) {
> +		input_report_abs(input, ABS_X, x);
> +		input_report_abs(input, ABS_Y, y);
> +	}
> +	input_report_key(input, BTN_TOUCH, prox);
>  
> -		switch (data[0]) {
> +	/* keep touch state for pen events */
> +	wacom->shared->touch_down = prox;
>  
> -		case WACOM_REPORT_TPC1FG:
> -			input_report_abs(input, ABS_X, le16_to_cpup((__le16 *)&data[2]));
> -			input_report_abs(input, ABS_Y, le16_to_cpup((__le16 *)&data[4]));
> -			input_report_abs(input, ABS_PRESSURE, le16_to_cpup((__le16 *)&data[6]));
> -			input_report_key(input, BTN_TOUCH, le16_to_cpup((__le16 *)&data[6]));
> -			input_report_abs(input, ABS_MISC, wacom->id[0]);
> -			input_report_key(input, wacom->tool[1], 1);
> -			input_sync(input);
> -			break;
> -		}
> -	} else {
> -		input_report_abs(input, ABS_X, get_unaligned_le16(&data[1]));
> -		input_report_abs(input, ABS_Y, get_unaligned_le16(&data[3]));
> -		input_report_key(input, BTN_TOUCH, 1);
> -		input_report_abs(input, ABS_MISC, wacom->id[1]);
> -		input_report_key(input, wacom->tool[1], 1);
> -		input_sync(input);
> -	}
> +	return 1;
>  }
>  
>  static int wacom_tpc_pen(struct wacom_wac *wacom)
> @@ -731,7 +717,7 @@ static int wacom_tpc_pen(struct wacom_wac *wacom)
>  
>  	prox = data[1] & 0x20;
>  
> -	if (!wacom->shared->stylus_in_proximity) { /* first in prox */
> +	if (!wacom->shared->stylus_in_proximity) {

Unrelated change, please drop.

>  		/* Going into proximity select tool */
>  		wacom->tool[0] = (data[1] & 0x0c) ? BTN_TOOL_RUBBER : BTN_TOOL_PEN;
>  		if (wacom->tool[0] == BTN_TOOL_PEN)
> @@ -741,57 +727,38 @@ static int wacom_tpc_pen(struct wacom_wac *wacom)
>  
>  		wacom->shared->stylus_in_proximity = true;
>  	}
> -	input_report_key(input, BTN_STYLUS, data[1] & 0x02);
> -	input_report_key(input, BTN_STYLUS2, data[1] & 0x10);
> -	input_report_abs(input, ABS_X, le16_to_cpup((__le16 *)&data[2]));
> -	input_report_abs(input, ABS_Y, le16_to_cpup((__le16 *)&data[4]));
> -	pressure = ((data[7] & 0x01) << 8) | data[6];
> -	if (pressure < 0)
> -		pressure = features->pressure_max + pressure + 1;
> -	input_report_abs(input, ABS_PRESSURE, pressure);
> -	input_report_key(input, BTN_TOUCH, data[1] & 0x05);
> -	if (!prox) { /* out-prox */
> -		wacom->id[0] = 0;
> -		wacom->shared->stylus_in_proximity = false;
> +
> +	/* send pen events only when touch is up or touch are forced out */
> +	if (!wacom->shared->touch_down) {
> +		input_report_key(input, BTN_STYLUS, data[1] & 0x02);
> +		input_report_key(input, BTN_STYLUS2, data[1] & 0x10);
> +		input_report_abs(input, ABS_X, le16_to_cpup((__le16 *)&data[2]));
> +		input_report_abs(input, ABS_Y, le16_to_cpup((__le16 *)&data[4]));
> +		pressure = ((data[7] & 0x01) << 8) | data[6];
> +		if (pressure < 0)
> +			pressure = features->pressure_max + pressure + 1;
> +		input_report_abs(input, ABS_PRESSURE, pressure);
> +		input_report_key(input, BTN_TOUCH, data[1] & 0x05);
> +		if (!prox) { /* out-prox */
> +			wacom->id[0] = 0;
> +			wacom->shared->stylus_in_proximity = false;
> +		}

It seems the if-clause here should be unrelated to the touch_down
status. Please move out to the main branch.

> +		input_report_key(input, wacom->tool[0], prox);
> +		return 1;
>  	}
> -	input_report_key(input, wacom->tool[0], prox);
>  
> -	return 1;
> +	return 0;
>  }
>  
>  static int wacom_tpc_irq(struct wacom_wac *wacom, size_t len)
>  {
>  	char *data = wacom->data;
> -	int prox = 0;
>  
>  	dbg("wacom_tpc_irq: received report #%d", data[0]);
>  
> -	if (len == WACOM_PKGLEN_TPC1FG ||
> -	    data[0] == WACOM_REPORT_TPC1FG) {	/* single touch */
> -
> -		if (wacom->shared->stylus_in_proximity) {
> -			if (wacom->id[1] & 0x01)
> -				wacom_tpc_touch_out(wacom, 0);
> -
> -			wacom->id[1] = 0;
> -			return 0;
> -		}
> -
> -		if (len == WACOM_PKGLEN_TPC1FG)
> -			prox = data[0] & 0x01;
> -		else  /* with capacity */
> -			prox = data[1] & 0x01;
> -
> -		if (prox)
> -			wacom_tpc_touch_in(wacom, len);
> -		else {
> -			wacom_tpc_touch_out(wacom, 0);
> -
> -			wacom->id[0] = 0;
> -		}
> -		/* keep prox bit to send proper out-prox event */
> -		wacom->id[1] = prox;
> -	} else if (data[0] == WACOM_REPORT_PENABLED)
> +	if (len == WACOM_PKGLEN_TPC1FG || data[0] == WACOM_REPORT_TPC1FG)
> +		return wacom_tpc_single_touch(wacom, len);
> +	else if (data[0] == WACOM_REPORT_PENABLED)
>  		return wacom_tpc_pen(wacom);
>  
>  	return 0;
> @@ -1172,6 +1139,8 @@ void wacom_setup_input_capabilities(struct input_dev *input_dev,
>  		/* fall through */
>  
>  	case TABLETPC:
> +		__clear_bit(ABS_MISC, input_dev->absbit);
> +
>  		if (features->device_type == BTN_TOOL_DOUBLETAP ||
>  		    features->device_type == BTN_TOOL_TRIPLETAP) {
>  			input_abs_set_res(input_dev, ABS_X,
> @@ -1180,7 +1149,6 @@ void wacom_setup_input_capabilities(struct input_dev *input_dev,
>  			input_abs_set_res(input_dev, ABS_Y,
>  				wacom_calculate_touch_res(features->y_max,
>  							features->y_phy));
> -			__set_bit(BTN_TOOL_DOUBLETAP, input_dev->keybit);
>  		}
>  
>  		if (features->device_type != BTN_TOOL_PEN)
> diff --git a/drivers/input/tablet/wacom_wac.h b/drivers/input/tablet/wacom_wac.h
> index 8f747dd..835f756 100644
> --- a/drivers/input/tablet/wacom_wac.h
> +++ b/drivers/input/tablet/wacom_wac.h
> @@ -88,6 +88,7 @@ struct wacom_features {
>  
>  struct wacom_shared {
>  	bool stylus_in_proximity;
> +	bool touch_down;
>  };
>  
>  struct wacom_wac {
> -- 
> 1.7.4

Thanks,
Henrik