[PATCH 2/2]input - wacom_w8001: Add one finger touch support

[PATCH 2/2]input - wacom_w8001: Add one finger touch support

[Ping Cheng]

Signed-off-by: Ping Cheng <pingc@wacom.com>
---
 drivers/input/touchscreen/wacom_w8001.c |   74 +++++++++++++++++++++++++++++--
 1 files changed, 70 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
index 90b92e8..68087d8 100644
--- a/drivers/input/touchscreen/wacom_w8001.c
+++ b/drivers/input/touchscreen/wacom_w8001.c
@@ -3,6 +3,7 @@
  *
  * Copyright (c) 2008 Jaya Kumar
  * Copyright (c) 2010 Red Hat, Inc.
+ * Copyright (c) 2010 Ping Cheng, Wacom. <pingc@wacom.com>
  *
  * This file is subject to the terms and conditions of the GNU General Public
  * License. See the file COPYING in the main directory of this archive for
@@ -86,6 +87,12 @@ struct w8001 {
 	char phys[32];
 	int type;
 	unsigned int pktlen;
+	bool pen_in_prox;
+	bool has_touch;
+	int max_touch_x;
+	int max_touch_y;
+	int max_pen_x;
+	int max_pen_y;
 };
 
 static void parse_data(u8 *data, struct w8001_coord *coord)
@@ -112,6 +119,29 @@ static void parse_data(u8 *data, struct w8001_coord *coord)
 	coord->tilt_y = data[8] & 0x7F;
 }
 
+static void parse_single_touch(struct w8001 *w8001)
+{
+	struct input_dev *dev = w8001->dev;
+	unsigned char *data = w8001->data;
+
+	int x = (data[1] << 7) | data[2];
+	int y = (data[3] << 7) | data[4];
+	w8001->has_touch = data[0] & 0x1;
+
+	/* scale to pen maximum */
+	if (w8001->max_pen_x && w8001->max_pen_y && w8001->max_touch_x) {
+		x = x * w8001->max_pen_x / w8001->max_touch_x;
+		y = y * w8001->max_pen_y / w8001->max_touch_y;
+	}
+
+	input_report_abs(dev, ABS_X, x);
+	input_report_abs(dev, ABS_Y, y);
+	input_report_key(dev, BTN_TOUCH, w8001->has_touch);
+	input_report_key(dev, BTN_TOOL_FINGER, w8001->has_touch);
+
+	input_sync(dev);
+}
+
 static void parse_touch(struct w8001 *w8001)
 {
 	struct input_dev *dev = w8001->dev;
@@ -151,6 +181,18 @@ static void parse_touchquery(u8 *data, struct w8001_touch_query *query)
 	query->y = data[5] << 9;
 	query->y |= data[6] << 2;
 	query->y |= (data[2] >> 3) & 0x3;
+
+	/* Early days' one finger touch models need the following defaults */
+	if (!query->x && !query->y) {
+		query->x = 1024;
+		query->y = 1024;
+		query->panel_res = 10;
+		query->panel_res = 10;
+		if (data[1]) {
+			query->x = (1 << data[1]);
+			query->y = (1 << data[1]);
+		}
+	}
 }
 
 static void report_pen_events(struct w8001 *w8001, struct w8001_coord *coord)
