All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] HID: wacom: Correct coordinate system of touchring and pen twist
@ 2017-08-24 23:09 Jason Gerecke
  2017-08-29 18:34 ` Ping Cheng
  0 siblings, 1 reply; 6+ messages in thread
From: Jason Gerecke @ 2017-08-24 23:09 UTC (permalink / raw)
  To: linux-input, Jiri Kosina
  Cc: Ping Cheng, Aaron Skomra, Benjamin Tissoires, Jason Gerecke,
	stable, Jason Gerecke

The MobileStudio Pro, Cintiq Pro, and 2nd-gen Intuos Pro devices use a
different coordinate system for their touchring and pen twist than prior
devices. Prior devices had zero aligned to the tablet's left and would
increase clockwise. Userspace expects data from the kernel to be in this
old coordinate space, so adjustments are necessary.

While the coordinate system for pen twist is formally defined by the HID
standard, no such definition existed for the touchring at the time these
tablets were introduced. Future tablets are expected to report touchring
data using the same "zero-up clockwise-increasing" coordinate system
defined for twist.

Fixes: 60a2218698 ("HID: wacom: generic: add support for touchring")
Fixes: 50066a042d ("HID: wacom: generic: Add support for height, tilt, and twist usages")
Cc: stable@vger.kernel.org
Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/hid/wacom_wac.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index bb17d7bbefd3..7f0b3d497ecb 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1616,6 +1616,20 @@ static int wacom_tpc_irq(struct wacom_wac *wacom, size_t len)
 	return 0;
 }
 
+static int wacom_offset_rotation(struct input_dev *input, struct hid_usage *usage,
+				 int value, int num, int denom)
+{
+	struct input_absinfo *abs = &input->absinfo[usage->code];
+	int range = (abs->maximum - abs->minimum + 1);
+
+	value += num*range/denom;
+	if (value > abs->maximum)
+		value -= range;
+	else if (value < abs->minimum)
+		value += range;
+	return value;
+}
+
 int wacom_equivalent_usage(int usage)
 {
 	if ((usage & HID_USAGE_PAGE) == WACOM_HID_UP_WACOMDIGITIZER) {
@@ -1898,6 +1912,7 @@ static void wacom_wac_pad_event(struct hid_device *hdev, struct hid_field *field
 	unsigned equivalent_usage = wacom_equivalent_usage(usage->hid);
 	int i;
 	bool is_touch_on = value;
+	bool do_report = false;
 
 	/*
 	 * Avoid reporting this event and setting inrange_state if this usage
@@ -1912,6 +1927,29 @@ static void wacom_wac_pad_event(struct hid_device *hdev, struct hid_field *field
 	}
 
 	switch (equivalent_usage) {
+	case WACOM_HID_WD_TOUCHRING:
+		/*
+		 * Userspace expects touchrings to increase in value with
+		 * clockwise gestures and have their zero point at the
+		 * tablet's left. HID events "should" be clockwise-
+		 * increasing and zero at top, though the MobileStudio
+		 * Pro and 2nd-gen Intuos Pro don't do this...
+		 */
+		if (hdev->vendor == 0x56a &&
+		    (hdev->product == 0x34d || hdev->product == 0x34e ||  /* MobileStudio Pro */
+		     hdev->product == 0x357 || hdev->product == 0x358)) { /* Intuos Pro 2 */
+			value = (field->logical_maximum - value);
+
+			if (hdev->product == 0x357 || hdev->product == 0x358)
+				value = wacom_offset_rotation(input, usage, value, 3, 16);
+			else if (hdev->product == 0x34d || hdev->product == 0x34e)
+				value = wacom_offset_rotation(input, usage, value, 1, 2);
+		}
+		else {
+			value = wacom_offset_rotation(input, usage, value, 1, 4);
+		}
+		do_report = true;
+		break;
 	case WACOM_HID_WD_TOUCHRINGSTATUS:
 		if (!value)
 			input_event(input, usage->type, usage->code, 0);
@@ -1945,10 +1983,14 @@ static void wacom_wac_pad_event(struct hid_device *hdev, struct hid_field *field
 					 value, i);
 		 /* fall through*/
 	default:
+		do_report = true;
+		break;
+	}
+
+	if (do_report) {
 		input_event(input, usage->type, usage->code, value);
 		if (value)
 			wacom_wac->hid_data.pad_input_event_flag = true;
-		break;
 	}
 }
 
@@ -2089,6 +2131,14 @@ static void wacom_wac_pen_event(struct hid_device *hdev, struct hid_field *field
 		wacom_wac->serial[0] = (wacom_wac->serial[0] & ~0xFFFFFFFFULL);
 		wacom_wac->serial[0] |= (__u32)value;
 		return;
+	case HID_DG_TWIST:
+		/*
+		 * Userspace expects pen twist to have its zero point when
+		 * the buttons/finger is on the tablet's left. HID values
+		 * are zero when buttons are toward the top.
+		 */
+		value = wacom_offset_rotation(input, usage, value, 1, 4);
+		break;
 	case WACOM_HID_WD_SENSE:
 		wacom_wac->hid_data.sense_state = value;
 		return;
-- 
2.14.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] HID: wacom: Correct coordinate system of touchring and pen twist
  2017-08-24 23:09 [PATCH] HID: wacom: Correct coordinate system of touchring and pen twist Jason Gerecke
@ 2017-08-29 18:34 ` Ping Cheng
  2017-08-30  0:37   ` Jason Gerecke
  0 siblings, 1 reply; 6+ messages in thread
From: Ping Cheng @ 2017-08-29 18:34 UTC (permalink / raw)
  To: Jason Gerecke, Jiri Kosina
  Cc: linux-input, Aaron Skomra, Benjamin Tissoires, stable, Jason Gerecke

On Thu, Aug 24, 2017 at 4:09 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> The MobileStudio Pro, Cintiq Pro, and 2nd-gen Intuos Pro devices use a
> different coordinate system for their touchring and pen twist than prior
> devices. Prior devices had zero aligned to the tablet's left and would
> increase clockwise. Userspace expects data from the kernel to be in this
> old coordinate space, so adjustments are necessary.
>
> While the coordinate system for pen twist is formally defined by the HID
> standard, no such definition existed for the touchring at the time these
> tablets were introduced. Future tablets are expected to report touchring
> data using the same "zero-up clockwise-increasing" coordinate system
> defined for twist.
>
> Fixes: 60a2218698 ("HID: wacom: generic: add support for touchring")
> Fixes: 50066a042d ("HID: wacom: generic: Add support for height, tilt, and twist usages")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>

Thank you Jason for catching and correcting the issue.

Intuos Pro 2 series supports wireless/bluetooth connection, which was
implemented in its own routine. Should we also update that code path?
Except that, the patch is:

Reviewed-by: Ping Cheng <ping.cheng@wacom.com>

Cheers,
Ping

> ---
>  drivers/hid/wacom_wac.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index bb17d7bbefd3..7f0b3d497ecb 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1616,6 +1616,20 @@ static int wacom_tpc_irq(struct wacom_wac *wacom, size_t len)
>         return 0;
>  }
>
> +static int wacom_offset_rotation(struct input_dev *input, struct hid_usage *usage,
> +                                int value, int num, int denom)
> +{
> +       struct input_absinfo *abs = &input->absinfo[usage->code];
> +       int range = (abs->maximum - abs->minimum + 1);
> +
> +       value += num*range/denom;
> +       if (value > abs->maximum)
> +               value -= range;
> +       else if (value < abs->minimum)
> +               value += range;
> +       return value;
> +}
> +
>  int wacom_equivalent_usage(int usage)
>  {
>         if ((usage & HID_USAGE_PAGE) == WACOM_HID_UP_WACOMDIGITIZER) {
> @@ -1898,6 +1912,7 @@ static void wacom_wac_pad_event(struct hid_device *hdev, struct hid_field *field
>         unsigned equivalent_usage = wacom_equivalent_usage(usage->hid);
>         int i;
>         bool is_touch_on = value;
> +       bool do_report = false;
>
>         /*
>          * Avoid reporting this event and setting inrange_state if this usage
> @@ -1912,6 +1927,29 @@ static void wacom_wac_pad_event(struct hid_device *hdev, struct hid_field *field
>         }
>
>         switch (equivalent_usage) {
> +       case WACOM_HID_WD_TOUCHRING:
> +               /*
> +                * Userspace expects touchrings to increase in value with
> +                * clockwise gestures and have their zero point at the
> +                * tablet's left. HID events "should" be clockwise-
> +                * increasing and zero at top, though the MobileStudio
> +                * Pro and 2nd-gen Intuos Pro don't do this...
> +                */
> +               if (hdev->vendor == 0x56a &&
> +                   (hdev->product == 0x34d || hdev->product == 0x34e ||  /* MobileStudio Pro */
> +                    hdev->product == 0x357 || hdev->product == 0x358)) { /* Intuos Pro 2 */
> +                       value = (field->logical_maximum - value);
> +
> +                       if (hdev->product == 0x357 || hdev->product == 0x358)
> +                               value = wacom_offset_rotation(input, usage, value, 3, 16);
> +                       else if (hdev->product == 0x34d || hdev->product == 0x34e)
> +                               value = wacom_offset_rotation(input, usage, value, 1, 2);
> +               }
> +               else {
> +                       value = wacom_offset_rotation(input, usage, value, 1, 4);
> +               }
> +               do_report = true;
> +               break;
>         case WACOM_HID_WD_TOUCHRINGSTATUS:
>                 if (!value)
>                         input_event(input, usage->type, usage->code, 0);
> @@ -1945,10 +1983,14 @@ static void wacom_wac_pad_event(struct hid_device *hdev, struct hid_field *field
>                                          value, i);
>                  /* fall through*/
>         default:
> +               do_report = true;
> +               break;
> +       }
> +
> +       if (do_report) {
>                 input_event(input, usage->type, usage->code, value);
>                 if (value)
>                         wacom_wac->hid_data.pad_input_event_flag = true;
> -               break;
>         }
>  }
>
> @@ -2089,6 +2131,14 @@ static void wacom_wac_pen_event(struct hid_device *hdev, struct hid_field *field
>                 wacom_wac->serial[0] = (wacom_wac->serial[0] & ~0xFFFFFFFFULL);
>                 wacom_wac->serial[0] |= (__u32)value;
>                 return;
> +       case HID_DG_TWIST:
> +               /*
> +                * Userspace expects pen twist to have its zero point when
> +                * the buttons/finger is on the tablet's left. HID values
> +                * are zero when buttons are toward the top.
> +                */
> +               value = wacom_offset_rotation(input, usage, value, 1, 4);
> +               break;
>         case WACOM_HID_WD_SENSE:
>                 wacom_wac->hid_data.sense_state = value;
>                 return;
> --
> 2.14.1
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] HID: wacom: Correct coordinate system of touchring and pen twist
  2017-08-29 18:34 ` Ping Cheng
