| Message ID | 20201102133658.4410-1-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Commit | ee5e58418a854755201eb4952b1230d873a457d5 |
| Headers | show |
| Series | [1/3] HID: logitech-dj: Handle quad/bluetooth keyboards with a builtin trackpad | expand |
Hi, On 11/2/20 2:36 PM, Hans de Goede wrote: > Some quad/bluetooth keyboards, such as the Dinovo Edge (Y-RAY81) have a > builtin touchpad. In this case when asking the receiver for paired devices, > we get only 1 paired device with a device_type of REPORT_TYPE_KEYBOARD. > > This means that we do not instantiate a second dj_hiddev for the mouse > (as we normally would) and thus there is no place for us to forward the > mouse input reports to, causing the touchpad part of the keyboard to not > work. > > There is no way for us to detect these keyboards, so this commit adds > an array with device-ids for such keyboards and when a keyboard is on > this list it adds STD_MOUSE to the reports_supported bitmap for the > dj_hiddev created for the keyboard fixing the touchpad not working. > > Using a list of device-ids for this is not ideal, but there are only > very few such keyboards so this should be fine. Besides the Dinovo Edge, > other known wireless Logitech keyboards with a builtin touchpad are: > > * Dinovo Mini (TODO add its device-id to the list) Benjamin, you have a Dinovo Mini, right ? It looks like that is using the same quad/bluetooth combo receiver as the Dinovo Edge, but then with slightly different USB ids, which means that atm we are not using the logitech-dj driver for it. But the dongles appear to be interchangeable I can pair the Dinovo Edge with both the MX5000 and the MX5500 dongles which I have, so someone who mixes up dongles (or gets a spare one) could end up using the Dinovo Mini with a dongle which is already handled by the logitech-dj driver. As such it would be good if you can add the Dinovo Mini to the device-id list this patch introduces (or if you tell me the device-id I can do a v2 adding it depending on the timing). Also I think you should probably add the USB-ids for your Dinovo dongle to the logitech-dj driver. This will allow you to verify that adding the device-id is necessary and also will give you battery status reporting while used in USB HID proxy mode. Last you may want to check battery-status reporting in Bluetooth mode, and maybe also make the logitech-hidpp driver handle the Dinovo Mini in bluetooth mode, as at least on the Dinovo Edge the standard HID battery reporting done in bluetooth mode (and not in HID proxy mode interesting enough) seems to be broken. Regards, Hans > * K400 (uses a unifying receiver so is not affected) > * K600 (uses a unifying receiver so is not affected) > > Cc: stable@vger.kernel.org > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1811424 > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/hid/hid-logitech-dj.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c > index ea1e40530f85..9ed7260b9593 100644 > --- a/drivers/hid/hid-logitech-dj.c > +++ b/drivers/hid/hid-logitech-dj.c > @@ -867,11 +867,23 @@ static void logi_dj_recv_queue_notification(struct dj_receiver_dev *djrcv_dev, > schedule_work(&djrcv_dev->work); > } > > +/* > + * Some quad/bluetooth keyboards have a builtin touchpad in this case we see > + * only 1 paired device with a device_type of REPORT_TYPE_KEYBOARD. For the > + * touchpad to work we must also forward mouse input reports to the dj_hiddev > + * created for the keyboard (instead of forwarding them to a second paired > + * device with a device_type of REPORT_TYPE_MOUSE as we normally would). > + */ > +static const u16 kbd_builtin_touchpad_ids[] = { > + 0xb309, /* Dinovo Edge */ > +}; > + > static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev, > struct hidpp_event *hidpp_report, > struct dj_workitem *workitem) > { > struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev); > + int i, id; > > workitem->type = WORKITEM_TYPE_PAIRED; > workitem->device_type = hidpp_report->params[HIDPP_PARAM_DEVICE_INFO] & > @@ -883,6 +895,13 @@ static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev, > workitem->reports_supported |= STD_KEYBOARD | MULTIMEDIA | > POWER_KEYS | MEDIA_CENTER | > HIDPP; > + id = (workitem->quad_id_msb << 8) | workitem->quad_id_lsb; > + for (i = 0; i < ARRAY_SIZE(kbd_builtin_touchpad_ids); i++) { > + if (id == kbd_builtin_touchpad_ids[i]) { > + workitem->reports_supported |= STD_MOUSE; > + break; > + } > + } > break; > case REPORT_TYPE_MOUSE: > workitem->reports_supported |= STD_MOUSE | HIDPP; >
Hi All, On 11/2/20 2:36 PM, Hans de Goede wrote: > Some quad/bluetooth keyboards, such as the Dinovo Edge (Y-RAY81) have a > builtin touchpad. In this case when asking the receiver for paired devices, > we get only 1 paired device with a device_type of REPORT_TYPE_KEYBOARD. > > This means that we do not instantiate a second dj_hiddev for the mouse > (as we normally would) and thus there is no place for us to forward the > mouse input reports to, causing the touchpad part of the keyboard to not > work. > > There is no way for us to detect these keyboards, so this commit adds > an array with device-ids for such keyboards and when a keyboard is on > this list it adds STD_MOUSE to the reports_supported bitmap for the > dj_hiddev created for the keyboard fixing the touchpad not working. > > Using a list of device-ids for this is not ideal, but there are only > very few such keyboards so this should be fine. Besides the Dinovo Edge, > other known wireless Logitech keyboards with a builtin touchpad are: > > * Dinovo Mini (TODO add its device-id to the list) > * K400 (uses a unifying receiver so is not affected) > * K600 (uses a unifying receiver so is not affected) > > Cc: stable@vger.kernel.org > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1811424 > Signed-off-by: Hans de Goede <hdegoede@redhat.com> ping? This is a bug fix for a regression caused by: Commit f2113c3020ef ("HID: logitech-dj: add support for Logitech Bluetooth Mini-Receiver") Specifically that commit caused the builtin touchpad to stop working on Logitech Dinovo Edge keyboards and this fixes this. I realize now that I forgot to add a: Fixes: f2113c3020ef ("HID: logitech-dj: add support for Logitech Bluetooth Mini-Receiver") Tag, let me know if you want a v2 for that. Regardless since this is a bug fix, it would be good if we can get this merged into one of the upcoming 5.10-rc#s. Even without the Dinovo Mini id added this is still worthwhile to get the reported regression fixed and we can add the Dinovo Mini id later. Regards, Hans > --- > drivers/hid/hid-logitech-dj.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c > index ea1e40530f85..9ed7260b9593 100644 > --- a/drivers/hid/hid-logitech-dj.c > +++ b/drivers/hid/hid-logitech-dj.c > @@ -867,11 +867,23 @@ static void logi_dj_recv_queue_notification(struct dj_receiver_dev *djrcv_dev, > schedule_work(&djrcv_dev->work); > } > > +/* > + * Some quad/bluetooth keyboards have a builtin touchpad in this case we see > + * only 1 paired device with a device_type of REPORT_TYPE_KEYBOARD. For the > + * touchpad to work we must also forward mouse input reports to the dj_hiddev > + * created for the keyboard (instead of forwarding them to a second paired > + * device with a device_type of REPORT_TYPE_MOUSE as we normally would). > + */ > +static const u16 kbd_builtin_touchpad_ids[] = { > + 0xb309, /* Dinovo Edge */ > +}; > + > static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev, > struct hidpp_event *hidpp_report, > struct dj_workitem *workitem) > { > struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev); > + int i, id; > > workitem->type = WORKITEM_TYPE_PAIRED; > workitem->device_type = hidpp_report->params[HIDPP_PARAM_DEVICE_INFO] & > @@ -883,6 +895,13 @@ static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev, > workitem->reports_supported |= STD_KEYBOARD | MULTIMEDIA | > POWER_KEYS | MEDIA_CENTER | > HIDPP; > + id = (workitem->quad_id_msb << 8) | workitem->quad_id_lsb; > + for (i = 0; i < ARRAY_SIZE(kbd_builtin_touchpad_ids); i++) { > + if (id == kbd_builtin_touchpad_ids[i]) { > + workitem->reports_supported |= STD_MOUSE; > + break; > + } > + } > break; > case REPORT_TYPE_MOUSE: > workitem->reports_supported |= STD_MOUSE | HIDPP; >
Hi, On 11/14/20 12:03 PM, Hans de Goede wrote: > Hi, > > On 11/12/20 4:49 PM, Benjamin Tissoires wrote: >> On Wed, Nov 11, 2020 at 12:07 PM Hans de Goede <hdegoede@redhat.com> wrote: > > <snip> > >>>> If so, I can try to push it later today or tomorrow. >>> >>> Sounds good, thank you. >> >> I have now applied the 3 patches to the for-5.10/upstream-fixes branch. >> I also added the Fixes tag to the first commit only. > > Great thank you. > > <snip> > >>>> Yeah, the Dinovo Mini will come later. > > So I've been working on this today and I hope to have a patch for you > ready to test soon. So no need for you to spend time on this now, > that would just be us doing double work. > > <snip> > >>> Do the original HID descriptors of the receiver perhaps have both a report 2 and >>> a report 5 and we should add both ? >> >> No, I think it only has report ID 5. > > So I checked and there are actually both INPUT(2) and INPUT(5) descriptors > on the mouse USB-device's HID intf. > > And looking at the USB ids again, this is not new vs old. This is Dinovo > vs non Dinovo. It seems that even though the air protocol is the same, > the Dinovo receivers have some special handling for the kbd touchpad where > as the non Dinovo receivers just send the same HID reports as for the normal > mouse. > > I paired both the Dinovo Edge + a BT mouse (you can pair any BT mouse, > just press the pair button on the receiver and then on the mouse) with: > > 1. The MX5000 receiver as the RH bugzilla reporter is doing > > In this case both the kbd-touchpad and the actual mouse send INPUT(2) > reports (there is no INPUT(5) in the descriptors). > > 2. The Dinovo Edge receiver. In this case the kbd-touchpad sends > INPUT(5) reports where as the actual mouse sends INPUT(2) reports. p.s. There being 2 mouse reports on the mouse intf. also explains why the hid-logitech-lg.c code needs the special LG_DUPLICATE_USAGES for the Dinovo receivers. We won't have this issue in the logitech-dj code since there (with the patch I'm working on) the INPUT(5) reports will be send to the keyboard hidpp-device where as the INPUT(2) reports will keep being send to the mouse hidpp-device as usual. So there will not be a single hidpp device which receives both mouse-reports and thus no need for something like the LG_DUPLICATE_USAGES quirk. Regards, Hans
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c index ea1e40530f85..9ed7260b9593 100644 --- a/drivers/hid/hid-logitech-dj.c +++ b/drivers/hid/hid-logitech-dj.c @@ -867,11 +867,23 @@ static void logi_dj_recv_queue_notification(struct dj_receiver_dev *djrcv_dev, schedule_work(&djrcv_dev->work); } +/* + * Some quad/bluetooth keyboards have a builtin touchpad in this case we see + * only 1 paired device with a device_type of REPORT_TYPE_KEYBOARD. For the + * touchpad to work we must also forward mouse input reports to the dj_hiddev + * created for the keyboard (instead of forwarding them to a second paired + * device with a device_type of REPORT_TYPE_MOUSE as we normally would). + */ +static const u16 kbd_builtin_touchpad_ids[] = { + 0xb309, /* Dinovo Edge */ +}; + static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev, struct hidpp_event *hidpp_report, struct dj_workitem *workitem) { struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev); + int i, id; workitem->type = WORKITEM_TYPE_PAIRED; workitem->device_type = hidpp_report->params[HIDPP_PARAM_DEVICE_INFO] & @@ -883,6 +895,13 @@ static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev, workitem->reports_supported |= STD_KEYBOARD | MULTIMEDIA | POWER_KEYS | MEDIA_CENTER | HIDPP; + id = (workitem->quad_id_msb << 8) | workitem->quad_id_lsb; + for (i = 0; i < ARRAY_SIZE(kbd_builtin_touchpad_ids); i++) { + if (id == kbd_builtin_touchpad_ids[i]) { + workitem->reports_supported |= STD_MOUSE; + break; + } + } break; case REPORT_TYPE_MOUSE: workitem->reports_supported |= STD_MOUSE | HIDPP;
Some quad/bluetooth keyboards, such as the Dinovo Edge (Y-RAY81) have a builtin touchpad. In this case when asking the receiver for paired devices, we get only 1 paired device with a device_type of REPORT_TYPE_KEYBOARD. This means that we do not instantiate a second dj_hiddev for the mouse (as we normally would) and thus there is no place for us to forward the mouse input reports to, causing the touchpad part of the keyboard to not work. There is no way for us to detect these keyboards, so this commit adds an array with device-ids for such keyboards and when a keyboard is on this list it adds STD_MOUSE to the reports_supported bitmap for the dj_hiddev created for the keyboard fixing the touchpad not working. Using a list of device-ids for this is not ideal, but there are only very few such keyboards so this should be fine. Besides the Dinovo Edge, other known wireless Logitech keyboards with a builtin touchpad are: * Dinovo Mini (TODO add its device-id to the list) * K400 (uses a unifying receiver so is not affected) * K600 (uses a unifying receiver so is not affected) Cc: stable@vger.kernel.org BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1811424 Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/hid/hid-logitech-dj.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)