@@ -199,6 +241,7 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
 				   unsigned char data, unsigned int flags)
 {
 	struct w8001 *w8001 = serio_get_drvdata(serio);
+	struct input_dev *dev = w8001->dev;
 	struct w8001_coord coord;
 	unsigned char tmp;
 
@@ -213,9 +256,15 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
 
 	case W8001_PKTLEN_TOUCH93 - 1:
 	case W8001_PKTLEN_TOUCH9A - 1:
-		/* ignore one-finger touch packet. */
-		if (w8001->pktlen == w8001->idx)
+		tmp = (w8001->data[0] & W8001_TOUCH_BYTE);
+		if (tmp != W8001_TOUCH_BYTE)
+			break;
+
+		if (w8001->pktlen == w8001->idx) {
 			w8001->idx = 0;
+			if (!w8001->pen_in_prox)
+				parse_single_touch(w8001);
+		}
 		break;
 
 	/* Pen coordinates packet */
@@ -228,9 +277,17 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
 		if (tmp == W8001_TOUCH_BYTE)
 			break;
 
+		if (w8001->has_touch) {
+			/* send touch data out */
+			w8001->has_touch = 0;
+			input_report_key(dev, BTN_TOUCH, 0);
+			input_report_key(dev, BTN_TOOL_FINGER, 0);
+		}
+
 		w8001->idx = 0;
 		parse_data(w8001->data, &coord);
 		report_pen_events(w8001, &coord);
+		w8001->pen_in_prox = coord.rdy ? true : false;
 		break;
 
 	/* control packet */
@@ -313,11 +370,20 @@ static int w8001_setup(struct w8001 *w8001)
 	 */
 	if (!error && w8001->response[1]) {
 		struct w8001_touch_query touch;
+		int px, py;
 
 		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);
+		px = w8001->max_touch_x = touch.x;
+		py = w8001->max_touch_x = touch.y;
+
+		/* scale to pen maximum */
+		if (coord.x && coord.y) {
+			px = w8001->max_pen_x = coord.x;
+			py = w8001->max_pen_y = coord.y;
+		}
+		input_set_abs_params(dev, ABS_X, 0, px, 0, 0);
+		input_set_abs_params(dev, ABS_Y, 0, py, 0, 0);
 		dev->keybit[BIT_WORD(BTN_TOOL_FINGER)] |= BIT_MASK(BTN_TOOL_FINGER);
 
 		switch (touch.sensor_id) {
-- 
1.7.2.3

Re: [PATCH 2/2]input - wacom_w8001: Add one finger touch support

[Dmitry Torokhov]

On Wed, Dec 08, 2010 at 05:23:49PM -0800, Ping Cheng wrote:
> Signed-off-by: Ping Cheng <pingc@wacom.com>
> ---
>  drivers/input/touchscreen/wacom_w8001.c |   74 +++++++++++++++++++++++++++++--
>  1 files changed, 70 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
> index 90b92e8..68087d8 100644
> --- a/drivers/input/touchscreen/wacom_w8001.c
> +++ b/drivers/input/touchscreen/wacom_w8001.c
> @@ -3,6 +3,7 @@
>   *
>   * Copyright (c) 2008 Jaya Kumar
>   * Copyright (c) 2010 Red Hat, Inc.
> + * Copyright (c) 2010 Ping Cheng, Wacom. <pingc@wacom.com>
>   *
>   * This file is subject to the terms and conditions of the GNU General Public
>   * License. See the file COPYING in the main directory of this archive for
> @@ -86,6 +87,12 @@ struct w8001 {
>  	char phys[32];
>  	int type;
>  	unsigned int pktlen;
> +	bool pen_in_prox;
> +	bool has_touch;
> +	int max_touch_x;
> +	int max_touch_y;
> +	int max_pen_x;
> +	int max_pen_y;

This does not apply to my tree. What tree was it made against?

-- 
Dmitry

Re: [PATCH 2/2]input - wacom_w8001: Add one finger touch support

[Chris Bagwell]

On Wed, Dec 8, 2010 at 7:23 PM, Ping Cheng <pinglinux@gmail.com> wrote:
> Signed-off-by: Ping Cheng <pingc@wacom.com>
> ---
>  drivers/input/touchscreen/wacom_w8001.c |   74 +++++++++++++++++++++++++++++--
>  1 files changed, 70 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
> index 90b92e8..68087d8 100644
> --- a/drivers/input/touchscreen/wacom_w8001.c
> +++ b/drivers/input/touchscreen/wacom_w8001.c
> @@ -3,6 +3,7 @@
>  *
>  * Copyright (c) 2008 Jaya Kumar
>  * Copyright (c) 2010 Red Hat, Inc.
> + * Copyright (c) 2010 Ping Cheng, Wacom. <pingc@wacom.com>
>  *
>  * This file is subject to the terms and conditions of the GNU General Public
>  * License. See the file COPYING in the main directory of this archive for
> @@ -86,6 +87,12 @@ struct w8001 {
>        char phys[32];
>        int type;
>        unsigned int pktlen;
> +       bool pen_in_prox;
> +       bool has_touch;
> +       int max_touch_x;
> +       int max_touch_y;
> +       int max_pen_x;
> +       int max_pen_y;
>  };
>
>  static void parse_data(u8 *data, struct w8001_coord *coord)
> @@ -112,6 +119,29 @@ static void parse_data(u8 *data, struct w8001_coord *coord)
>        coord->tilt_y = data[8] & 0x7F;
>  }
>
> +static void parse_single_touch(struct w8001 *w8001)
> +{
> +       struct input_dev *dev = w8001->dev;
> +       unsigned char *data = w8001->data;
> +
> +       int x = (data[1] << 7) | data[2];
> +       int y = (data[3] << 7) | data[4];
> +       w8001->has_touch = data[0] & 0x1;
> +
> +       /* scale to pen maximum */
> +       if (w8001->max_pen_x && w8001->max_pen_y && w8001->max_touch_x) {
> +               x = x * w8001->max_pen_x / w8001->max_touch_x;
> +               y = y * w8001->max_pen_y / w8001->max_touch_y;

Ok, you are doing scaling.

> +       }
> +
> +       input_report_abs(dev, ABS_X, x);
> +       input_report_abs(dev, ABS_Y, y);
> +       input_report_key(dev, BTN_TOUCH, w8001->has_touch);
> +       input_report_key(dev, BTN_TOOL_FINGER, w8001->has_touch);

Wacom seems good about report x/y=0 when !has_touch but its probably
safer to enforce that if this can be a combo device.  More below.

> +
> +       input_sync(dev);
> +}
> +
>  static void parse_touch(struct w8001 *w8001)
>  {
>        struct input_dev *dev = w8001->dev;
> @@ -151,6 +181,18 @@ static void parse_touchquery(u8 *data, struct w8001_touch_query *query)
>        query->y = data[5] << 9;
>        query->y |= data[6] << 2;
>        query->y |= (data[2] >> 3) & 0x3;
> +
> +       /* Early days' one finger touch models need the following defaults */
> +       if (!query->x && !query->y) {
> +               query->x = 1024;
> +               query->y = 1024;
> +               query->panel_res = 10;
> +               query->panel_res = 10;
> +               if (data[1]) {
> +                       query->x = (1 << data[1]);
> +                       query->y = (1 << data[1]);
> +               }
> +       }
>  }
>
>  static void report_pen_events(struct w8001 *w8001, struct w8001_coord *coord)
> @@ -199,6 +241,7 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
>                                   unsigned char data, unsigned int flags)
>  {
>        struct w8001 *w8001 = serio_get_drvdata(serio);
> +       struct input_dev *dev = w8001->dev;
>        struct w8001_coord coord;
>        unsigned char tmp;
>
> @@ -213,9 +256,15 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
>
>        case W8001_PKTLEN_TOUCH93 - 1:
>        case W8001_PKTLEN_TOUCH9A - 1:
> -               /* ignore one-finger touch packet. */
> -               if (w8001->pktlen == w8001->idx)
> +               tmp = (w8001->data[0] & W8001_TOUCH_BYTE);
> +               if (tmp != W8001_TOUCH_BYTE)
> +                       break;
> +
> +               if (w8001->pktlen == w8001->idx) {
>                        w8001->idx = 0;
> +                       if (!w8001->pen_in_prox)
> +                               parse_single_touch(w8001);
> +               }
>                break;
>
>        /* Pen coordinates packet */
> @@ -228,9 +277,17 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
>                if (tmp == W8001_TOUCH_BYTE)
>                        break;
>
> +               if (w8001->has_touch) {
> +                       /* send touch data out */
> +                       w8001->has_touch = 0;
> +                       input_report_key(dev, BTN_TOUCH, 0);
> +                       input_report_key(dev, BTN_TOOL_FINGER, 0);

Probably its better to set ABS_X/ABS_Y to zero and do a sync here?  So
duplicate x/y values don't get dropped and aligns with wacom_wac.c.
This is related to comment about forcing ABS_X/Y to zero above.  Its
so pen has known starting point when coming in proximity.  I wouldn't
do one without the other.

Chris

> +               }
> +
>                w8001->idx = 0;
>                parse_data(w8001->data, &coord);
>                report_pen_events(w8001, &coord);
> +               w8001->pen_in_prox = coord.rdy ? true : false;
>                break;
>
>        /* control packet */
> @@ -313,11 +370,20 @@ static int w8001_setup(struct w8001 *w8001)
>         */
>        if (!error && w8001->response[1]) {
>                struct w8001_touch_query touch;
> +               int px, py;
>
>                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);
> +               px = w8001->max_touch_x = touch.x;
> +               py = w8001->max_touch_x = touch.y;
> +
> +               /* scale to pen maximum */
> +               if (coord.x && coord.y) {
> +                       px = w8001->max_pen_x = coord.x;
> +                       py = w8001->max_pen_y = coord.y;
> +               }
> +               input_set_abs_params(dev, ABS_X, 0, px, 0, 0);
> +               input_set_abs_params(dev, ABS_Y, 0, py, 0, 0);

I see.  My previous comments are addressed... except for MT comment.
Should those be disabled or does sensor_id take care of that?

>                dev->keybit[BIT_WORD(BTN_TOOL_FINGER)] |= BIT_MASK(BTN_TOOL_FINGER);
>
>                switch (touch.sensor_id) {
> --
> 1.7.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2]input - wacom_w8001: Add one finger touch support

[Ping Cheng]

On Wed, Dec 8, 2010 at 10:44 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Dec 08, 2010 at 05:23:49PM -0800, Ping Cheng wrote:
>> Signed-off-by: Ping Cheng <pingc@wacom.com>
>> ---
>>  drivers/input/touchscreen/wacom_w8001.c |   74 +++++++++++++++++++++++++++++--
>>  1 files changed, 70 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
>> index 90b92e8..68087d8 100644
>> --- a/drivers/input/touchscreen/wacom_w8001.c
>> +++ b/drivers/input/touchscreen/wacom_w8001.c
>> @@ -3,6 +3,7 @@
>>   *
>>   * Copyright (c) 2008 Jaya Kumar
>>   * Copyright (c) 2010 Red Hat, Inc.
>> + * Copyright (c) 2010 Ping Cheng, Wacom. <pingc@wacom.com>
>>   *
>>   * This file is subject to the terms and conditions of the GNU General Public
>>   * License. See the file COPYING in the main directory of this archive for
>> @@ -86,6 +87,12 @@ struct w8001 {
>>       char phys[32];
>>       int type;
>>       unsigned int pktlen;
>> +     bool pen_in_prox;
>> +     bool has_touch;
>> +     int max_touch_x;
>> +     int max_touch_y;
>> +     int max_pen_x;
>> +     int max_pen_y;
>
> This does not apply to my tree. What tree was it made against?

It was based on Henrik's mt-next branch since there are MT support in
the driver. Should I use yours for v2?

Thank you.

Ping
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2]input - wacom_w8001: Add one finger touch support

[Dmitry Torokhov]

On Thursday, December 09, 2010 09:39:09 am Ping Cheng wrote:
> On Wed, Dec 8, 2010 at 10:44 PM, Dmitry Torokhov
> 
> <dmitry.torokhov@gmail.com> wrote:
> > On Wed, Dec 08, 2010 at 05:23:49PM -0800, Ping Cheng wrote:
> >> Signed-off-by: Ping Cheng <pingc@wacom.com>
> >> ---
> >>  drivers/input/touchscreen/wacom_w8001.c |   74
> >> +++++++++++++++++++++++++++++-- 1 files changed, 70 insertions(+), 4
> >> deletions(-)
> >> 
> >> diff --git a/drivers/input/touchscreen/wacom_w8001.c
> >> b/drivers/input/touchscreen/wacom_w8001.c index 90b92e8..68087d8 100644
> >> --- a/drivers/input/touchscreen/wacom_w8001.c
> >> +++ b/drivers/input/touchscreen/wacom_w8001.c
> >> @@ -3,6 +3,7 @@
> >>   *
> >>   * Copyright (c) 2008 Jaya Kumar
> >>   * Copyright (c) 2010 Red Hat, Inc.
> >> + * Copyright (c) 2010 Ping Cheng, Wacom. <pingc@wacom.com>
> >>   *
> >>   * This file is subject to the terms and conditions of the GNU General
> >> Public * License. See the file COPYING in the main directory of this
> >> archive for @@ -86,6 +87,12 @@ struct w8001 {
> >>       char phys[32];
> >>       int type;
> >>       unsigned int pktlen;
> >> +     bool pen_in_prox;
> >> +     bool has_touch;
> >> +     int max_touch_x;
> >> +     int max_touch_y;
> >> +     int max_pen_x;
> >> +     int max_pen_y;
> > 
> > This does not apply to my tree. What tree was it made against?
> 
> It was based on Henrik's mt-next branch since there are MT support in
> the driver. Should I use yours for v2?

Now that I have merged Henrik's it should not matter but generally it would
be helpful to specify which tree the patch was made against (if not mine)
so I can plan the merging accordingly.

Thanks.

-- 
Dmitry

Re: [PATCH 2/2]input - wacom_w8001: Add one finger touch support

[Ping Cheng]

On Thu, Dec 9, 2010 at 7:06 AM, Chris Bagwell <chris@cnpbagwell.com> wrote:
> On Wed, Dec 8, 2010 at 7:23 PM, Ping Cheng <pinglinux@gmail.com> wrote:
>> Signed-off-by: Ping Cheng <pingc@wacom.com>
>> ---
>>  drivers/input/touchscreen/wacom_w8001.c |   74 +++++++++++++++++++++++++++++--
>>  1 files changed, 70 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
>> index 90b92e8..68087d8 100644
>> --- a/drivers/input/touchscreen/wacom_w8001.c
>> +++ b/drivers/input/touchscreen/wacom_w8001.c
>> @@ -3,6 +3,7 @@
>>  *
>>  * Copyright (c) 2008 Jaya Kumar
>>  * Copyright (c) 2010 Red Hat, Inc.
>> + * Copyright (c) 2010 Ping Cheng, Wacom. <pingc@wacom.com>
>>  *
>>  * This file is subject to the terms and conditions of the GNU General Public
>>  * License. See the file COPYING in the main directory of this archive for
>> @@ -86,6 +87,12 @@ struct w8001 {
>>        char phys[32];
>>        int type;
>>        unsigned int pktlen;
>> +       bool pen_in_prox;
>> +       bool has_touch;
>> +       int max_touch_x;
>> +       int max_touch_y;
>> +       int max_pen_x;
>> +       int max_pen_y;
>>  };
>>
>>  static void parse_data(u8 *data, struct w8001_coord *coord)
>> @@ -112,6 +119,29 @@ static void parse_data(u8 *data, struct w8001_coord *coord)
>>        coord->tilt_y = data[8] & 0x7F;
>>  }
>>
>> +static void parse_single_touch(struct w8001 *w8001)
>> +{
>> +       struct input_dev *dev = w8001->dev;
>> +       unsigned char *data = w8001->data;
>> +
>> +       int x = (data[1] << 7) | data[2];
>> +       int y = (data[3] << 7) | data[4];
>> +       w8001->has_touch = data[0] & 0x1;
>> +
>> +       /* scale to pen maximum */
>> +       if (w8001->max_pen_x && w8001->max_pen_y && w8001->max_touch_x) {
>> +               x = x * w8001->max_pen_x / w8001->max_touch_x;
>> +               y = y * w8001->max_pen_y / w8001->max_touch_y;
>
> Ok, you are doing scaling.
>
>> +       }
>> +
>> +       input_report_abs(dev, ABS_X, x);
>> +       input_report_abs(dev, ABS_Y, y);
>> +       input_report_key(dev, BTN_TOUCH, w8001->has_touch);
>> +       input_report_key(dev, BTN_TOOL_FINGER, w8001->has_touch);
>
> Wacom seems good about report x/y=0 when !has_touch but its probably
> safer to enforce that if this can be a combo device.  More below.

I can do that although it's not necessary, especially only for (x,y).
We don't emit motion events once the finger is up or the tool is
out-prox.

>> +
>> +       input_sync(dev);
>> +}
>> +
>>  static void parse_touch(struct w8001 *w8001)
>>  {
>>        struct input_dev *dev = w8001->dev;
>> @@ -151,6 +181,18 @@ static void parse_touchquery(u8 *data, struct w8001_touch_query *query)
>>        query->y = data[5] << 9;
>>        query->y |= data[6] << 2;
>>        query->y |= (data[2] >> 3) & 0x3;
>> +
>> +       /* Early days' one finger touch models need the following defaults */
>> +       if (!query->x && !query->y) {
>> +               query->x = 1024;
>> +               query->y = 1024;
>> +               query->panel_res = 10;
>> +               query->panel_res = 10;
>> +               if (data[1]) {
>> +                       query->x = (1 << data[1]);
>> +                       query->y = (1 << data[1]);
>> +               }
>> +       }
>>  }
>>
>>  static void report_pen_events(struct w8001 *w8001, struct w8001_coord *coord)
>> @@ -199,6 +241,7 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
>>                                   unsigned char data, unsigned int flags)
>>  {
>>        struct w8001 *w8001 = serio_get_drvdata(serio);
>> +       struct input_dev *dev = w8001->dev;
>>        struct w8001_coord coord;
>>        unsigned char tmp;
>>
>> @@ -213,9 +256,15 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
>>
>>        case W8001_PKTLEN_TOUCH93 - 1:
>>        case W8001_PKTLEN_TOUCH9A - 1:
>> -               /* ignore one-finger touch packet. */
>> -               if (w8001->pktlen == w8001->idx)
>> +               tmp = (w8001->data[0] & W8001_TOUCH_BYTE);
>> +               if (tmp != W8001_TOUCH_BYTE)
>> +                       break;
>> +
>> +               if (w8001->pktlen == w8001->idx) {
>>                        w8001->idx = 0;
>> +                       if (!w8001->pen_in_prox)
>> +                               parse_single_touch(w8001);
>> +               }
>>                break;
>>
>>        /* Pen coordinates packet */
>> @@ -228,9 +277,17 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
>>                if (tmp == W8001_TOUCH_BYTE)
>>                        break;
>>
>> +               if (w8001->has_touch) {
>> +                       /* send touch data out */
>> +                       w8001->has_touch = 0;
>> +                       input_report_key(dev, BTN_TOUCH, 0);
>> +                       input_report_key(dev, BTN_TOOL_FINGER, 0);
>
> Probably its better to set ABS_X/ABS_Y to zero and do a sync here?  So
> duplicate x/y values don't get dropped and aligns with wacom_wac.c.
> This is related to comment about forcing ABS_X/Y to zero above.  Its
> so pen has known starting point when coming in proximity.  I wouldn't
> do one without the other.

I'll do both to make you happy (just kidding, to make it safe ;).

>
>> +               }
>> +
>>                w8001->idx = 0;
>>                parse_data(w8001->data, &coord);
>>                report_pen_events(w8001, &coord);
>> +               w8001->pen_in_prox = coord.rdy ? true : false;
>>                break;
>>
>>        /* control packet */
>> @@ -313,11 +370,20 @@ static int w8001_setup(struct w8001 *w8001)
>>         */
>>        if (!error && w8001->response[1]) {
>>                struct w8001_touch_query touch;
>> +               int px, py;
>>
>>                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);
>> +               px = w8001->max_touch_x = touch.x;
>> +               py = w8001->max_touch_x = touch.y;
>> +
>> +               /* scale to pen maximum */
>> +               if (coord.x && coord.y) {
>> +                       px = w8001->max_pen_x = coord.x;
>> +                       py = w8001->max_pen_y = coord.y;
>> +               }
>> +               input_set_abs_params(dev, ABS_X, 0, px, 0, 0);
>> +               input_set_abs_params(dev, ABS_Y, 0, py, 0, 0);
>
> I see.  My previous comments are addressed... except for MT comment.
> Should those be disabled or does sensor_id take care of that?

sensor_id takes care of it.

Thank you for reviewing.

Ping

>>                dev->keybit[BIT_WORD(BTN_TOOL_FINGER)] |= BIT_MASK(BTN_TOOL_FINGER);
>>
>>                switch (touch.sensor_id) {
>> --
>> 1.7.2.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2]input - wacom_w8001: Add one finger touch support

[Dmitry Torokhov]

On Thu, Dec 09, 2010 at 11:36:19AM -0800, Ping Cheng wrote:
> On Thu, Dec 9, 2010 at 7:06 AM, Chris Bagwell <chris@cnpbagwell.com> wrote:
> > On Wed, Dec 8, 2010 at 7:23 PM, Ping Cheng <pinglinux@gmail.com> wrote:
> >> @@ -228,9 +277,17 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
> >>                if (tmp == W8001_TOUCH_BYTE)
> >>                        break;
> >>
> >> +               if (w8001->has_touch) {
> >> +                       /* send touch data out */
> >> +                       w8001->has_touch = 0;
> >> +                       input_report_key(dev, BTN_TOUCH, 0);
> >> +                       input_report_key(dev, BTN_TOOL_FINGER, 0);
> >
> > Probably its better to set ABS_X/ABS_Y to zero and do a sync here?  So
> > duplicate x/y values don't get dropped and aligns with wacom_wac.c.
> > This is related to comment about forcing ABS_X/Y to zero above.  Its
> > so pen has known starting point when coming in proximity.  I wouldn't
> > do one without the other.
> 
> I'll do both to make you happy (just kidding, to make it safe ;).
> 

Actually do not see how (0,0) is any safer than let's say (123,78)
sincve i believe (0,0) is a valid coordinate. Userspace should still
hang on to the last reported coordinate and use it if it did not get
a new one.

Also, if you add input_sync() here won't it cause (in certain
situtations) false click or tap events - BTN_TOUCH goes from 1 to 0 and
then again to 1 if pen is already in proximity...

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2]input - wacom_w8001: Add one finger touch support

[Chris Bagwell]

On Thu, Dec 9, 2010 at 1:50 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Dec 09, 2010 at 11:36:19AM -0800, Ping Cheng wrote:
>> On Thu, Dec 9, 2010 at 7:06 AM, Chris Bagwell <chris@cnpbagwell.com> wrote:
>> > On Wed, Dec 8, 2010 at 7:23 PM, Ping Cheng <pinglinux@gmail.com> wrote:
>> >> @@ -228,9 +277,17 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
>> >>                if (tmp == W8001_TOUCH_BYTE)
>> >>                        break;
>> >>
>> >> +               if (w8001->has_touch) {
>> >> +                       /* send touch data out */
>> >> +                       w8001->has_touch = 0;
>> >> +                       input_report_key(dev, BTN_TOUCH, 0);
>> >> +                       input_report_key(dev, BTN_TOOL_FINGER, 0);
>> >
>> > Probably its better to set ABS_X/ABS_Y to zero and do a sync here?  So
>> > duplicate x/y values don't get dropped and aligns with wacom_wac.c.
>> > This is related to comment about forcing ABS_X/Y to zero above.  Its
>> > so pen has known starting point when coming in proximity.  I wouldn't
>> > do one without the other.
>>
>> I'll do both to make you happy (just kidding, to make it safe ;).
>>
>
> Actually do not see how (0,0) is any safer than let's say (123,78)
> sincve i believe (0,0) is a valid coordinate. Userspace should still
> hang on to the last reported coordinate and use it if it did not get
> a new one.
>
> Also, if you add input_sync() here won't it cause (in certain
> situtations) false click or tap events - BTN_TOUCH goes from 1 to 0 and
> then again to 1 if pen is already in proximity...

The patches behavior happens to align good with existing wacom
behavior and user land apps (xf86-input-wacom mostly) but its good to
get this input from those not concentrating on wacom as much.  It
helps remind me the old way is not the only way.

Let me answer the second point first.

The above code is following a kinda rule that other wacom tablet
drivers obey.  That rule is that if 2 tools share ABS_* events then
only 1 of those tools can be in proximity at one time.  If a driver
follows that rule then you will not get the unwanted clicks you
mention.

The 1 tool in proximity probably originates from physical attribute of
having to flip pen to get eraser or switching to another physical
stylus.  I think it wasn't until touch+stylus that HW supported 2
tools in proximity with same ABS_*.  The above logic comes in to
simulate older HW enforced behavior.  There is some user land code I
know of that depends on that rule right now but easy enough to fix (by
buffering last (x,y,touch) values).

For first point, your right that (123,78) is just a good "known
starting value" if a driver is going to use that concept because it
could be filtered just as easy as (0,0).  Its the "known" part thats
more useful to userland.  They can do a memset() when entering
proximity instead of buffering previous values.

I think this part of past driver behavior originated because, before
touch tools, wacom hardware was good about sending (x,y,touch) as
(0,0,0) when tool was out of proximity and this behavior came out to
userland without driver doing anything.  For touch case, software
driver must simulate older hardware behavior.  Again, I happen to know
some userland code that came to depend on that (0,0,0) but its easy
enough to fix.

So the overall patch is in line with existing multi-tool wacom tablets
and their code flow.

My original comment can be restated as if you going to send
BTN_TOUCH=0 then probably you should send ABS_X/Y=0 for consistency
with other wacom behavior.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2]input - wacom_w8001: Add one finger touch support

