Re: [PATCH] HID: wacom: Correct coordinate system of touchring and pen twist

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Aug 24, 2017 at 4:09 PM, Jason Gerecke <killertofu@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Jason Gerecke <jason.gerecke@xxxxxxxxx>

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@xxxxxxxxx>

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
>



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]