| 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 |
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 >
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 >>
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 >>
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
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
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 --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; }
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(-)