[Ping Cheng]

On Thu, Dec 9, 2010 at 1:21 PM, Chris Bagwell <chris@cnpbagwell.com> wrote:
> On Thu, Dec 9, 2010 at 1:50 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Thu, Dec 09, 2010 at 11:36:19AM -0800, Ping Cheng wrote:
>>> On Thu, Dec 9, 2010 at 7:06 AM, Chris Bagwell <chris@cnpbagwell.com> wrote:
>>> > On Wed, Dec 8, 2010 at 7:23 PM, Ping Cheng <pinglinux@gmail.com> wrote:
>>> >> @@ -228,9 +277,17 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
>>> >>                if (tmp == W8001_TOUCH_BYTE)
>>> >>                        break;
>>> >>
>>> >> +               if (w8001->has_touch) {
>>> >> +                       /* send touch data out */
>>> >> +                       w8001->has_touch = 0;
>>> >> +                       input_report_key(dev, BTN_TOUCH, 0);
>>> >> +                       input_report_key(dev, BTN_TOOL_FINGER, 0);
>>> >
>>> > Probably its better to set ABS_X/ABS_Y to zero and do a sync here?  So
>>> > duplicate x/y values don't get dropped and aligns with wacom_wac.c.
>>> > This is related to comment about forcing ABS_X/Y to zero above.  Its
>>> > so pen has known starting point when coming in proximity.  I wouldn't
>>> > do one without the other.
>>>
>>> I'll do both to make you happy (just kidding, to make it safe ;).
>>>
>>
>> Actually do not see how (0,0) is any safer than let's say (123,78)
>> sincve i believe (0,0) is a valid coordinate. Userspace should still
>> hang on to the last reported coordinate and use it if it did not get
>> a new one.
>>
>> Also, if you add input_sync() here won't it cause (in certain
>> situtations) false click or tap events - BTN_TOUCH goes from 1 to 0 and
>> then again to 1 if pen is already in proximity...
>
> The patches behavior happens to align good with existing wacom
> behavior and user land apps (xf86-input-wacom mostly) but its good to
> get this input from those not concentrating on wacom as much.  It
> helps remind me the old way is not the only way.
>
> Let me answer the second point first.
>
> The above code is following a kinda rule that other wacom tablet
> drivers obey.  That rule is that if 2 tools share ABS_* events then
> only 1 of those tools can be in proximity at one time.  If a driver
> follows that rule then you will not get the unwanted clicks you
> mention.
>
> The 1 tool in proximity probably originates from physical attribute of
> having to flip pen to get eraser or switching to another physical
> stylus.  I think it wasn't until touch+stylus that HW supported 2
> tools in proximity with same ABS_*.  The above logic comes in to
> simulate older HW enforced behavior.  There is some user land code I
> know of that depends on that rule right now but easy enough to fix (by
> buffering last (x,y,touch) values).
>
> For first point, your right that (123,78) is just a good "known
> starting value" if a driver is going to use that concept because it
> could be filtered just as easy as (0,0).  Its the "known" part thats
> more useful to userland.  They can do a memset() when entering
> proximity instead of buffering previous values.
>
> I think this part of past driver behavior originated because, before
> touch tools, wacom hardware was good about sending (x,y,touch) as
> (0,0,0) when tool was out of proximity and this behavior came out to
> userland without driver doing anything.  For touch case, software
> driver must simulate older hardware behavior.  Again, I happen to know
> some userland code that came to depend on that (0,0,0) but its easy
> enough to fix.
>
> So the overall patch is in line with existing multi-tool wacom tablets
> and their code flow.
>
> My original comment can be restated as if you going to send
> BTN_TOUCH=0 then probably you should send ABS_X/Y=0 for consistency
> with other wacom behavior.

