diff mbox series

HID: fix A4Tech horizontal scrolling

Message ID 20190502213639.7632-1-spaz16@wp.pl (mailing list archive)
State Superseded
Delegated to: Jiri Kosina
Headers show
Series HID: fix A4Tech horizontal scrolling | expand

Commit Message

Błażej Szczygieł May 2, 2019, 9:36 p.m. UTC
Since recent high resolution scrolling changes the A4Tech driver must
check for the "REL_WHEEL_HI_RES" usage code.

Fixes: 2dc702c991e3774af9d7ce410eef410ca9e2357e (HID: input: use the
Resolution Multiplier for high-resolution scrolling)

Signed-off-by: Błażej Szczygieł <spaz16@wp.pl>
---
 drivers/hid/hid-a4tech.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Benjamin Tissoires May 3, 2019, 7:36 a.m. UTC | #1
Hi,

On Thu, May 2, 2019 at 11:37 PM Błażej Szczygieł <spaz16@wp.pl> wrote:
>
> Since recent high resolution scrolling changes the A4Tech driver must
> check for the "REL_WHEEL_HI_RES" usage code.
>
> Fixes: 2dc702c991e3774af9d7ce410eef410ca9e2357e (HID: input: use the
> Resolution Multiplier for high-resolution scrolling)
>
> Signed-off-by: Błażej Szczygieł <spaz16@wp.pl>

Thanks for the patch. I do not doubt this fixes the issues, but I
still wonder if we should not export REL_HWHEEL_HI_RES instead of
REL_HWHEEL events.

Also, I can not figure out how the events are processed by the kernel.
Could you attach a hid-recorder dump of the mouse wheels with
hid-recorder from https://gitlab.freedesktop.org/libevdev/hid-tools ?

This should give me a better view of the mouse, and I could also add
it to the regression tests I am running for each commit.

Cheers,
Benjamin