@ 2017-08-30  0:37   ` Jason Gerecke
  2017-08-30 22:13     ` [PATCH v2 1/2] HID: wacom: Properly report negative values from Intuos Pro 2 Bluetooth Jason Gerecke
  0 siblings, 1 reply; 6+ messages in thread
From: Jason Gerecke @ 2017-08-30  0:37 UTC (permalink / raw)
  To: Ping Cheng
  Cc: Jiri Kosina, linux-input, Aaron Skomra, Benjamin Tissoires,
	# v3.17+,
	Jason Gerecke

On Tue, Aug 29, 2017 at 11:34 AM, Ping Cheng <pinglinux@gmail.com> wrote:
> On Thu, Aug 24, 2017 at 4:09 PM, Jason Gerecke <killertofu@gmail.com> wrote:
>> The MobileStudio Pro, Cintiq Pro, and 2nd-gen Intuos Pro devices use a
>> different coordinate system for their touchring and pen twist than prior
>> devices. Prior devices had zero aligned to the tablet's left and would
>> increase clockwise. Userspace expects data from the kernel to be in this
>> old coordinate space, so adjustments are necessary.
>>
>> While the coordinate system for pen twist is formally defined by the HID
>> standard, no such definition existed for the touchring at the time these
>> tablets were introduced. Future tablets are expected to report touchring
>> data using the same "zero-up clockwise-increasing" coordinate system
>> defined for twist.
>>
>> Fixes: 60a2218698 ("HID: wacom: generic: add support for touchring")
>> Fixes: 50066a042d ("HID: wacom: generic: Add support for height, tilt, and twist usages")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
>
> Thank you Jason for catching and correcting the issue.
>
> Intuos Pro 2 series supports wireless/bluetooth connection, which was
> implemented in its own routine. Should we also update that code path?
> Except that, the patch is:
>
> Reviewed-by: Ping Cheng <ping.cheng@wacom.com>
>
> Cheers,
> Ping
>