So, do you mean if we do not send BTN_TOUCH=0, we do not need to send
ABS_X/Y=0? That is we only need to send BTN_TOOL_FINGER=0?

Ping
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2]input - wacom_w8001: Add one finger touch support

[Dmitry Torokhov]

On Thu, Dec 09, 2010 at 03:21:34PM -0600, Chris Bagwell wrote:
> On Thu, Dec 9, 2010 at 1:50 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Thu, Dec 09, 2010 at 11:36:19AM -0800, Ping Cheng wrote:
> >> On Thu, Dec 9, 2010 at 7:06 AM, Chris Bagwell <chris@cnpbagwell.com> wrote:
> >> > On Wed, Dec 8, 2010 at 7:23 PM, Ping Cheng <pinglinux@gmail.com> wrote:
> >> >> @@ -228,9 +277,17 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
> >> >>                if (tmp == W8001_TOUCH_BYTE)
> >> >>                        break;
> >> >>
> >> >> +               if (w8001->has_touch) {
> >> >> +                       /* send touch data out */
> >> >> +                       w8001->has_touch = 0;
> >> >> +                       input_report_key(dev, BTN_TOUCH, 0);
> >> >> +                       input_report_key(dev, BTN_TOOL_FINGER, 0);
> >> >
> >> > Probably its better to set ABS_X/ABS_Y to zero and do a sync here?  So
> >> > duplicate x/y values don't get dropped and aligns with wacom_wac.c.
> >> > This is related to comment about forcing ABS_X/Y to zero above.  Its
> >> > so pen has known starting point when coming in proximity.  I wouldn't
> >> > do one without the other.
> >>
> >> I'll do both to make you happy (just kidding, to make it safe ;).
> >>
> >
> > Actually do not see how (0,0) is any safer than let's say (123,78)
> > sincve i believe (0,0) is a valid coordinate. Userspace should still
> > hang on to the last reported coordinate and use it if it did not get
> > a new one.
> >
> > Also, if you add input_sync() here won't it cause (in certain
> > situtations) false click or tap events - BTN_TOUCH goes from 1 to 0 and
> > then again to 1 if pen is already in proximity...
> 
> The patches behavior happens to align good with existing wacom
> behavior and user land apps (xf86-input-wacom mostly) but its good to
> get this input from those not concentrating on wacom as much.  It
> helps remind me the old way is not the only way.
> 
> Let me answer the second point first.
> 
> The above code is following a kinda rule that other wacom tablet
> drivers obey.  That rule is that if 2 tools share ABS_* events then
> only 1 of those tools can be in proximity at one time.  If a driver
> follows that rule then you will not get the unwanted clicks you
> mention.
> 
> The 1 tool in proximity probably originates from physical attribute of
> having to flip pen to get eraser or switching to another physical
> stylus.  I think it wasn't until touch+stylus that HW supported 2
> tools in proximity with same ABS_*.  The above logic comes in to
> simulate older HW enforced behavior.  There is some user land code I
> know of that depends on that rule right now but easy enough to fix (by
> buffering last (x,y,touch) values).
> 
> For first point, your right that (123,78) is just a good "known
> starting value" if a driver is going to use that concept because it
> could be filtered just as easy as (0,0). 