> ---
>  drivers/hid/hid-a4tech.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-a4tech.c b/drivers/hid/hid-a4tech.c
> index 9428ea7cdf8a..fafb9fa558e7 100644
> --- a/drivers/hid/hid-a4tech.c
> +++ b/drivers/hid/hid-a4tech.c
> @@ -38,7 +38,7 @@ static int a4_input_mapped(struct hid_device *hdev, struct hid_input *hi,
>  {
>         struct a4tech_sc *a4 = hid_get_drvdata(hdev);
>
> -       if (usage->type == EV_REL && usage->code == REL_WHEEL)
> +       if (usage->type == EV_REL && usage->code == REL_WHEEL_HI_RES)
>                 set_bit(REL_HWHEEL, *bit);
>
>         if ((a4->quirks & A4_2WHEEL_MOUSE_HACK_7) && usage->hid == 0x00090007)
> @@ -60,7 +60,7 @@ static int a4_event(struct hid_device *hdev, struct hid_field *field,
>         input = field->hidinput->input;
>
>         if (a4->quirks & A4_2WHEEL_MOUSE_HACK_B8) {
> -               if (usage->type == EV_REL && usage->code == REL_WHEEL) {
> +               if (usage->type == EV_REL && usage->code == REL_WHEEL_HI_RES) {
>                         a4->delayed_value = value;
>                         return 1;
>                 }
> @@ -77,7 +77,7 @@ static int a4_event(struct hid_device *hdev, struct hid_field *field,
>                 return 1;
>         }
>
> -       if (usage->code == REL_WHEEL && a4->hw_wheel) {
> +       if (usage->code == REL_WHEEL_HI_RES && a4->hw_wheel) {
>                 input_event(input, usage->type, REL_HWHEEL, value);
>                 return 1;
>         }
> --
> 2.21.0
>
Błażej Szczygieł May 3, 2019, 9:22 a.m. UTC | #2
Hi,

I used the hid-record tool and my results are here:
https://gitlab.com/snippets/1853568

Cheers,
Błażej

> Hi,
> 
> On Thu, May 2, 2019 at 11:37 PM Błażej Szczygieł <spaz16@wp.pl> wrote:
>>
>> Since recent high resolution scrolling changes the A4Tech driver must
>> check for the "REL_WHEEL_HI_RES" usage code.
>>
>> Fixes: 2dc702c991e3774af9d7ce410eef410ca9e2357e (HID: input: use the
>> Resolution Multiplier for high-resolution scrolling)
>>
>> Signed-off-by: Błażej Szczygieł <spaz16@wp.pl>
> 
> Thanks for the patch. I do not doubt this fixes the issues, but I
> still wonder if we should not export REL_HWHEEL_HI_RES instead of
> REL_HWHEEL events.
> 
> Also, I can not figure out how the events are processed by the kernel.
> Could you attach a hid-recorder dump of the mouse wheels with
> hid-recorder from https://gitlab.freedesktop.org/libevdev/hid-tools ?
> 
> This should give me a better view of the mouse, and I could also add
> it to the regression tests I am running for each commit.
> 
> Cheers,
> Benjamin
> 
>> ---
>>   drivers/hid/hid-a4tech.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hid/hid-a4tech.c b/drivers/hid/hid-a4tech.c
>> index 9428ea7cdf8a..fafb9fa558e7 100644
>> --- a/drivers/hid/hid-a4tech.c
>> +++ b/drivers/hid/hid-a4tech.c
>> @@ -38,7 +38,7 @@ static int a4_input_mapped(struct hid_device *hdev, struct hid_input *hi,
>>   {
>>          struct a4tech_sc *a4 = hid_get_drvdata(hdev);
>>
>> -       if (usage->type == EV_REL && usage->code == REL_WHEEL)
>> +       if (usage->type == EV_REL && usage->code == REL_WHEEL_HI_RES)
>>                  set_bit(REL_HWHEEL, *bit);
>>
>>          if ((a4->quirks & A4_2WHEEL_MOUSE_HACK_7) && usage->hid == 0x00090007)
>> @@ -60,7 +60,7 @@ static int a4_event(struct hid_device *hdev, struct hid_field *field,
>>          input = field->hidinput->input;
>>
>>          if (a4->quirks & A4_2WHEEL_MOUSE_HACK_B8) {
>> -               if (usage->type == EV_REL && usage->code == REL_WHEEL) {
>> +               if (usage->type == EV_REL && usage->code == REL_WHEEL_HI_RES) {
>>                          a4->delayed_value = value;
>>                          return 1;
>>                  }
>> @@ -77,7 +77,7 @@ static int a4_event(struct hid_device *hdev, struct hid_field *field,
>>                  return 1;
>>          }
>>
>> -       if (usage->code == REL_WHEEL && a4->hw_wheel) {
>> +       if (usage->code == REL_WHEEL_HI_RES && a4->hw_wheel) {
>>                  input_event(input, usage->type, REL_HWHEEL, value);
>>                  return 1;
>>          }
>> --
>> 2.21.0
>>
Igor Kushnir May 3, 2019, 9:36 a.m. UTC | #3
Hi Benjamin,

On 5/3/19 10:36 AM, Benjamin Tissoires wrote:
> Hi,
> 
> On Thu, May 2, 2019 at 11:37 PM Błażej Szczygieł <spaz16@wp.pl> wrote:
>>
>> Since recent high resolution scrolling changes the A4Tech driver must
>> check for the "REL_WHEEL_HI_RES" usage code.
>>
>> Fixes: 2dc702c991e3774af9d7ce410eef410ca9e2357e (HID: input: use the
>> Resolution Multiplier for high-resolution scrolling)
>>
>> Signed-off-by: Błażej Szczygieł <spaz16@wp.pl>
> 
> Thanks for the patch. I do not doubt this fixes the issues, but I
> still wonder if we should not export REL_HWHEEL_HI_RES instead of
> REL_HWHEEL events.


If you mean exporting REL_HWHEEL_HI_RES instead of REL_HWHEEL from
hid-a4tech.c, then it makes sense to me, though I do not know the code
well enough to be certain.

Błażej and I have discussed the bug and the patch here:
https://bugzilla.kernel.org/show_bug.cgi?id=203369

In summary: the patch fixes the bug for both our mice;
the documentation in input/event-codes.rst states that
REL_WHEEL, REL_HWHEEL "are legacy codes and REL_WHEEL_HI_RES and
REL_HWHEEL_HI_RES should be preferred where available."

> Also, I can not figure out how the events are processed by the kernel.
> Could you attach a hid-recorder dump of the mouse wheels with
> hid-recorder from https://gitlab.freedesktop.org/libevdev/hid-tools ?
> 
> This should give me a better view of the mouse, and I could also add
> it to the regression tests I am running for each commit.
> 
> Cheers,
> Benjamin

After launching hid-recorder for my A4Tech WOP-49Z mouse under kernel
5.0.10 patched with Błażej's patch I:
* scrolled the vertical wheel down ("Wheel: -1");
* scrolled the vertical wheel up ("Wheel: 1");
* scrolled the horizontal wheel "left" ("Wheel: -1");
* scrolled the horizontal wheel "right" ("Wheel: 1").
Note that the horizontal wheel is physically scrolled just like the
vertical one in this mouse (forward/back), so "left" and "right" are the
effects these scrollings make in applications when the kernel supports
the mouse properly.

$ sudo ./hid-recorder /dev/hidraw1
# A4Tech PS/2+USB Mouse
# 0x05, 0x01,                    // Usage Page (Generic Desktop)        0
# 0x09, 0x02,                    // Usage (Mouse)                       2
# 0xa1, 0x01,                    // Collection (Application)            4
# 0x09, 0x01,                    //  Usage (Pointer)                    6
# 0xa1, 0x00,                    //  Collection (Physical)              8
# 0x05, 0x09,                    //   Usage Page (Button)               10
# 0x19, 0x01,                    //   Usage Minimum (1)                 12
# 0x29, 0x07,                    //   Usage Maximum (7)                 14
# 0x15, 0x00,                    //   Logical Minimum (0)               16
# 0x25, 0x01,                    //   Logical Maximum (1)               18
# 0x75, 0x01,                    //   Report Size (1)                   20
# 0x95, 0x07,                    //   Report Count (7)                  22
# 0x81, 0x02,                    //   Input (Data,Var,Abs)              24
# 0x75, 0x01,                    //   Report Size (1)                   26
# 0x95, 0x01,                    //   Report Count (1)                  28
# 0x81, 0x01,                    //   Input (Cnst,Arr,Abs)              30
# 0x05, 0x01,                    //   Usage Page (Generic Desktop)      32
# 0x09, 0x30,                    //   Usage (X)                         34
# 0x09, 0x31,                    //   Usage (Y)                         36
# 0x09, 0x38,                    //   Usage (Wheel)                     38
# 0x15, 0x81,                    //   Logical Minimum (-127)            40
# 0x25, 0x7f,                    //   Logical Maximum (127)             42
# 0x75, 0x08,                    //   Report Size (8)                   44
# 0x95, 0x03,                    //   Report Count (3)                  46
# 0x81, 0x06,                    //   Input (Data,Var,Rel)              48
# 0xc0,                          //  End Collection                     50
# 0xc0,                          // End Collection                      51
#
R: 52 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 07 15 00 25 01 75 01 
95 07 81 02 75 01 95 01 81 01 05 01 09 30 09 31 09 38 15 81 25 7f 75 08 
95 03 81 06 c0 c0
N: A4Tech PS/2+USB Mouse
I: 3 09da 0006
#  Button: 0  0  0  0  0  0  0 | # | X:    0 | Y:    0 | Wheel:   -1
E: 000000.000000 4 00 00 00 ff
#  Button: 0  0  0  0  0  0  0 | # | X:    0 | Y:    0 | Wheel:   -1
E: 000000.071952 4 00 00 00 ff
#  Button: 0  0  0  0  0  0  0 | # | X:    0 | Y:    0 | Wheel:   -1
E: 000000.159957 4 00 00 00 ff
#  Button: 0  0  0  0  0  0  0 | # | X:    0 | Y:    0 | Wheel:    1
E: 000002.912232 4 00 00 00 01
#  Button: 0  0  0  0  0  0  0 | # | X:    0 | Y:    0 | Wheel:    1
E: 000002.952190 4 00 00 00 01
#  Button: 0  0  0  0  0  0  0 | # | X:    0 | Y:    0 | Wheel:    1
E: 000004.512359 4 00 00 00 01
#  Button: 0  0  0  0  0  0  0 | # | X:    0 | Y:    0 | Wheel:    1
E: 000004.584332 4 00 00 00 01
#  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:   -1
E: 000007.528626 4 40 00 00 ff
#  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:   -1
E: 000007.568577 4 40 00 00 ff
#  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:   -1
E: 000008.256395 4 40 00 00 ff
#  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:   -1
E: 000008.336669 4 40 00 00 ff
#  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:   -1
E: 000008.400649 4 40 00 00 ff
#  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:    1
E: 000010.936908 4 40 00 00 01
#  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:    1
E: 000010.984864 4 40 00 00 01
#  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:    1
E: 000011.056897 4 40 00 00 01
#  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:    1
E: 000011.528936 4 40 00 00 01
#  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:    1
E: 000011.616923 4 40 00 00 01

Cheers,
Igor

>> ---
>>   drivers/hid/hid-a4tech.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hid/hid-a4tech.c b/drivers/hid/hid-a4tech.c
>> index 9428ea7cdf8a..fafb9fa558e7 100644
>> --- a/drivers/hid/hid-a4tech.c
>> +++ b/drivers/hid/hid-a4tech.c
>> @@ -38,7 +38,7 @@ static int a4_input_mapped(struct hid_device *hdev, struct hid_input *hi,
>>   {
>>          struct a4tech_sc *a4 = hid_get_drvdata(hdev);
>>
>> -       if (usage->type == EV_REL && usage->code == REL_WHEEL)
>> +       if (usage->type == EV_REL && usage->code == REL_WHEEL_HI_RES)
>>                  set_bit(REL_HWHEEL, *bit);
>>
>>          if ((a4->quirks & A4_2WHEEL_MOUSE_HACK_7) && usage->hid == 0x00090007)
>> @@ -60,7 +60,7 @@ static int a4_event(struct hid_device *hdev, struct hid_field *field,
>>          input = field->hidinput->input;
>>
>>          if (a4->quirks & A4_2WHEEL_MOUSE_HACK_B8) {
>> -               if (usage->type == EV_REL && usage->code == REL_WHEEL) {
>> +               if (usage->type == EV_REL && usage->code == REL_WHEEL_HI_RES) {
>>                          a4->delayed_value = value;
>>                          return 1;
>>                  }
>> @@ -77,7 +77,7 @@ static int a4_event(struct hid_device *hdev, struct hid_field *field,
>>                  return 1;
>>          }
>>
>> -       if (usage->code == REL_WHEEL && a4->hw_wheel) {
>> +       if (usage->code == REL_WHEEL_HI_RES && a4->hw_wheel) {
>>                  input_event(input, usage->type, REL_HWHEEL, value);
>>                  return 1;
>>          }
>> --
>> 2.21.0
>>
Benjamin Tissoires May 3, 2019, 11:59 a.m. UTC | #4
Hi,

On Fri, May 3, 2019 at 11:43 AM Igor Kushnir <igorkuo@gmail.com> wrote:
>
> Hi Benjamin,
>
> On 5/3/19 10:36 AM, Benjamin Tissoires wrote:
> > Hi,
> >
> > On Thu, May 2, 2019 at 11:37 PM Błażej Szczygieł <spaz16@wp.pl> wrote:
> >>
> >> Since recent high resolution scrolling changes the A4Tech driver must
> >> check for the "REL_WHEEL_HI_RES" usage code.
> >>
> >> Fixes: 2dc702c991e3774af9d7ce410eef410ca9e2357e (HID: input: use the
> >> Resolution Multiplier for high-resolution scrolling)
> >>
> >> Signed-off-by: Błażej Szczygieł <spaz16@wp.pl>
> >
> > Thanks for the patch. I do not doubt this fixes the issues, but I
> > still wonder if we should not export REL_HWHEEL_HI_RES instead of
> > REL_HWHEEL events.
>
>
> If you mean exporting REL_HWHEEL_HI_RES instead of REL_HWHEEL from
> hid-a4tech.c, then it makes sense to me, though I do not know the code
> well enough to be certain.

Yep, that's what I meant. I am worried that userspace doesn't know
well how to deal with a device that mixes the new and old REL_WHEEL
events.

>
> Błażej and I have discussed the bug and the patch here:
> https://bugzilla.kernel.org/show_bug.cgi?id=203369

Oh cool.
Then we should add: "Link:
https://bugzilla.kernel.org/show_bug.cgi?id=203369" in the commit
description.

Also, given that the patch will likely see a v2, te format of the
"Fixes" tag is not correct: see
https://www.kernel.org/doc/html/v4.20/process/submitting-patches.html#describe-your-changes
(I have been notified that I tend to not follow the rules here, so I
am trying to do better here :-P )

>
> In summary: the patch fixes the bug for both our mice;
> the documentation in input/event-codes.rst states that
> REL_WHEEL, REL_HWHEEL "are legacy codes and REL_WHEEL_HI_RES and
> REL_HWHEEL_HI_RES should be preferred where available."
>
> > Also, I can not figure out how the events are processed by the kernel.
> > Could you attach a hid-recorder dump of the mouse wheels with
> > hid-recorder from https://gitlab.freedesktop.org/libevdev/hid-tools ?
> >
> > This should give me a better view of the mouse, and I could also add
> > it to the regression tests I am running for each commit.
> >
> > Cheers,
> > Benjamin
>
> After launching hid-recorder for my A4Tech WOP-49Z mouse under kernel
> 5.0.10 patched with Błażej's patch I:
> * scrolled the vertical wheel down ("Wheel: -1");
> * scrolled the vertical wheel up ("Wheel: 1");
> * scrolled the horizontal wheel "left" ("Wheel: -1");
> * scrolled the horizontal wheel "right" ("Wheel: 1").
> Note that the horizontal wheel is physically scrolled just like the
> vertical one in this mouse (forward/back), so "left" and "right" are the
> effects these scrollings make in applications when the kernel supports
> the mouse properly.
>
> $ sudo ./hid-recorder /dev/hidraw1
> # A4Tech PS/2+USB Mouse
> # 0x05, 0x01,                    // Usage Page (Generic Desktop)        0
> # 0x09, 0x02,                    // Usage (Mouse)                       2
> # 0xa1, 0x01,                    // Collection (Application)            4
> # 0x09, 0x01,                    //  Usage (Pointer)                    6
> # 0xa1, 0x00,                    //  Collection (Physical)              8
> # 0x05, 0x09,                    //   Usage Page (Button)               10
> # 0x19, 0x01,                    //   Usage Minimum (1)                 12
> # 0x29, 0x07,                    //   Usage Maximum (7)                 14
> # 0x15, 0x00,                    //   Logical Minimum (0)               16
> # 0x25, 0x01,                    //   Logical Maximum (1)               18
> # 0x75, 0x01,                    //   Report Size (1)                   20
> # 0x95, 0x07,                    //   Report Count (7)                  22
> # 0x81, 0x02,                    //   Input (Data,Var,Abs)              24
> # 0x75, 0x01,                    //   Report Size (1)                   26
> # 0x95, 0x01,                    //   Report Count (1)                  28
> # 0x81, 0x01,                    //   Input (Cnst,Arr,Abs)              30
> # 0x05, 0x01,                    //   Usage Page (Generic Desktop)      32
> # 0x09, 0x30,                    //   Usage (X)                         34
> # 0x09, 0x31,                    //   Usage (Y)                         36
> # 0x09, 0x38,                    //   Usage (Wheel)                     38
> # 0x15, 0x81,                    //   Logical Minimum (-127)            40
> # 0x25, 0x7f,                    //   Logical Maximum (127)             42
> # 0x75, 0x08,                    //   Report Size (8)                   44
> # 0x95, 0x03,                    //   Report Count (3)                  46
> # 0x81, 0x06,                    //   Input (Data,Var,Rel)              48
> # 0xc0,                          //  End Collection                     50
> # 0xc0,                          // End Collection                      51
> #
> R: 52 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 07 15 00 25 01 75 01
> 95 07 81 02 75 01 95 01 81 01 05 01 09 30 09 31 09 38 15 81 25 7f 75 08
> 95 03 81 06 c0 c0
> N: A4Tech PS/2+USB Mouse
> I: 3 09da 0006
> #  Button: 0  0  0  0  0  0  0 | # | X:    0 | Y:    0 | Wheel:   -1
> E: 000000.000000 4 00 00 00 ff
> #  Button: 0  0  0  0  0  0  0 | # | X:    0 | Y:    0 | Wheel:   -1
> E: 000000.071952 4 00 00 00 ff
> #  Button: 0  0  0  0  0  0  0 | # | X:    0 | Y:    0 | Wheel:   -1
> E: 000000.159957 4 00 00 00 ff
> #  Button: 0  0  0  0  0  0  0 | # | X:    0 | Y:    0 | Wheel:    1
> E: 000002.912232 4 00 00 00 01
> #  Button: 0  0  0  0  0  0  0 | # | X:    0 | Y:    0 | Wheel:    1
> E: 000002.952190 4 00 00 00 01
> #  Button: 0  0  0  0  0  0  0 | # | X:    0 | Y:    0 | Wheel:    1
> E: 000004.512359 4 00 00 00 01
> #  Button: 0  0  0  0  0  0  0 | # | X:    0 | Y:    0 | Wheel:    1
> E: 000004.584332 4 00 00 00 01
> #  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:   -1
> E: 000007.528626 4 40 00 00 ff
> #  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:   -1
> E: 000007.568577 4 40 00 00 ff
> #  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:   -1
> E: 000008.256395 4 40 00 00 ff
> #  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:   -1
> E: 000008.336669 4 40 00 00 ff
> #  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:   -1
> E: 000008.400649 4 40 00 00 ff
> #  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:    1
> E: 000010.936908 4 40 00 00 01
> #  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:    1
> E: 000010.984864 4 40 00 00 01
> #  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:    1
> E: 000011.056897 4 40 00 00 01
> #  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:    1
> E: 000011.528936 4 40 00 00 01
> #  Button: 0  0  0  0  0  0  1 | # | X:    0 | Y:    0 | Wheel:    1
> E: 000011.616923 4 40 00 00 01
>

OK, thanks both of you for your logs, this is helpful.
So just in case I need to come back later, the horizontal wheel is
"just" the normal wheel plus a modifier in the report.

Anyway, ideally, can we have a v2 of the patch with the 2 changes
requested above in the commit message and the introduction of
REL_HWHEEL_HI_RES events in addition to REL_HWHEEL?
REL_HWHEEL_HI_RES should report `120*value` and we should also keep
the reporting of REL_WHEEL as it is currently.

Peter, I grepped in the hid code, and it seems hid-cypress.c is having
the exact same issue. Sigh.

Cheers,
Benjamin
Peter Hutterer May 7, 2019, 5:01 a.m. UTC | #5
On Fri, May 03, 2019 at 01:59:23PM +0200, Benjamin Tissoires wrote:
> Hi,
> 
> On Fri, May 3, 2019 at 11:43 AM Igor Kushnir <igorkuo@gmail.com> wrote:
> >
> > Hi Benjamin,
> >
> > On 5/3/19 10:36 AM, Benjamin Tissoires wrote:
> > > Hi,
> > >
> > > On Thu, May 2, 2019 at 11:37 PM Błażej Szczygieł <spaz16@wp.pl> wrote:
> > >>
> > >> Since recent high resolution scrolling changes the A4Tech driver must
> > >> check for the "REL_WHEEL_HI_RES" usage code.
> > >>
> > >> Fixes: 2dc702c991e3774af9d7ce410eef410ca9e2357e (HID: input: use the
> > >> Resolution Multiplier for high-resolution scrolling)
> > >>
> > >> Signed-off-by: Błażej Szczygieł <spaz16@wp.pl>
> > >
> > > Thanks for the patch. I do not doubt this fixes the issues, but I
> > > still wonder if we should not export REL_HWHEEL_HI_RES instead of
> > > REL_HWHEEL events.
> >
> >
> > If you mean exporting REL_HWHEEL_HI_RES instead of REL_HWHEEL from
> > hid-a4tech.c, then it makes sense to me, though I do not know the code
> > well enough to be certain.
> 
> Yep, that's what I meant. I am worried that userspace doesn't know
> well how to deal with a device that mixes the new and old REL_WHEEL
> events.

sorry, I'm not sure what you mean here. The new events are always mixed with
the old ones anyway, and both should be treated as separate event streams.
The kernel interface to userspace is fairly easy to deal with, it's the rest
that's a bit of mess.

[..]

> >
> 
> OK, thanks both of you for your logs, this is helpful.
> So just in case I need to come back later, the horizontal wheel is
> "just" the normal wheel plus a modifier in the report.
> 
> Anyway, ideally, can we have a v2 of the patch with the 2 changes
> requested above in the commit message and the introduction of
> REL_HWHEEL_HI_RES events in addition to REL_HWHEEL?
> REL_HWHEEL_HI_RES should report `120*value` and we should also keep
> the reporting of REL_WHEEL as it is currently.
> 
> Peter, I grepped in the hid code, and it seems hid-cypress.c is having
> the exact same issue. Sigh.

yeah, I found that too when grepping through it. seems to be the only other
one though and we can use Błażej's patch as boilerplate once it's done.

Cheers,
   Peter
Błażej Szczygieł May 12, 2019, 8:59 p.m. UTC | #6
Hi,

On 07.05.2019 at 07:01, Peter Hutterer wrote:
> On Fri, May 03, 2019 at 01:59:23PM +0200, Benjamin Tissoires wrote:
>> Hi,
>>
>> On Fri, May 3, 2019 at 11:43 AM Igor Kushnir <igorkuo@gmail.com> wrote:
>>>
>>> Hi Benjamin,
>>>
>>> On 5/3/19 10:36 AM, Benjamin Tissoires wrote:
>>>> Hi,
>>>>
>>>> On Thu, May 2, 2019 at 11:37 PM Błażej Szczygieł <spaz16@wp.pl> wrote:
>>>>>
>>>>> Since recent high resolution scrolling changes the A4Tech driver must
>>>>> check for the "REL_WHEEL_HI_RES" usage code.
>>>>>
>>>>> Fixes: 2dc702c991e3774af9d7ce410eef410ca9e2357e (HID: input: use the
>>>>> Resolution Multiplier for high-resolution scrolling)
>>>>>
>>>>> Signed-off-by: Błażej Szczygieł <spaz16@wp.pl>
>>>>
>>>> Thanks for the patch. I do not doubt this fixes the issues, but I
>>>> still wonder if we should not export REL_HWHEEL_HI_RES instead of
>>>> REL_HWHEEL events.
>>>
>>>
>>> If you mean exporting REL_HWHEEL_HI_RES instead of REL_HWHEEL from
>>> hid-a4tech.c, then it makes sense to me, though I do not know the code
>>> well enough to be certain.
>>
>> Yep, that's what I meant. I am worried that userspace doesn't know
>> well how to deal with a device that mixes the new and old REL_WHEEL
>> events.
> 
> sorry, I'm not sure what you mean here. The new events are always mixed with
> the old ones anyway, and both should be treated as separate event streams.
> The kernel interface to userspace is fairly easy to deal with, it's the rest
> that's a bit of mess.
> 
> [..]
> 
>>>
>>
>> OK, thanks both of you for your logs, this is helpful.
>> So just in case I need to come back later, the horizontal wheel is
>> "just" the normal wheel plus a modifier in the report.
>>
>> Anyway, ideally, can we have a v2 of the patch with the 2 changes
>> requested above in the commit message and the introduction of
>> REL_HWHEEL_HI_RES events in addition to REL_HWHEEL?
>> REL_HWHEEL_HI_RES should report `120*value` and we should also keep
>> the reporting of REL_WHEEL as it is currently.
>>
>> Peter, I grepped in the hid code, and it seems hid-cypress.c is having
>> the exact same issue. Sigh.
> 
> yeah, I found that too when grepping through it. seems to be the only other
> one though and we can use Błażej's patch as boilerplate once it's done.

Peter, I also found comparison of "usage->code ==" with "REL_HWHEEL"
and "REL_WHEEL" in hid-lenovo.c, hid-apple.c, hid-ezkey.c, hid-lg.c.
Unfortunatelly, I don't have such devices to test :(

Cheers,
Błażej.
diff mbox series

Patch

diff --git a/drivers/hid/hid-a4tech.c b/drivers/hid/hid-a4tech.c
index 9428ea7cdf8a..fafb9fa558e7 100644
--- a/drivers/hid/hid-a4tech.c
+++ b/drivers/hid/hid-a4tech.c
@@ -38,7 +38,7 @@  static int a4_input_mapped(struct hid_device *hdev, struct hid_input *hi,
 {
 	struct a4tech_sc *a4 = hid_get_drvdata(hdev);
 
-	if (usage->type == EV_REL && usage->code == REL_WHEEL)
+	if (usage->type == EV_REL && usage->code == REL_WHEEL_HI_RES)
 		set_bit(REL_HWHEEL, *bit);
 
 	if ((a4->quirks & A4_2WHEEL_MOUSE_HACK_7) && usage->hid == 0x00090007)
@@ -60,7 +60,7 @@  static int a4_event(struct hid_device *hdev, struct hid_field *field,
 	input = field->hidinput->input;
 
 	if (a4->quirks & A4_2WHEEL_MOUSE_HACK_B8) {
-		if (usage->type == EV_REL && usage->code == REL_WHEEL) {
+		if (usage->type == EV_REL && usage->code == REL_WHEEL_HI_RES) {
 			a4->delayed_value = value;
 			return 1;
 		}
@@ -77,7 +77,7 @@  static int a4_event(struct hid_device *hdev, struct hid_field *field,
 		return 1;
 	}
 
-	if (usage->code == REL_WHEEL && a4->hw_wheel) {
+	if (usage->code == REL_WHEEL_HI_RES && a4->hw_wheel) {
 		input_event(input, usage->type, REL_HWHEEL, value);
 		return 1;
 	}