| Message ID | 20220320190602.7484-1-jose.exposito89@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: multitouch: fix Dell Precision 7550 and 7750 button type | expand |
thx for working on this On 20.03.22 20:06, José Expósito wrote: > The touchpad present in the Dell Precision 7550 and 7750 laptops > reports a HID_DG_BUTTONTYPE of type MT_BUTTONTYPE_CLICKPAD. However, > the device is not a clickpad, it is a touchpad with physical buttons. > > In order to fix this issue, a quirk for the device was introduced in > libinput [1] [2] to disable the INPUT_PROP_BUTTONPAD property: > > [Precision 7x50 Touchpad] > MatchBus=i2c > MatchUdevType=touchpad > MatchDMIModalias=dmi:*svnDellInc.:pnPrecision7?50* > AttrInputPropDisable=INPUT_PROP_BUTTONPAD > > However, because of the change introduced in 37ef4c19b4 ("Input: clear > BTN_RIGHT/MIDDLE on buttonpads") the BTN_RIGHT key bit is not mapped > anymore breaking the device right click button. > > In order to fix the issue, create a quirk for the device forcing its > button type to touchpad regardless of the value reported by the > firmware. > > [1] https://gitlab.freedesktop.org/libinput/libinput/-/merge_requests/481 > [2] https://bugzilla.redhat.com/show_bug.cgi?id=1868789 Nitpicking: those should be "Link" tags, as explained in 'Documentation/process/submitting-patches.rst' and 'Documentation/process/5.Posting.rst'? Never tried, but if you want to make them footnote-style this should work according to the docs: Link: https://gitlab.freedesktop.org/libinput/libinput/-/merge_requests/481 [1] Link: https://bugzilla.redhat.com/show_bug.cgi?id=1868789 [2] In anyone wonders why I care: there are internal and publicly used tools and scripts out there that reply on proper "Link" tags. I don't known how many, but there is at least one public tool I'm running that cares: regzbot, my regression tracking bot, which I use to track Linux kernel regressions and generate the regression reports sent to Linus. Proper "Link:" tags allow the bot to automatically connect regression reports with fixes being posted or applied to resolve the particular regression -- which makes regression tracking a whole lot easier and feasible for the Linux kernel. That's why it's a great help for me if people set proper "Link" tags. While at it, let me tell regzbot about this thread: #regzbot ^backmonitor: https://linux-regtracking.leemhuis.info/regzbot/regression/s5htubv32s8.wl-tiwai@suse.de/ > Fixes: 37ef4c19b4 ("Input: clear BTN_RIGHT/MIDDLE on buttonpads") > Signed-off-by: José Expósito <jose.exposito89@gmail.com> > [...] Ciao, Thorsten
Hi José, On Sun, Mar 20, 2022 at 8:06 PM José Expósito <jose.exposito89@gmail.com> wrote: > > The touchpad present in the Dell Precision 7550 and 7750 laptops > reports a HID_DG_BUTTONTYPE of type MT_BUTTONTYPE_CLICKPAD. However, > the device is not a clickpad, it is a touchpad with physical buttons. > > In order to fix this issue, a quirk for the device was introduced in > libinput [1] [2] to disable the INPUT_PROP_BUTTONPAD property: > > [Precision 7x50 Touchpad] > MatchBus=i2c > MatchUdevType=touchpad > MatchDMIModalias=dmi:*svnDellInc.:pnPrecision7?50* > AttrInputPropDisable=INPUT_PROP_BUTTONPAD > > However, because of the change introduced in 37ef4c19b4 ("Input: clear > BTN_RIGHT/MIDDLE on buttonpads") the BTN_RIGHT key bit is not mapped > anymore breaking the device right click button. > > In order to fix the issue, create a quirk for the device forcing its > button type to touchpad regardless of the value reported by the > firmware. > > [1] https://gitlab.freedesktop.org/libinput/libinput/-/merge_requests/481 > [2] https://bugzilla.redhat.com/show_bug.cgi?id=1868789 > > Fixes: 37ef4c19b4 ("Input: clear BTN_RIGHT/MIDDLE on buttonpads") > Signed-off-by: José Expósito <jose.exposito89@gmail.com> > --- Thanks for the patch. However, I'd like to put this one on hold for a bit. I am discussing it right now with Peter and we are trying to see what are the possible implications of starting to fix those in the kernel one by one. So Jiri, please hold on before applying this one. Cheers, Benjamin > drivers/hid/hid-ids.h | 3 +++ > drivers/hid/hid-multitouch.c | 20 ++++++++++++++++++++ > 2 files changed, 23 insertions(+) > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > index 78bd3ddda442..6cf7a5b6835b 100644 > --- a/drivers/hid/hid-ids.h > +++ b/drivers/hid/hid-ids.h > @@ -287,6 +287,9 @@ > > #define USB_VENDOR_ID_CIDC 0x1677 > > +#define USB_VENDOR_ID_CIRQUE_CORP 0x0488 > +#define USB_DEVICE_ID_DELL_PRECISION_7X50 0x120A > + > #define USB_VENDOR_ID_CJTOUCH 0x24b8 > #define USB_DEVICE_ID_CJTOUCH_MULTI_TOUCH_0020 0x0020 > #define USB_DEVICE_ID_CJTOUCH_MULTI_TOUCH_0040 0x0040 > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c > index 99eabfb4145b..f012cf8e0b8c 100644 > --- a/drivers/hid/hid-multitouch.c > +++ b/drivers/hid/hid-multitouch.c > @@ -71,6 +71,7 @@ MODULE_LICENSE("GPL"); > #define MT_QUIRK_SEPARATE_APP_REPORT BIT(19) > #define MT_QUIRK_FORCE_MULTI_INPUT BIT(20) > #define MT_QUIRK_DISABLE_WAKEUP BIT(21) > +#define MT_QUIRK_BUTTONTYPE_TOUCHPAD BIT(22) > > #define MT_INPUTMODE_TOUCHSCREEN 0x02 > #define MT_INPUTMODE_TOUCHPAD 0x03 > @@ -194,6 +195,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app); > #define MT_CLS_WIN_8_FORCE_MULTI_INPUT 0x0015 > #define MT_CLS_WIN_8_DISABLE_WAKEUP 0x0016 > #define MT_CLS_WIN_8_NO_STICKY_FINGERS 0x0017 > +#define MT_CLS_BUTTONTYPE_TOUCHPAD 0x0018 > > /* vendor specific classes */ > #define MT_CLS_3M 0x0101 > @@ -302,6 +304,15 @@ static const struct mt_class mt_classes[] = { > MT_QUIRK_CONTACT_CNT_ACCURATE | > MT_QUIRK_WIN8_PTP_BUTTONS, > .export_all_inputs = true }, > + { .name = MT_CLS_BUTTONTYPE_TOUCHPAD, > + .quirks = MT_QUIRK_ALWAYS_VALID | > + MT_QUIRK_IGNORE_DUPLICATES | > + MT_QUIRK_HOVERING | > + MT_QUIRK_CONTACT_CNT_ACCURATE | > + MT_QUIRK_STICKY_FINGERS | > + MT_QUIRK_WIN8_PTP_BUTTONS | > + MT_QUIRK_BUTTONTYPE_TOUCHPAD, > + .export_all_inputs = true }, > > /* > * vendor specific classes > @@ -1286,6 +1297,9 @@ static int mt_touch_input_configured(struct hid_device *hdev, > (app->buttons_count == 1)) > td->is_buttonpad = true; > > + if (app->quirks & MT_QUIRK_BUTTONTYPE_TOUCHPAD) > + td->is_buttonpad = false; > + > if (td->is_buttonpad) > __set_bit(INPUT_PROP_BUTTONPAD, input->propbit); > > @@ -1872,6 +1886,12 @@ static const struct hid_device_id mt_devices[] = { > MT_USB_DEVICE(USB_VENDOR_ID_CHUNGHWAT, > USB_DEVICE_ID_CHUNGHWAT_MULTITOUCH) }, > > + /* Cirque Corp (Dell Precision 7550 and 7750 touchpad) */ > + { .driver_data = MT_CLS_BUTTONTYPE_TOUCHPAD, > + HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8, > + USB_VENDOR_ID_CIRQUE_CORP, > + USB_DEVICE_ID_DELL_PRECISION_7X50) }, > + > /* CJTouch panels */ > { .driver_data = MT_CLS_NSMU, > MT_USB_DEVICE(USB_VENDOR_ID_CJTOUCH, > -- > 2.25.1 >
On Mon, Mar 21, 2022 at 10:25 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Hi José, > > On Sun, Mar 20, 2022 at 8:06 PM José Expósito <jose.exposito89@gmail.com> wrote: > > > > The touchpad present in the Dell Precision 7550 and 7750 laptops > > reports a HID_DG_BUTTONTYPE of type MT_BUTTONTYPE_CLICKPAD. However, > > the device is not a clickpad, it is a touchpad with physical buttons. > > > > In order to fix this issue, a quirk for the device was introduced in > > libinput [1] [2] to disable the INPUT_PROP_BUTTONPAD property: > > > > [Precision 7x50 Touchpad] > > MatchBus=i2c > > MatchUdevType=touchpad > > MatchDMIModalias=dmi:*svnDellInc.:pnPrecision7?50* > > AttrInputPropDisable=INPUT_PROP_BUTTONPAD > > > > However, because of the change introduced in 37ef4c19b4 ("Input: clear > > BTN_RIGHT/MIDDLE on buttonpads") the BTN_RIGHT key bit is not mapped > > anymore breaking the device right click button. > > > > In order to fix the issue, create a quirk for the device forcing its > > button type to touchpad regardless of the value reported by the > > firmware. > > > > [1] https://gitlab.freedesktop.org/libinput/libinput/-/merge_requests/481 > > [2] https://bugzilla.redhat.com/show_bug.cgi?id=1868789 > > > > Fixes: 37ef4c19b4 ("Input: clear BTN_RIGHT/MIDDLE on buttonpads") > > Signed-off-by: José Expósito <jose.exposito89@gmail.com> > > --- > > Thanks for the patch. > However, I'd like to put this one on hold for a bit. I am discussing > it right now with Peter and we are trying to see what are the possible > implications of starting to fix those in the kernel one by one. > > So Jiri, please hold on before applying this one. Giving a little bit more context here (and quoting Peter). """ The problem with [37ef4c19b4] is that it removes functionality - before a clickpad was falsely advertised but the button worked, now in the affected devices it simply no longer works because the button code gets filtered. And user-space can't work around this. ... So the main question remains: why are we doing this? And the answer here is: because libinput can't handle clickpads with right buttons. But that's not really true either, libinput just doesn't want to, and for no other reason than that it's easier to handle it this way. """ So basically, we tried to fix a choice on libinput assuming that all devices are perfect, for the only sake of making it easy for libinput. But the solution prevents further tweaks, and we then need to manually quirk devices in the kernel which involves a slightly heavier difficulty for end users than just dropping a config file or changing a setting in their UI. With that said, this patch is: Nacked-by: me José, could you send a revert of 37ef4c19b4, and add "Cc: stable@vger.kernel.org" and all the other tags for the regression tracker bot? Thanks in advance. Cheers, Benjamin > > Cheers, > Benjamin > > > drivers/hid/hid-ids.h | 3 +++ > > drivers/hid/hid-multitouch.c | 20 ++++++++++++++++++++ > > 2 files changed, 23 insertions(+) > > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > > index 78bd3ddda442..6cf7a5b6835b 100644 > > --- a/drivers/hid/hid-ids.h > > +++ b/drivers/hid/hid-ids.h > > @@ -287,6 +287,9 @@ > > > > #define USB_VENDOR_ID_CIDC 0x1677 > > > > +#define USB_VENDOR_ID_CIRQUE_CORP 0x0488 > > +#define USB_DEVICE_ID_DELL_PRECISION_7X50 0x120A > > + > > #define USB_VENDOR_ID_CJTOUCH 0x24b8 > > #define USB_DEVICE_ID_CJTOUCH_MULTI_TOUCH_0020 0x0020 > > #define USB_DEVICE_ID_CJTOUCH_MULTI_TOUCH_0040 0x0040 > > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c > > index 99eabfb4145b..f012cf8e0b8c 100644 > > --- a/drivers/hid/hid-multitouch.c > > +++ b/drivers/hid/hid-multitouch.c > > @@ -71,6 +71,7 @@ MODULE_LICENSE("GPL"); > > #define MT_QUIRK_SEPARATE_APP_REPORT BIT(19) > > #define MT_QUIRK_FORCE_MULTI_INPUT BIT(20) > > #define MT_QUIRK_DISABLE_WAKEUP BIT(21) > > +#define MT_QUIRK_BUTTONTYPE_TOUCHPAD BIT(22) > > > > #define MT_INPUTMODE_TOUCHSCREEN 0x02 > > #define MT_INPUTMODE_TOUCHPAD 0x03 > > @@ -194,6 +195,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app); > > #define MT_CLS_WIN_8_FORCE_MULTI_INPUT 0x0015 > > #define MT_CLS_WIN_8_DISABLE_WAKEUP 0x0016 > > #define MT_CLS_WIN_8_NO_STICKY_FINGERS 0x0017 > > +#define MT_CLS_BUTTONTYPE_TOUCHPAD 0x0018 > > > > /* vendor specific classes */ > > #define MT_CLS_3M 0x0101 > > @@ -302,6 +304,15 @@ static const struct mt_class mt_classes[] = { > > MT_QUIRK_CONTACT_CNT_ACCURATE | > > MT_QUIRK_WIN8_PTP_BUTTONS, > > .export_all_inputs = true }, > > + { .name = MT_CLS_BUTTONTYPE_TOUCHPAD, > > + .quirks = MT_QUIRK_ALWAYS_VALID | > > + MT_QUIRK_IGNORE_DUPLICATES | > > + MT_QUIRK_HOVERING | > > + MT_QUIRK_CONTACT_CNT_ACCURATE | > > + MT_QUIRK_STICKY_FINGERS | > > + MT_QUIRK_WIN8_PTP_BUTTONS | > > + MT_QUIRK_BUTTONTYPE_TOUCHPAD, > > + .export_all_inputs = true }, > > > > /* > > * vendor specific classes > > @@ -1286,6 +1297,9 @@ static int mt_touch_input_configured(struct hid_device *hdev, > > (app->buttons_count == 1)) > > td->is_buttonpad = true; > > > > + if (app->quirks & MT_QUIRK_BUTTONTYPE_TOUCHPAD) > > + td->is_buttonpad = false; > > + > > if (td->is_buttonpad) > > __set_bit(INPUT_PROP_BUTTONPAD, input->propbit); > > > > @@ -1872,6 +1886,12 @@ static const struct hid_device_id mt_devices[] = { > > MT_USB_DEVICE(USB_VENDOR_ID_CHUNGHWAT, > > USB_DEVICE_ID_CHUNGHWAT_MULTITOUCH) }, > > > > + /* Cirque Corp (Dell Precision 7550 and 7750 touchpad) */ > > + { .driver_data = MT_CLS_BUTTONTYPE_TOUCHPAD, > > + HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8, > > + USB_VENDOR_ID_CIRQUE_CORP, > > + USB_DEVICE_ID_DELL_PRECISION_7X50) }, > > + > > /* CJTouch panels */ > > { .driver_data = MT_CLS_NSMU, > > MT_USB_DEVICE(USB_VENDOR_ID_CJTOUCH, > > -- > > 2.25.1 > >
On Mon, Mar 21, 2022 at 07:22:45PM +0100, Benjamin Tissoires wrote: > Giving a little bit more context here (and quoting Peter). > > """ > The problem with [37ef4c19b4] is that it removes functionality - > before a clickpad was falsely advertised but the button worked, now in > the affected devices it simply no longer works because the button code > gets filtered. And user-space can't work around this. > ... > So the main question remains: why are we doing this? > > And the answer here is: because libinput can't handle clickpads with > right buttons. But that's not really true either, libinput just > doesn't want to, and for no other reason than that it's easier to > handle it this way. > """ > > So basically, we tried to fix a choice on libinput assuming that all > devices are perfect, for the only sake of making it easy for libinput. > But the solution prevents further tweaks, and we then need to manually > quirk devices in the kernel which involves a slightly heavier > difficulty for end users than just dropping a config file or changing > a setting in their UI. > > With that said, this patch is: > Nacked-by: me > > José, could you send a revert of 37ef4c19b4, and add "Cc: > stable@vger.kernel.org" and all the other tags for the regression > tracker bot? Sure, for reference, this patch should be ignored in favor of: https://lore.kernel.org/linux-input/20220321184404.20025-1-jose.exposito89@gmail.com/T/ Jose
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index 78bd3ddda442..6cf7a5b6835b 100644 --- a/drivers/hid/hid-ids.h +++ b/drivers/hid/hid-ids.h @@ -287,6 +287,9 @@ #define USB_VENDOR_ID_CIDC 0x1677 +#define USB_VENDOR_ID_CIRQUE_CORP 0x0488 +#define USB_DEVICE_ID_DELL_PRECISION_7X50 0x120A + #define USB_VENDOR_ID_CJTOUCH 0x24b8 #define USB_DEVICE_ID_CJTOUCH_MULTI_TOUCH_0020 0x0020 #define USB_DEVICE_ID_CJTOUCH_MULTI_TOUCH_0040 0x0040 diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 99eabfb4145b..f012cf8e0b8c 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -71,6 +71,7 @@ MODULE_LICENSE("GPL"); #define MT_QUIRK_SEPARATE_APP_REPORT BIT(19) #define MT_QUIRK_FORCE_MULTI_INPUT BIT(20) #define MT_QUIRK_DISABLE_WAKEUP BIT(21) +#define MT_QUIRK_BUTTONTYPE_TOUCHPAD BIT(22) #define MT_INPUTMODE_TOUCHSCREEN 0x02 #define MT_INPUTMODE_TOUCHPAD 0x03 @@ -194,6 +195,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app); #define MT_CLS_WIN_8_FORCE_MULTI_INPUT 0x0015 #define MT_CLS_WIN_8_DISABLE_WAKEUP 0x0016 #define MT_CLS_WIN_8_NO_STICKY_FINGERS 0x0017 +#define MT_CLS_BUTTONTYPE_TOUCHPAD 0x0018 /* vendor specific classes */ #define MT_CLS_3M 0x0101 @@ -302,6 +304,15 @@ static const struct mt_class mt_classes[] = { MT_QUIRK_CONTACT_CNT_ACCURATE | MT_QUIRK_WIN8_PTP_BUTTONS, .export_all_inputs = true }, + { .name = MT_CLS_BUTTONTYPE_TOUCHPAD, + .quirks = MT_QUIRK_ALWAYS_VALID | + MT_QUIRK_IGNORE_DUPLICATES | + MT_QUIRK_HOVERING | + MT_QUIRK_CONTACT_CNT_ACCURATE | + MT_QUIRK_STICKY_FINGERS | + MT_QUIRK_WIN8_PTP_BUTTONS | + MT_QUIRK_BUTTONTYPE_TOUCHPAD, + .export_all_inputs = true }, /* * vendor specific classes @@ -1286,6 +1297,9 @@ static int mt_touch_input_configured(struct hid_device *hdev, (app->buttons_count == 1)) td->is_buttonpad = true; + if (app->quirks & MT_QUIRK_BUTTONTYPE_TOUCHPAD) + td->is_buttonpad = false; + if (td->is_buttonpad) __set_bit(INPUT_PROP_BUTTONPAD, input->propbit); @@ -1872,6 +1886,12 @@ static const struct hid_device_id mt_devices[] = { MT_USB_DEVICE(USB_VENDOR_ID_CHUNGHWAT, USB_DEVICE_ID_CHUNGHWAT_MULTITOUCH) }, + /* Cirque Corp (Dell Precision 7550 and 7750 touchpad) */ + { .driver_data = MT_CLS_BUTTONTYPE_TOUCHPAD, + HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8, + USB_VENDOR_ID_CIRQUE_CORP, + USB_DEVICE_ID_DELL_PRECISION_7X50) }, + /* CJTouch panels */ { .driver_data = MT_CLS_NSMU, MT_USB_DEVICE(USB_VENDOR_ID_CJTOUCH,
The touchpad present in the Dell Precision 7550 and 7750 laptops reports a HID_DG_BUTTONTYPE of type MT_BUTTONTYPE_CLICKPAD. However, the device is not a clickpad, it is a touchpad with physical buttons. In order to fix this issue, a quirk for the device was introduced in libinput [1] [2] to disable the INPUT_PROP_BUTTONPAD property: [Precision 7x50 Touchpad] MatchBus=i2c MatchUdevType=touchpad MatchDMIModalias=dmi:*svnDellInc.:pnPrecision7?50* AttrInputPropDisable=INPUT_PROP_BUTTONPAD However, because of the change introduced in 37ef4c19b4 ("Input: clear BTN_RIGHT/MIDDLE on buttonpads") the BTN_RIGHT key bit is not mapped anymore breaking the device right click button. In order to fix the issue, create a quirk for the device forcing its button type to touchpad regardless of the value reported by the firmware. [1] https://gitlab.freedesktop.org/libinput/libinput/-/merge_requests/481 [2] https://bugzilla.redhat.com/show_bug.cgi?id=1868789 Fixes: 37ef4c19b4 ("Input: clear BTN_RIGHT/MIDDLE on buttonpads") Signed-off-by: José Expósito <jose.exposito89@gmail.com> --- drivers/hid/hid-ids.h | 3 +++ drivers/hid/hid-multitouch.c | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+)