I am afraid I did not explain myself well enough. (0,0) is valid
coordinate, same as (123, 78). Thus, even if we try to make driver send
(0,0, !touch), input core may suppress it and never deliver to
userspace if last touching point happened to be also (0,0). Thus
userspace should not rely on having coordinates reset I think.

> Its the "known" part thats
> more useful to userland.  They can do a memset() when entering
> proximity instead of buffering previous values.
> 
> I think this part of past driver behavior originated because, before
> touch tools, wacom hardware was good about sending (x,y,touch) as
> (0,0,0) when tool was out of proximity and this behavior came out to
> userland without driver doing anything.  For touch case, software
> driver must simulate older hardware behavior.  Again, I happen to know
> some userland code that came to depend on that (0,0,0) but its easy
> enough to fix.


Yep, that would be great. Userspace should expect that any duplicate
event will be filtered out.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2]input - wacom_w8001: Add one finger touch support

[Chris Bagwell]

On Thu, Dec 9, 2010 at 7:29 PM, Ping Cheng <pinglinux@gmail.com> wrote:
> On Thu, Dec 9, 2010 at 1:21 PM, Chris Bagwell <chris@cnpbagwell.com> wrote:
>> On Thu, Dec 9, 2010 at 1:50 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>>> On Thu, Dec 09, 2010 at 11:36:19AM -0800, Ping Cheng wrote:
>>>> On Thu, Dec 9, 2010 at 7:06 AM, Chris Bagwell <chris@cnpbagwell.com> wrote:
>>>> > On Wed, Dec 8, 2010 at 7:23 PM, Ping Cheng <pinglinux@gmail.com> wrote:
>>>> >> @@ -228,9 +277,17 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
>>>> >>                if (tmp == W8001_TOUCH_BYTE)
>>>> >>                        break;
>>>> >>
>>>> >> +               if (w8001->has_touch) {
>>>> >> +                       /* send touch data out */
>>>> >> +                       w8001->has_touch = 0;
>>>> >> +                       input_report_key(dev, BTN_TOUCH, 0);
>>>> >> +                       input_report_key(dev, BTN_TOOL_FINGER, 0);
>>>> >
>>>> > Probably its better to set ABS_X/ABS_Y to zero and do a sync here?  So
>>>> > duplicate x/y values don't get dropped and aligns with wacom_wac.c.
>>>> > This is related to comment about forcing ABS_X/Y to zero above.  Its
>>>> > so pen has known starting point when coming in proximity.  I wouldn't
>>>> > do one without the other.
>>>>
>>>> I'll do both to make you happy (just kidding, to make it safe ;).
>>>>
>>>
>>> Actually do not see how (0,0) is any safer than let's say (123,78)
>>> sincve i believe (0,0) is a valid coordinate. Userspace should still
>>> hang on to the last reported coordinate and use it if it did not get
>>> a new one.
>>>
>>> Also, if you add input_sync() here won't it cause (in certain
>>> situtations) false click or tap events - BTN_TOUCH goes from 1 to 0 and
>>> then again to 1 if pen is already in proximity...
>>
>> The patches behavior happens to align good with existing wacom
>> behavior and user land apps (xf86-input-wacom mostly) but its good to
>> get this input from those not concentrating on wacom as much.  It
>> helps remind me the old way is not the only way.
>>
>> Let me answer the second point first.
>>
>> The above code is following a kinda rule that other wacom tablet
>> drivers obey.  That rule is that if 2 tools share ABS_* events then
>> only 1 of those tools can be in proximity at one time.  If a driver
>> follows that rule then you will not get the unwanted clicks you
>> mention.
>>
>> The 1 tool in proximity probably originates from physical attribute of
>> having to flip pen to get eraser or switching to another physical
>> stylus.  I think it wasn't until touch+stylus that HW supported 2
>> tools in proximity with same ABS_*.  The above logic comes in to
>> simulate older HW enforced behavior.  There is some user land code I
>> know of that depends on that rule right now but easy enough to fix (by
>> buffering last (x,y,touch) values).
>>
>> For first point, your right that (123,78) is just a good "known
>> starting value" if a driver is going to use that concept because it
>> could be filtered just as easy as (0,0).  Its the "known" part thats
>> more useful to userland.  They can do a memset() when entering
>> proximity instead of buffering previous values.
>>
>> I think this part of past driver behavior originated because, before
>> touch tools, wacom hardware was good about sending (x,y,touch) as
>> (0,0,0) when tool was out of proximity and this behavior came out to
>> userland without driver doing anything.  For touch case, software
>> driver must simulate older hardware behavior.  Again, I happen to know
>> some userland code that came to depend on that (0,0,0) but its easy
>> enough to fix.
>>
>> So the overall patch is in line with existing multi-tool wacom tablets
>> and their code flow.
>>
>> My original comment can be restated as if you going to send
>> BTN_TOUCH=0 then probably you should send ABS_X/Y=0 for consistency
>> with other wacom behavior.
>
> So, do you mean if we do not send BTN_TOUCH=0, we do not need to send
> ABS_X/Y=0? That is we only need to send BTN_TOOL_FINGER=0?
>