Good call. I've also found a second issue with the wireless codepath's
handling of negative numbers that also needs fixing. I'll send an
updated patchset shortly.

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one  /
(That is to say, eight) to the two,     /
But you can’t take seven from three,    /
So you look at the sixty-fours....

>> ---
>>  drivers/hid/wacom_wac.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
>> index bb17d7bbefd3..7f0b3d497ecb 100644
>> --- a/drivers/hid/wacom_wac.c
>> +++ b/drivers/hid/wacom_wac.c
>> @@ -1616,6 +1616,20 @@ static int wacom_tpc_irq(struct wacom_wac *wacom, size_t len)
>>         return 0;
>>  }
>>
>> +static int wacom_offset_rotation(struct input_dev *input, struct hid_usage *usage,
>> +                                int value, int num, int denom)
>> +{
>> +       struct input_absinfo *abs = &input->absinfo[usage->code];
>> +       int range = (abs->maximum - abs->minimum + 1);
>> +
>> +       value += num*range/denom;
>> +       if (value > abs->maximum)
>> +               value -= range;
>> +       else if (value < abs->minimum)
>> +               value += range;
>> +       return value;
>> +}
>> +
>>  int wacom_equivalent_usage(int usage)
>>  {
>>         if ((usage & HID_USAGE_PAGE) == WACOM_HID_UP_WACOMDIGITIZER) {
>> @@ -1898,6 +1912,7 @@ static void wacom_wac_pad_event(struct hid_device *hdev, struct hid_field *field
>>         unsigned equivalent_usage = wacom_equivalent_usage(usage->hid);
>>         int i;
>>         bool is_touch_on = value;
>> +       bool do_report = false;
>>
>>         /*
>>          * Avoid reporting this event and setting inrange_state if this usage
>> @@ -1912,6 +1927,29 @@ static void wacom_wac_pad_event(struct hid_device *hdev, struct hid_field *field
>>         }
>>
>>         switch (equivalent_usage) {
>> +       case WACOM_HID_WD_TOUCHRING:
>> +               /*
>> +                * Userspace expects touchrings to increase in value with
>> +                * clockwise gestures and have their zero point at the
>> +                * tablet's left. HID events "should" be clockwise-
>> +                * increasing and zero at top, though the MobileStudio
>> +                * Pro and 2nd-gen Intuos Pro don't do this...
>> +                */
>> +               if (hdev->vendor == 0x56a &&
>> +                   (hdev->product == 0x34d || hdev->product == 0x34e ||  /* MobileStudio Pro */
>> +                    hdev->product == 0x357 || hdev->product == 0x358)) { /* Intuos Pro 2 */
>> +                       value = (field->logical_maximum - value);
>> +
>> +                       if (hdev->product == 0x357 || hdev->product == 0x358)
>> +                               value = wacom_offset_rotation(input, usage, value, 3, 16);
>> +                       else if (hdev->product == 0x34d || hdev->product == 0x34e)
>> +                               value = wacom_offset_rotation(input, usage, value, 1, 2);
>> +               }
>> +               else {
>> +                       value = wacom_offset_rotation(input, usage, value, 1, 4);
>> +               }
>> +               do_report = true;
>> +               break;
>>         case WACOM_HID_WD_TOUCHRINGSTATUS:
>>                 if (!value)
>>                         input_event(input, usage->type, usage->code, 0);
>> @@ -1945,10 +1983,14 @@ static void wacom_wac_pad_event(struct hid_device *hdev, struct hid_field *field
>>                                          value, i);
>>                  /* fall through*/
>>         default:
>> +               do_report = true;
>> +               break;
>> +       }
>> +
>> +       if (do_report) {
>>                 input_event(input, usage->type, usage->code, value);
>>                 if (value)
>>                         wacom_wac->hid_data.pad_input_event_flag = true;
>> -               break;
>>         }
>>  }
>>
>> @@ -2089,6 +2131,14 @@ static void wacom_wac_pen_event(struct hid_device *hdev, struct hid_field *field
>>                 wacom_wac->serial[0] = (wacom_wac->serial[0] & ~0xFFFFFFFFULL);
>>                 wacom_wac->serial[0] |= (__u32)value;
>>                 return;
>> +       case HID_DG_TWIST:
>> +               /*
>> +                * Userspace expects pen twist to have its zero point when
>> +                * the buttons/finger is on the tablet's left. HID values
>> +                * are zero when buttons are toward the top.
>> +                */
>> +               value = wacom_offset_rotation(input, usage, value, 1, 4);
>> +               break;
>>         case WACOM_HID_WD_SENSE:
>>                 wacom_wac->hid_data.sense_state = value;
>>                 return;
>> --
>> 2.14.1
>>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/2] HID: wacom: Properly report negative values from Intuos Pro 2 Bluetooth
  2017-08-30  0:37   ` Jason Gerecke
@ 2017-08-30 22:13     ` Jason Gerecke
  2017-08-30 22:13       ` [PATCH v2 2/2] HID: wacom: Correct coordinate system of touchring and pen twist Jason Gerecke
  2017-09-06  9:02       ` [PATCH v2 1/2] HID: wacom: Properly report negative values from Intuos Pro 2 Bluetooth Jiri Kosina
  0 siblings, 2 replies; 6+ messages in thread
From: Jason Gerecke @ 2017-08-30 22:13 UTC (permalink / raw)
  To: linux-input, Jiri Kosina
  Cc: stable, Ping Cheng, Aaron Skomra, Benjamin Tissoires,
	Jason Gerecke, Jason Gerecke

The wacom driver's IRQ handler for Bluetooth reports from the 2nd-gen
Intuos Pro does not correctly process negative numbers. Values for
tilt and rotation (which can go negative) are instead interpreted as
unsigned and so jump to very large values when the data should be
negative. This commit properly casts the data to ensure we report
negative numbers when necessary.

Fixes: 4922cd2 ("HID: wacom: Support 2nd-gen Intuos Pro's Bluetooth classic interface")
Cc: stable@vger.kernel.org # v4.11
Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
Changes from v1:
  * Patch introduced.

 drivers/hid/wacom_wac.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index bb17d7bbefd3..d7ca1eb9d559 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1229,9 +1229,9 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
 		if (range) {
 			input_report_abs(pen_input, ABS_X, get_unaligned_le16(&frame[1]));
 			input_report_abs(pen_input, ABS_Y, get_unaligned_le16(&frame[3]));
-			input_report_abs(pen_input, ABS_TILT_X, frame[7]);
-			input_report_abs(pen_input, ABS_TILT_Y, frame[8]);
-			input_report_abs(pen_input, ABS_Z, get_unaligned_le16(&frame[9]));
+			input_report_abs(pen_input, ABS_TILT_X, (char)frame[7]);
+			input_report_abs(pen_input, ABS_TILT_Y, (char)frame[8]);
+			input_report_abs(pen_input, ABS_Z, (int16_t)get_unaligned_le16(&frame[9]));
 			input_report_abs(pen_input, ABS_WHEEL, get_unaligned_le16(&frame[11]));
 		}
 		input_report_abs(pen_input, ABS_PRESSURE, get_unaligned_le16(&frame[5]));
-- 
2.14.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 2/2] HID: wacom: Correct coordinate system of touchring and pen twist
  2017-08-30 22:13     ` [PATCH v2 1/2] HID: wacom: Properly report negative values from Intuos Pro 2 Bluetooth Jason Gerecke
@ 2017-08-30 22:13       ` Jason Gerecke
  2017-09-06  9:02       ` [PATCH v2 1/2] HID: wacom: Properly report negative values from Intuos Pro 2 Bluetooth Jiri Kosina
  1 sibling, 0 replies; 6+ messages in thread
From: Jason Gerecke @ 2017-08-30 22:13 UTC (permalink / raw)
  To: linux-input, Jiri Kosina
  Cc: stable, Ping Cheng, Aaron Skomra, Benjamin Tissoires,
	Jason Gerecke, Jason Gerecke

The MobileStudio Pro, Cintiq Pro, and 2nd-gen Intuos Pro devices use a
different coordinate system for their touchring and pen twist than prior
devices. Prior devices had zero aligned to the tablet's left and would
increase clockwise. Userspace expects data from the kernel to be in this
old coordinate space, so adjustments are necessary.

While the coordinate system for pen twist is formally defined by the HID
standard, no such definition existed for the touchring at the time these
tablets were introduced. Future tablets are expected to report touchring
data using the same "zero-up clockwise-increasing" coordinate system
defined for twist.

Fixes: 50066a042d ("HID: wacom: generic: Add support for height, tilt, and twist usages")
Fixes: 4922cd26f0 ("HID: wacom: Support 2nd-gen Intuos Pro's Bluetooth classic interface")
Fixes: 60a2218698 ("HID: wacom: generic: add support for touchring")
Cc: stable@vger.kernel.org # 4.10, 4.11
Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
Reviewed-by: Ping Cheng <ping.cheng@wacom.com>
---
Changes from v1:
  * Applied fixes to bluetooth codepath as well

 drivers/hid/wacom_wac.c | 73 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 68 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index d7ca1eb9d559..d1bac66108e2 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1227,11 +1227,17 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
 			continue;
 
 		if (range) {
+			/* Fix rotation alignment: userspace expects zero at left */
+			int16_t rotation = (int16_t)get_unaligned_le16(&frame[9]);
+			rotation += 1800/4;
+			if (rotation > 899)
+				rotation -= 1800;
+
 			input_report_abs(pen_input, ABS_X, get_unaligned_le16(&frame[1]));
 			input_report_abs(pen_input, ABS_Y, get_unaligned_le16(&frame[3]));
 			input_report_abs(pen_input, ABS_TILT_X, (char)frame[7]);
 			input_report_abs(pen_input, ABS_TILT_Y, (char)frame[8]);
-			input_report_abs(pen_input, ABS_Z, (int16_t)get_unaligned_le16(&frame[9]));
+			input_report_abs(pen_input, ABS_Z, rotation);
 			input_report_abs(pen_input, ABS_WHEEL, get_unaligned_le16(&frame[11]));
 		}
 		input_report_abs(pen_input, ABS_PRESSURE, get_unaligned_le16(&frame[5]));
@@ -1319,12 +1325,19 @@ static void wacom_intuos_pro2_bt_pad(struct wacom_wac *wacom)
 	unsigned char *data = wacom->data;
 
 	int buttons = (data[282] << 1) | ((data[281] >> 6) & 0x01);
-	int ring = data[285];
-	int prox = buttons | (ring & 0x80);
+	int ring = data[285] & 0x7F;
+	bool ringstatus = data[285] & 0x80;
+	bool prox = buttons || ringstatus;
+
+	/* Fix touchring data: userspace expects 0 at left and increasing clockwise */
+	ring = 71 - ring;
+	ring += 3*72/16;
+	if (ring > 71)
+		ring -= 72;
 
 	wacom_report_numbered_buttons(pad_input, 9, buttons);
 
-	input_report_abs(pad_input, ABS_WHEEL, (ring & 0x80) ? (ring & 0x7f) : 0);
+	input_report_abs(pad_input, ABS_WHEEL, ringstatus ? ring : 0);
 
 	input_report_key(pad_input, wacom->tool[1], prox ? 1 : 0);
 	input_report_abs(pad_input, ABS_MISC, prox ? PAD_DEVICE_ID : 0);
@@ -1616,6 +1629,20 @@ static int wacom_tpc_irq(struct wacom_wac *wacom, size_t len)
 	return 0;
 }
 
+static int wacom_offset_rotation(struct input_dev *input, struct hid_usage *usage,
+				 int value, int num, int denom)
+{
+	struct input_absinfo *abs = &input->absinfo[usage->code];
+	int range = (abs->maximum - abs->minimum + 1);
+
+	value += num*range/denom;
+	if (value > abs->maximum)
+		value -= range;
+	else if (value < abs->minimum)
+		value += range;
+	return value;
+}
+
 int wacom_equivalent_usage(int usage)
 {
 	if ((usage & HID_USAGE_PAGE) == WACOM_HID_UP_WACOMDIGITIZER) {
@@ -1898,6 +1925,7 @@ static void wacom_wac_pad_event(struct hid_device *hdev, struct hid_field *field
 	unsigned equivalent_usage = wacom_equivalent_usage(usage->hid);
 	int i;
 	bool is_touch_on = value;
+	bool do_report = false;
 
 	/*
 	 * Avoid reporting this event and setting inrange_state if this usage
@@ -1912,6 +1940,29 @@ static void wacom_wac_pad_event(struct hid_device *hdev, struct hid_field *field
 	}
 
 	switch (equivalent_usage) {
+	case WACOM_HID_WD_TOUCHRING:
+		/*
+		 * Userspace expects touchrings to increase in value with
+		 * clockwise gestures and have their zero point at the
+		 * tablet's left. HID events "should" be clockwise-
+		 * increasing and zero at top, though the MobileStudio
+		 * Pro and 2nd-gen Intuos Pro don't do this...
+		 */
+		if (hdev->vendor == 0x56a &&
+		    (hdev->product == 0x34d || hdev->product == 0x34e ||  /* MobileStudio Pro */
+		     hdev->product == 0x357 || hdev->product == 0x358)) { /* Intuos Pro 2 */
+			value = (field->logical_maximum - value);
+
+			if (hdev->product == 0x357 || hdev->product == 0x358)
+				value = wacom_offset_rotation(input, usage, value, 3, 16);
+			else if (hdev->product == 0x34d || hdev->product == 0x34e)
+				value = wacom_offset_rotation(input, usage, value, 1, 2);
+		}
+		else {
+			value = wacom_offset_rotation(input, usage, value, 1, 4);
+		}
+		do_report = true;
+		break;
 	case WACOM_HID_WD_TOUCHRINGSTATUS:
 		if (!value)
 			input_event(input, usage->type, usage->code, 0);
@@ -1945,10 +1996,14 @@ static void wacom_wac_pad_event(struct hid_device *hdev, struct hid_field *field
 					 value, i);
 		 /* fall through*/
 	default:
+		do_report = true;
+		break;
+	}
+
+	if (do_report) {
 		input_event(input, usage->type, usage->code, value);
 		if (value)
 			wacom_wac->hid_data.pad_input_event_flag = true;
-		break;
 	}
 }
 
@@ -2089,6 +2144,14 @@ static void wacom_wac_pen_event(struct hid_device *hdev, struct hid_field *field
 		wacom_wac->serial[0] = (wacom_wac->serial[0] & ~0xFFFFFFFFULL);
 		wacom_wac->serial[0] |= (__u32)value;
 		return;
+	case HID_DG_TWIST:
+		/*
+		 * Userspace expects pen twist to have its zero point when
+		 * the buttons/finger is on the tablet's left. HID values
+		 * are zero when buttons are toward the top.
+		 */
+		value = wacom_offset_rotation(input, usage, value, 1, 4);
+		break;
 	case WACOM_HID_WD_SENSE:
 		wacom_wac->hid_data.sense_state = value;
 		return;
-- 
2.14.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] HID: wacom: Properly report negative values from Intuos Pro 2 Bluetooth
  2017-08-30 22:13     ` [PATCH v2 1/2] HID: wacom: Properly report negative values from Intuos Pro 2 Bluetooth Jason Gerecke
  2017-08-30 22:13       ` [PATCH v2 2/2] HID: wacom: Correct coordinate system of touchring and pen twist Jason Gerecke
@ 2017-09-06  9:02       ` Jiri Kosina
  1 sibling, 0 replies; 6+ messages in thread
