Hi, On 11/10/20 7:29 PM, Benjamin Tissoires wrote: > Hi Hans, > > On Tue, Nov 10, 2020 at 2:17 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: >> >> 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@xxxxxxxxxxxxxxx >> > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1811424 >> > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> >> >> ping? This is a bug fix for a regression caused by: > > Series looks good. I tried to enable the Dinovo Mini this afternoon (see > patch below), but it's not that clean and easy... > >> >> 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. > > I guess you want the tag on all 3 patches, not just the first. Patch 2 and 3 do not fix a regression: Patch 2 enables extra functionality (non working A-D and phone keys) Patch 3 fixes there being 2 batteries under /sys/class/power when the kbd is in bluetooth mode, this problem is introduced by patch 2. I think that taking patch 2/3 for 5.10-rc# is fine, but they don't really fix anything related to commit f2113c3020ef. If anything patch 3 should have a fixes tag for the final commit hash of patch 2 (or maybe just squash them?) Note not taking patch 2/3 for 5.10-rc# is fine too. > If so, I can try to push it later today or tomorrow. Sounds good, thank you. >> 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. > > Yeah, the Dinovo Mini will come later. > > My current WIP is the following: Oh interesting, and good timing, let me explain: I bought a 2nd hand Dinovo Edge to debug the reported regression (*), but that came without a receiver. So I paired it with the MX5000 receiver which I already had (and which has the same USB-ids as the reporters receiver) and that works fine with this patch. But I did want to have a complete set, so I found some US store on amazon selling spare Dinovo Edge dongles and shipping them to Europe in a letter (so no crazy shipping costs). That dongle arrived yesterday and I did a quick test run. It has usb-ids of c713 for the keyboard usb-device and c714 for the mouse usb-device (the dongle has a builtin hub and presents 2 separate usb devices, like the MX5000 / MX5500 dongles). So I added these ids to hid-logitech-dj.c (and dropped one of them from hid-lg.c) after that the mousepad on the Dinovo Edge however stopped working again when paired with this dongle. So I already guessed that the mouse descriptor would be different, but I did not get around to actually checking this. Note this has an important implication for your patch, you assume the mouse-report changes based on the paired device. But that is not the case it is simple that some (newer? at least a somewhat higher usb-dev-id) quad/bt2.0 combo dongles have a different mouse report. At least with the Dinovo Edge the mouse report changes when it is paired with a different dongle, so it is the dongle which determines the mouse report, not the paired device. So we need a "recvr_type_bluetooth_v2" and then this new report can just be added to the big: if (djdev->dj_receiver_dev->type == recvr_type_gaming_hidpp || djdev->dj_receiver_dev->type == recvr_type_mouse_only) rdcat(rdesc, &rsize, mse_high_res_descriptor, sizeof(mse_high_res_descriptor)); else if (djdev->dj_receiver_dev->type == recvr_type_27mhz) rdcat(rdesc, &rsize, mse_27mhz_descriptor, sizeof(mse_27mhz_descriptor)); else if (djdev->dj_receiver_dev->type == recvr_type_bluetooth) rdcat(rdesc, &rsize, mse_bluetooth_descriptor, sizeof(mse_bluetooth_descriptor)); else rdcat(rdesc, &rsize, mse_descriptor, sizeof(mse_descriptor)); block. Hmm, I also see that the new descriptor has a report-id of 5, so it looks like we do need the BT_MOUSE thing, but then set it based on the receiver usb-id instead? Do the original HID descriptors of the receiver perhaps have both a report 2 and a report 5 and we should add both ? Note I also see that you add a separate id-array for the dinovo-mini because of this given my experience that the behavior changes based on the used receiver, I don't think that is necessary. Instead we need to add or not add the BT_MOUSE bit to the keyboards reports_supported based on the receiver-type (I think). Anyways this definitely needs some more work, so as you said lets move forward with the fix for the MX5000 receiver usb-ids as is. Although adding the dinovo-mini device-id to the kbd_builtin_touchpad_ids[] in case it gets paired with sat the MX5000 receiver probably cannot hurt. Regards, Hans *) and I'm glad I did I don't think I would have enjoyed debugging this remotely > --- > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c > index 1cafb65428b0..1c7857bf3290 100644 > --- a/drivers/hid/hid-logitech-dj.c > +++ b/drivers/hid/hid-logitech-dj.c > @@ -84,6 +84,7 @@ > #define STD_MOUSE BIT(2) > #define MULTIMEDIA BIT(3) > #define POWER_KEYS BIT(4) > +#define BT_MOUSE BIT(5) > #define MEDIA_CENTER BIT(8) > #define KBD_LEDS BIT(14) > /* Fake (bitnr > NUMBER_OF_HID_REPORTS) bit to track HID++ capability */ > @@ -333,6 +334,47 @@ static const char mse_bluetooth_descriptor[] = { > 0xC0, /* END_COLLECTION */ > }; > > +/* Mouse descriptor (5) for Bluetooth receiver, low-res hwheel, 8 buttons */ > +static const char mse5_bluetooth_descriptor[] = { > + 0x05, 0x01, /* USAGE_PAGE (Generic Desktop) */ > + 0x09, 0x02, /* Usage (Mouse) */ > + 0xa1, 0x01, /* Collection (Application) */ > + 0x85, 0x05, /* Report ID (5) */ > + 0x09, 0x01, /* Usage (Pointer) */ > + 0xa1, 0x00, /* Collection (Physical) */ > + 0x05, 0x09, /* Usage Page (Button) */ > + 0x19, 0x01, /* Usage Minimum (1) */ > + 0x29, 0x08, /* Usage Maximum (8) */ > + 0x15, 0x00, /* Logical Minimum (0) */ > + 0x25, 0x01, /* Logical Maximum (1) */ > + 0x95, 0x08, /* Report Count (8) */ > + 0x75, 0x01, /* Report Size (1) */ > + 0x81, 0x02, /* Input (Data,Var,Abs) */ > + 0x05, 0x01, /* Usage Page (Generic Desktop) */ > + 0x16, 0x01, 0xf8, /* Logical Minimum (-2047) */ > + 0x26, 0xff, 0x07, /* Logical Maximum (2047) */ > + 0x75, 0x0c, /* Report Size (12) */ > + 0x95, 0x02, /* Report Count (2) */ > + 0x09, 0x30, /* Usage (X) */ > + 0x09, 0x31, /* Usage (Y) */ > + 0x81, 0x06, /* Input (Data,Var,Rel) */ > + 0x15, 0x81, /* Logical Minimum (-127) */ > + 0x25, 0x7f, /* Logical Maximum (127) */ > + 0x75, 0x08, /* Report Size (8) */ > + 0x95, 0x01, /* Report Count (1) */ > + 0x09, 0x38, /* Usage (Wheel) */ > + 0x81, 0x06, /* Input (Data,Var,Rel) */ > + 0x05, 0x0c, /* Usage Page (Consumer Devices) */ > + 0x0a, 0x38, 0x02, /* Usage (AC Pan) */ > + 0x15, 0x81, /* Logical Minimum (-127) */ > + 0x25, 0x7f, /* Logical Maximum (127) */ > + 0x75, 0x08, /* Report Size (8) */ > + 0x95, 0x01, /* Report Count (1) */ > + 0x81, 0x06, /* Input (Data,Var,Rel) */ > + 0xc0, /* End Collection */ > + 0xc0, /* End Collection */ > +}; > + > /* Gaming Mouse descriptor (2) */ > static const char mse_high_res_descriptor[] = { > 0x05, 0x01, /* USAGE_PAGE (Generic Desktop) */ > @@ -877,6 +919,10 @@ static const u16 kbd_builtin_touchpad_ids[] = { > 0xb309, /* Dinovo Edge */ > }; > > +static const u16 kbd_builtin_touchpad5_ids[] = { > + 0xb30c, /* Dinovo Mini */ > +}; > + > static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev, > struct hidpp_event *hidpp_report, > struct dj_workitem *workitem) > @@ -901,6 +947,12 @@ static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev, > break; > } > } > + for (i = 0; i < ARRAY_SIZE(kbd_builtin_touchpad5_ids); i++) { > + if (id == kbd_builtin_touchpad5_ids[i]) { > + workitem->reports_supported |= BT_MOUSE; > + break; > + } > + } > break; > case REPORT_TYPE_MOUSE: > workitem->reports_supported |= STD_MOUSE | HIDPP; > @@ -1368,6 +1420,13 @@ static int logi_dj_ll_parse(struct hid_device *hid) > sizeof(mse_descriptor)); > } > > + if (djdev->reports_supported & BT_MOUSE) { > + dbg_hid("%s: sending a mouse descriptor, reports_supported: %llx\n", > + __func__, djdev->reports_supported); > + rdcat(rdesc, &rsize, mse5_bluetooth_descriptor, > + sizeof(mse5_bluetooth_descriptor)); > + } > + > if (djdev->reports_supported & MULTIMEDIA) { > dbg_hid("%s: sending a multimedia report descriptor: %llx\n", > __func__, djdev->reports_supported); > @@ -1907,6 +1966,14 @@ static const struct hid_device_id logi_dj_receivers[] = { > HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, > 0xc71c), > .driver_data = recvr_type_bluetooth}, > + { /* Logitech DiNovo Mini HID++ / bluetooth receiver mouse intf. */ > + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, > + 0xc71e), > + .driver_data = recvr_type_bluetooth}, > + { /* Logitech DiNovo Mini HID++ / bluetooth receiver keyboard intf. */ > + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, > + 0xc71f), > + .driver_data = recvr_type_bluetooth}, > {} > }; > > --- > > And the keyboard is not sending the proper KEY_MEDIA like with the > hid-logitech.ko driver. So this WIP can not go into a stable tree. > > Cheers, > Benjamin >