Correct.  Especially if there is no sync there because BTN_TOUCH gets
updated with real value shortly after and it implies userland needs to
know previous tools BTN_TOUCH in case a new BTN_TOUCH value is not
sent.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2]input - wacom_w8001: Add one finger touch support

[Chris Bagwell]

On Fri, Dec 10, 2010 at 1:38 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Dec 09, 2010 at 03:21:34PM -0600, Chris Bagwell wrote:
>>
>> For first point, your right that (123,78) is just a good "known
>> starting value" if a driver is going to use that concept because it
>> could be filtered just as easy as (0,0).
>
> I am afraid I did not explain myself well enough. (0,0) is valid
> coordinate, same as (123, 78). Thus, even if we try to make driver send
> (0,0, !touch), input core may suppress it and never deliver to
> userspace if last touching point happened to be also (0,0). Thus
> userspace should not rely on having coordinates reset I think.
>

I had understood ya.  What I meant is if a driver sends (0,0,!touch)
and a sync when switching tools then you do not need to buffer
previous values but instead can imply what previous values are.
Userland would still need to account for case of filtered events.  Its
a memset() vs. a memcpy() when switching tools.

Anyways,  I'm not recommending any behavior; just letting you know
some existing assumptions on driver behavior.

Thanks,
Chris