From: Jiri Kosina @ 2017-09-06  9:02 UTC (permalink / raw)
  To: Jason Gerecke
  Cc: linux-input, stable, Ping Cheng, Aaron Skomra,
	Benjamin Tissoires, Jason Gerecke

On Wed, 30 Aug 2017, Jason Gerecke wrote:

> The wacom driver's IRQ handler for Bluetooth reports from the 2nd-gen
> Intuos Pro does not correctly process negative numbers. Values for
> tilt and rotation (which can go negative) are instead interpreted as
> unsigned and so jump to very large values when the data should be
> negative. This commit properly casts the data to ensure we report
> negative numbers when necessary.
> 
> Fixes: 4922cd2 ("HID: wacom: Support 2nd-gen Intuos Pro's Bluetooth classic interface")
> Cc: stable@vger.kernel.org # v4.11
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>

Both patches applied to for-4.14/upstream-fixes.

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-09-06  9:02 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-24 23:09 [PATCH] HID: wacom: Correct coordinate system of touchring and pen twist Jason Gerecke
2017-08-29 18:34 ` Ping Cheng
2017-08-30  0:37   ` Jason Gerecke
2017-08-30 22:13     ` [PATCH v2 1/2] HID: wacom: Properly report negative values from Intuos Pro 2 Bluetooth Jason Gerecke
2017-08-30 22:13       ` [PATCH v2 2/2] HID: wacom: Correct coordinate system of touchring and pen twist Jason Gerecke
2017-09-06  9:02       ` [PATCH v2 1/2] HID: wacom: Properly report negative values from Intuos Pro 2 Bluetooth Jiri Kosina

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.