| Message ID | 20190610185343.27614-1-nsaenzjulienne@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: input: fix a4tech horizontal wheel custom usage id | expand |
Hi Nicolas, On Mon, Jun 10, 2019 at 8:54 PM Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote: > > Some a4tech mice use the 'GenericDesktop.00b8' usage id to inform > whether the previous wheel report was horizontal or vertical. Before > c01908a14bf73 ("HID: input: add mapping for "Toggle Display" key") this > usage id was being mapped to 'Relative.Misc'. After the patch it's > simply ignored (usage->type == 0 & usage->code == 0). Checking the HID > Usage Tables it turns out it's a reserved usage_id, so it makes sense to > map it the way it was. Ultimately this makes hid-a4tech ignore the > WHEEL/HWHEEL selection event, as it has no usage->type. > > The patch reverts the handling of the usage id back to it's previous > behavior. Hmm, if A4Tech is using a reserved usage, we shouldn't fix this in hid-input.c but in hid-a4tech instead. Because you won't know when someone else in the HID consortium will remap this usage to some other random axis, and your mouse will be broken again. How about you add a .input_mapping callback in hid-a4tech and map this usage there to your needs? This way you will be sure that such a situation will not happen again. Cheers, Benjamin > > Fixes: c01908a14bf73 ("HID: input: add mapping for "Toggle Display" key") > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > --- > drivers/hid/hid-input.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c > index 63855f275a38..6a956d5a195e 100644 > --- a/drivers/hid/hid-input.c > +++ b/drivers/hid/hid-input.c > @@ -671,7 +671,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel > if ((usage->hid & 0xf0) == 0xb0) { /* SC - Display */ > switch (usage->hid & 0xf) { > case 0x05: map_key_clear(KEY_SWITCHVIDEOMODE); break; > - default: goto ignore; > + default: goto unknown; > } > break; > } > -- > 2.21.0 >
On Tue, 2019-06-11 at 10:43 +0200, Benjamin Tissoires wrote: > Hi Nicolas, > > On Mon, Jun 10, 2019 at 8:54 PM Nicolas Saenz Julienne > <nsaenzjulienne@suse.de> wrote: > > Some a4tech mice use the 'GenericDesktop.00b8' usage id to inform > > whether the previous wheel report was horizontal or vertical. Before > > c01908a14bf73 ("HID: input: add mapping for "Toggle Display" key") this > > usage id was being mapped to 'Relative.Misc'. After the patch it's > > simply ignored (usage->type == 0 & usage->code == 0). Checking the HID > > Usage Tables it turns out it's a reserved usage_id, so it makes sense to > > map it the way it was. Ultimately this makes hid-a4tech ignore the > > WHEEL/HWHEEL selection event, as it has no usage->type. > > > > The patch reverts the handling of the usage id back to it's previous > > behavior. > > Hmm, if A4Tech is using a reserved usage, we shouldn't fix this in > hid-input.c but in hid-a4tech instead. > Because you won't know when someone else in the HID consortium will > remap this usage to some other random axis, and your mouse will be > broken again. > > How about you add a .input_mapping callback in hid-a4tech and map this > usage there to your needs? This way you will be sure that such a > situation will not happen again. I agree it would be a cleaner solution. In summary the first report indicates the wheel relative value, the second the orientation. The first report is already being mapped to REL_WHEEL and REL_WHEEL (or the high-res versions), but what would be a correct code for the second report? The way I see it, we shouldn't map it to anything. And then catch both events in the custom driver to build the input_event() accordinlgy (as it's almost being done already). Is this somewhat correct? I'll send a followup patch anyway so we have something more tangible comment on. > > Fixes: c01908a14bf73 ("HID: input: add mapping for "Toggle Display" key") > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > > --- > > drivers/hid/hid-input.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c > > index 63855f275a38..6a956d5a195e 100644 > > --- a/drivers/hid/hid-input.c > > +++ b/drivers/hid/hid-input.c > > @@ -671,7 +671,7 @@ static void hidinput_configure_usage(struct hid_input > > *hidinput, struct hid_fiel > > if ((usage->hid & 0xf0) == 0xb0) { /* SC - Display */ > > switch (usage->hid & 0xf) { > > case 0x05: map_key_clear(KEY_SWITCHVIDEOMODE); > > break; > > - default: goto ignore; > > + default: goto unknown; > > } > > break; > > } > > -- > > 2.21.0 > >
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index 63855f275a38..6a956d5a195e 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -671,7 +671,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel if ((usage->hid & 0xf0) == 0xb0) { /* SC - Display */ switch (usage->hid & 0xf) { case 0x05: map_key_clear(KEY_SWITCHVIDEOMODE); break; - default: goto ignore; + default: goto unknown; } break; }
Some a4tech mice use the 'GenericDesktop.00b8' usage id to inform whether the previous wheel report was horizontal or vertical. Before c01908a14bf73 ("HID: input: add mapping for "Toggle Display" key") this usage id was being mapped to 'Relative.Misc'. After the patch it's simply ignored (usage->type == 0 & usage->code == 0). Checking the HID Usage Tables it turns out it's a reserved usage_id, so it makes sense to map it the way it was. Ultimately this makes hid-a4tech ignore the WHEEL/HWHEEL selection event, as it has no usage->type. The patch reverts the handling of the usage id back to it's previous behavior. Fixes: c01908a14bf73 ("HID: input: add mapping for "Toggle Display" key") Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> --- drivers/hid/hid-input.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)