Re: [PATCH 2/2]input - wacom_w8001: Add one finger touch support

[Ping Cheng]

On Fri, Dec 10, 2010 at 5:47 AM, Chris Bagwell <chris@cnpbagwell.com> wrote:
> On Fri, Dec 10, 2010 at 1:38 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Thu, Dec 09, 2010 at 03:21:34PM -0600, Chris Bagwell wrote:
>>>
>>> For first point, your right that (123,78) is just a good "known
>>> starting value" if a driver is going to use that concept because it
>>> could be filtered just as easy as (0,0).
>>
>> I am afraid I did not explain myself well enough. (0,0) is valid
>> coordinate, same as (123, 78). Thus, even if we try to make driver send
>> (0,0, !touch), input core may suppress it and never deliver to
>> userspace if last touching point happened to be also (0,0). Thus
>> userspace should not rely on having coordinates reset I think.
>>
>
> I had understood ya.  What I meant is if a driver sends (0,0,!touch)
> and a sync when switching tools then you do not need to buffer
> previous values but instead can imply what previous values are.
> Userland would still need to account for case of filtered events.  Its
> a memset() vs. a memcpy() when switching tools.

Oh, I see your point. I think we need to add a sync statement to mark
the end of the touch events. This is consistent with the existing
behavior.

Thank you for reviewing the patchset.

Ping

> Anyways,  I'm not recommending any behavior; just letting you know
> some existing assumptions on driver behavior.
>
> Thanks,
> Chris
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html