| Message ID | 20210123180334.3062995-1-lains@archlinux.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [1/2] HID: logitech-dj: add support for the new lightspeed receiver iteration | expand |
On Sat, 23 Jan 2021, Filipe Laíns wrote: > From: Filipe Laíns <lains@riseup.net> > > Tested with the G Pro X Superlight. libratbag sees the device, as > expected, and input events are passing trough. > > https://github.com/libratbag/libratbag/pull/1122 > > The receiver has a quirk where the moused interface doesn't have a > report ID, I am not sure why, perhaps they forgot. All other interfaces > have report IDs so I am left scratching my head. > Since this driver doesn't have a quirk system, I simply implemented it > as a different receiver type, which is true, it just wouldn't be the > prefered approach :P Benjamin, do you have any other idea how to accomplish this without this kind of spaghetti-style conditions? If not, I am tempted to apply the patch as is. Thanks,
On Fri, 2021-02-05 at 16:14 +0100, Jiri Kosina wrote: > On Sat, 23 Jan 2021, Filipe Laíns wrote: > > > From: Filipe Laíns <lains@riseup.net> > > > > Tested with the G Pro X Superlight. libratbag sees the device, as > > expected, and input events are passing trough. > > > > https://github.com/libratbag/libratbag/pull/1122 > > > > The receiver has a quirk where the moused interface doesn't have a > > report ID, I am not sure why, perhaps they forgot. All other interfaces > > have report IDs so I am left scratching my head. > > Since this driver doesn't have a quirk system, I simply implemented it > > as a different receiver type, which is true, it just wouldn't be the > > prefered approach :P > > Benjamin, do you have any other idea how to accomplish this without this > kind of spaghetti-style conditions? > If not, I am tempted to apply the patch as is. > > Thanks, > Hi Benjamin, Jiri, Could we get an update on this? Cheers, Filipe Laíns
On Sat, Jan 23, 2021 at 7:03 PM Filipe Laíns <lains@archlinux.org> wrote: > > From: Filipe Laíns <lains@riseup.net> > > Tested with the G Pro X Superlight. libratbag sees the device, as > expected, and input events are passing trough. > > https://github.com/libratbag/libratbag/pull/1122 > > The receiver has a quirk where the moused interface doesn't have a > report ID, I am not sure why, perhaps they forgot. All other interfaces > have report IDs so I am left scratching my head. > Since this driver doesn't have a quirk system, I simply implemented it > as a different receiver type, which is true, it just wouldn't be the > prefered approach :P > > Signed-off-by: Filipe Laíns <lains@riseup.net> > --- > drivers/hid/hid-ids.h | 1 + > drivers/hid/hid-logitech-dj.c | 49 +++++++++++++++++++++++++---------- > 2 files changed, 37 insertions(+), 13 deletions(-) > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > index 4c5f23640f9c..8eac3c93fa38 100644 > --- a/drivers/hid/hid-ids.h > +++ b/drivers/hid/hid-ids.h > @@ -803,6 +803,7 @@ > #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1 0xc539 > #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_1 0xc53f > #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_POWERPLAY 0xc53a > +#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_2 0xc547 > #define USB_DEVICE_ID_SPACETRAVELLER 0xc623 > #define USB_DEVICE_ID_SPACENAVIGATOR 0xc626 > #define USB_DEVICE_ID_DINOVO_DESKTOP 0xc704 > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c > index 1401ee2067ca..6596c81947a8 100644 > --- a/drivers/hid/hid-logitech-dj.c > +++ b/drivers/hid/hid-logitech-dj.c > @@ -114,6 +114,7 @@ enum recvr_type { > recvr_type_dj, > recvr_type_hidpp, > recvr_type_gaming_hidpp, > + recvr_type_gaming_hidpp_missing_mse_report_id, /* lightspeed receiver missing the mouse report ID */ > recvr_type_mouse_only, > recvr_type_27mhz, > recvr_type_bluetooth, > @@ -1360,6 +1361,7 @@ static int logi_dj_ll_parse(struct hid_device *hid) > dbg_hid("%s: sending a mouse descriptor, reports_supported: %llx\n", > __func__, djdev->reports_supported); > if (djdev->dj_receiver_dev->type == recvr_type_gaming_hidpp || > + djdev->dj_receiver_dev->type == recvr_type_gaming_hidpp_missing_mse_report_id || > djdev->dj_receiver_dev->type == recvr_type_mouse_only) > rdcat(rdesc, &rsize, mse_high_res_descriptor, > sizeof(mse_high_res_descriptor)); > @@ -1605,19 +1607,35 @@ static int logi_dj_raw_event(struct hid_device *hdev, > data[0] = data[1]; > data[1] = 0; > } > - /* > - * Mouse-only receivers send unnumbered mouse data. The 27 MHz > - * receiver uses 6 byte packets, the nano receiver 8 bytes. > - */ > - if (djrcv_dev->unnumbered_application == HID_GD_MOUSE && > - size <= 8) { > - u8 mouse_report[9]; > - > - /* Prepend report id */ > - mouse_report[0] = REPORT_TYPE_MOUSE; > - memcpy(mouse_report + 1, data, size); > - logi_dj_recv_forward_input_report(hdev, mouse_report, > - size + 1); > + > + > + if (djrcv_dev->unnumbered_application == HID_GD_MOUSE) { > + /* > + * Mouse-only receivers send unnumbered mouse data. The 27 MHz > + * receiver uses 6 byte packets, the nano receiver 8 bytes. > + */ > + if (size <= 8) { > + u8 mouse_report[9]; Hmm, as stated above, the 27 MHz receiver already does the exact same thing. Can't we just bump the array size and check above to the following: if (size <= 16) { u8 mouse_report[17]; The property "djrcv_dev->unnumbered_application" is based on the report descriptor entirely, and they are just following the HID norm here. So I think this should be enough. Cheers, Benjamin > + > + /* Prepend report id */ > + mouse_report[0] = REPORT_TYPE_MOUSE; > + memcpy(mouse_report + 1, data, size); > + logi_dj_recv_forward_input_report(hdev, mouse_report, > + size + 1); > + > + /* > + * A variant of the ligtpseed receivers is missing the mouse > + * report ID. > + */ > + } else if (djrcv_dev->type == recvr_type_gaming_hidpp_missing_mse_report_id) { > + u8 mouse_report[17]; > + > + /* Prepend report id */ > + mouse_report[0] = REPORT_TYPE_MOUSE; > + memcpy(mouse_report + 1, data, size); > + logi_dj_recv_forward_input_report(hdev, mouse_report, > + size + 1); > + } > } > > return false; > @@ -1688,6 +1706,7 @@ static int logi_dj_probe(struct hid_device *hdev, > case recvr_type_dj: no_dj_interfaces = 3; break; > case recvr_type_hidpp: no_dj_interfaces = 2; break; > case recvr_type_gaming_hidpp: no_dj_interfaces = 3; break; > + case recvr_type_gaming_hidpp_missing_mse_report_id: no_dj_interfaces = 3; break; > case recvr_type_mouse_only: no_dj_interfaces = 2; break; > case recvr_type_27mhz: no_dj_interfaces = 2; break; > case recvr_type_bluetooth: no_dj_interfaces = 2; break; > @@ -1886,6 +1905,10 @@ static const struct hid_device_id logi_dj_receivers[] = { > HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, > USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_1), > .driver_data = recvr_type_gaming_hidpp}, > + { /* Logitech lightspeed receiver (0xc547) */ > + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, > + USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_2), > + .driver_data = recvr_type_gaming_hidpp_missing_mse_report_id}, > { /* Logitech 27 MHz HID++ 1.0 receiver (0xc513) */ > HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER), > .driver_data = recvr_type_27mhz}, > -- > 2.30.0 >
Hi Filipe, On Mon, Mar 1, 2021 at 3:46 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Sat, Jan 23, 2021 at 7:03 PM Filipe Laíns <lains@archlinux.org> wrote: > > > > From: Filipe Laíns <lains@riseup.net> > > > > Tested with the G Pro X Superlight. libratbag sees the device, as > > expected, and input events are passing trough. > > > > https://github.com/libratbag/libratbag/pull/1122 > > > > The receiver has a quirk where the moused interface doesn't have a > > report ID, I am not sure why, perhaps they forgot. All other interfaces > > have report IDs so I am left scratching my head. > > Since this driver doesn't have a quirk system, I simply implemented it > > as a different receiver type, which is true, it just wouldn't be the > > prefered approach :P > > > > Signed-off-by: Filipe Laíns <lains@riseup.net> > > --- > > drivers/hid/hid-ids.h | 1 + > > drivers/hid/hid-logitech-dj.c | 49 +++++++++++++++++++++++++---------- > > 2 files changed, 37 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > > index 4c5f23640f9c..8eac3c93fa38 100644 > > --- a/drivers/hid/hid-ids.h > > +++ b/drivers/hid/hid-ids.h > > @@ -803,6 +803,7 @@ > > #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1 0xc539 > > #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_1 0xc53f > > #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_POWERPLAY 0xc53a > > +#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_2 0xc547 > > #define USB_DEVICE_ID_SPACETRAVELLER 0xc623 > > #define USB_DEVICE_ID_SPACENAVIGATOR 0xc626 > > #define USB_DEVICE_ID_DINOVO_DESKTOP 0xc704 > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c > > index 1401ee2067ca..6596c81947a8 100644 > > --- a/drivers/hid/hid-logitech-dj.c > > +++ b/drivers/hid/hid-logitech-dj.c > > @@ -114,6 +114,7 @@ enum recvr_type { > > recvr_type_dj, > > recvr_type_hidpp, > > recvr_type_gaming_hidpp, > > + recvr_type_gaming_hidpp_missing_mse_report_id, /* lightspeed receiver missing the mouse report ID */ > > recvr_type_mouse_only, > > recvr_type_27mhz, > > recvr_type_bluetooth, > > @@ -1360,6 +1361,7 @@ static int logi_dj_ll_parse(struct hid_device *hid) > > dbg_hid("%s: sending a mouse descriptor, reports_supported: %llx\n", > > __func__, djdev->reports_supported); > > if (djdev->dj_receiver_dev->type == recvr_type_gaming_hidpp || > > + djdev->dj_receiver_dev->type == recvr_type_gaming_hidpp_missing_mse_report_id || > > djdev->dj_receiver_dev->type == recvr_type_mouse_only) > > rdcat(rdesc, &rsize, mse_high_res_descriptor, > > sizeof(mse_high_res_descriptor)); > > @@ -1605,19 +1607,35 @@ static int logi_dj_raw_event(struct hid_device *hdev, > > data[0] = data[1]; > > data[1] = 0; > > } > > - /* > > - * Mouse-only receivers send unnumbered mouse data. The 27 MHz > > - * receiver uses 6 byte packets, the nano receiver 8 bytes. > > - */ > > - if (djrcv_dev->unnumbered_application == HID_GD_MOUSE && > > - size <= 8) { > > - u8 mouse_report[9]; > > - > > - /* Prepend report id */ > > - mouse_report[0] = REPORT_TYPE_MOUSE; > > - memcpy(mouse_report + 1, data, size); > > - logi_dj_recv_forward_input_report(hdev, mouse_report, > > - size + 1); > > + > > + > > + if (djrcv_dev->unnumbered_application == HID_GD_MOUSE) { > > + /* > > + * Mouse-only receivers send unnumbered mouse data. The 27 MHz > > + * receiver uses 6 byte packets, the nano receiver 8 bytes. > > + */ > > + if (size <= 8) { > > + u8 mouse_report[9]; > > Hmm, as stated above, the 27 MHz receiver already does the exact same thing. > > Can't we just bump the array size and check above to the following: > > if (size <= 16) { > u8 mouse_report[17]; > > The property "djrcv_dev->unnumbered_application" is based on the > report descriptor entirely, and they are just following the HID norm > here. So I think this should be enough. I've been pinged last week about the G Pro X superlight, and Peter found that this patch never got into upstream. AFAICT, compared to upstream the following code change should have the same effect that what you proposed here: --- diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c index 7106b921b53c..f5cdfce272e7 100644 --- a/drivers/hid/hid-logitech-dj.c +++ b/drivers/hid/hid-logitech-dj.c @@ -1691,11 +1691,12 @@ static int logi_dj_raw_event(struct hid_device *hdev, } /* * Mouse-only receivers send unnumbered mouse data. The 27 MHz - * receiver uses 6 byte packets, the nano receiver 8 bytes. + * receiver uses 6 byte packets, the nano receiver 8 bytes and + * some gaming ones are using 16 bytes. */ if (djrcv_dev->unnumbered_application == HID_GD_MOUSE && - size <= 8) { - u8 mouse_report[9]; + size <= 16) { + u8 mouse_report[17]; /* Prepend report id */ mouse_report[0] = REPORT_TYPE_MOUSE; --- Would you mind sending a v2 of the patch updated to the current for-next so we can schedule this for 5.17? Cheers, Benjamin > > Cheers, > Benjamin > > > + > > + /* Prepend report id */ > > + mouse_report[0] = REPORT_TYPE_MOUSE; > > + memcpy(mouse_report + 1, data, size); > > + logi_dj_recv_forward_input_report(hdev, mouse_report, > > + size + 1); > > + > > + /* > > + * A variant of the ligtpseed receivers is missing the mouse > > + * report ID. > > + */ > > + } else if (djrcv_dev->type == recvr_type_gaming_hidpp_missing_mse_report_id) { > > + u8 mouse_report[17]; > > + > > + /* Prepend report id */ > > + mouse_report[0] = REPORT_TYPE_MOUSE; > > + memcpy(mouse_report + 1, data, size); > > + logi_dj_recv_forward_input_report(hdev, mouse_report, > > + size + 1); > > + } > > } > > > > return false; > > @@ -1688,6 +1706,7 @@ static int logi_dj_probe(struct hid_device *hdev, > > case recvr_type_dj: no_dj_interfaces = 3; break; > > case recvr_type_hidpp: no_dj_interfaces = 2; break; > > case recvr_type_gaming_hidpp: no_dj_interfaces = 3; break; > > + case recvr_type_gaming_hidpp_missing_mse_report_id: no_dj_interfaces = 3; break; > > case recvr_type_mouse_only: no_dj_interfaces = 2; break; > > case recvr_type_27mhz: no_dj_interfaces = 2; break; > > case recvr_type_bluetooth: no_dj_interfaces = 2; break; > > @@ -1886,6 +1905,10 @@ static const struct hid_device_id logi_dj_receivers[] = { > > HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, > > USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_1), > > .driver_data = recvr_type_gaming_hidpp}, > > + { /* Logitech lightspeed receiver (0xc547) */ > > + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, > > + USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_2), > > + .driver_data = recvr_type_gaming_hidpp_missing_mse_report_id}, > > { /* Logitech 27 MHz HID++ 1.0 receiver (0xc513) */ > > HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER), > > .driver_data = recvr_type_27mhz}, > > -- > > 2.30.0 > >
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index 4c5f23640f9c..8eac3c93fa38 100644 --- a/drivers/hid/hid-ids.h +++ b/drivers/hid/hid-ids.h @@ -803,6 +803,7 @@ #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1 0xc539 #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_1 0xc53f #define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_POWERPLAY 0xc53a +#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_2 0xc547 #define USB_DEVICE_ID_SPACETRAVELLER 0xc623 #define USB_DEVICE_ID_SPACENAVIGATOR 0xc626 #define USB_DEVICE_ID_DINOVO_DESKTOP 0xc704 diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c index 1401ee2067ca..6596c81947a8 100644 --- a/drivers/hid/hid-logitech-dj.c +++ b/drivers/hid/hid-logitech-dj.c @@ -114,6 +114,7 @@ enum recvr_type { recvr_type_dj, recvr_type_hidpp, recvr_type_gaming_hidpp, + recvr_type_gaming_hidpp_missing_mse_report_id, /* lightspeed receiver missing the mouse report ID */ recvr_type_mouse_only, recvr_type_27mhz, recvr_type_bluetooth, @@ -1360,6 +1361,7 @@ static int logi_dj_ll_parse(struct hid_device *hid) dbg_hid("%s: sending a mouse descriptor, reports_supported: %llx\n", __func__, djdev->reports_supported); if (djdev->dj_receiver_dev->type == recvr_type_gaming_hidpp || + djdev->dj_receiver_dev->type == recvr_type_gaming_hidpp_missing_mse_report_id || djdev->dj_receiver_dev->type == recvr_type_mouse_only) rdcat(rdesc, &rsize, mse_high_res_descriptor, sizeof(mse_high_res_descriptor)); @@ -1605,19 +1607,35 @@ static int logi_dj_raw_event(struct hid_device *hdev, data[0] = data[1]; data[1] = 0; } - /* - * Mouse-only receivers send unnumbered mouse data. The 27 MHz - * receiver uses 6 byte packets, the nano receiver 8 bytes. - */ - if (djrcv_dev->unnumbered_application == HID_GD_MOUSE && - size <= 8) { - u8 mouse_report[9]; - - /* Prepend report id */ - mouse_report[0] = REPORT_TYPE_MOUSE; - memcpy(mouse_report + 1, data, size); - logi_dj_recv_forward_input_report(hdev, mouse_report, - size + 1); + + + if (djrcv_dev->unnumbered_application == HID_GD_MOUSE) { + /* + * Mouse-only receivers send unnumbered mouse data. The 27 MHz + * receiver uses 6 byte packets, the nano receiver 8 bytes. + */ + if (size <= 8) { + u8 mouse_report[9]; + + /* Prepend report id */ + mouse_report[0] = REPORT_TYPE_MOUSE; + memcpy(mouse_report + 1, data, size); + logi_dj_recv_forward_input_report(hdev, mouse_report, + size + 1); + + /* + * A variant of the ligtpseed receivers is missing the mouse + * report ID. + */ + } else if (djrcv_dev->type == recvr_type_gaming_hidpp_missing_mse_report_id) { + u8 mouse_report[17]; + + /* Prepend report id */ + mouse_report[0] = REPORT_TYPE_MOUSE; + memcpy(mouse_report + 1, data, size); + logi_dj_recv_forward_input_report(hdev, mouse_report, + size + 1); + } } return false; @@ -1688,6 +1706,7 @@ static int logi_dj_probe(struct hid_device *hdev, case recvr_type_dj: no_dj_interfaces = 3; break; case recvr_type_hidpp: no_dj_interfaces = 2; break; case recvr_type_gaming_hidpp: no_dj_interfaces = 3; break; + case recvr_type_gaming_hidpp_missing_mse_report_id: no_dj_interfaces = 3; break; case recvr_type_mouse_only: no_dj_interfaces = 2; break; case recvr_type_27mhz: no_dj_interfaces = 2; break; case recvr_type_bluetooth: no_dj_interfaces = 2; break; @@ -1886,6 +1905,10 @@ static const struct hid_device_id logi_dj_receivers[] = { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_1), .driver_data = recvr_type_gaming_hidpp}, + { /* Logitech lightspeed receiver (0xc547) */ + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, + USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_2), + .driver_data = recvr_type_gaming_hidpp_missing_mse_report_id}, { /* Logitech 27 MHz HID++ 1.0 receiver (0xc513) */ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER), .driver_data = recvr_type_27mhz},