| Message ID | 20200112220612.3448741-1-lains@archlinux.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Benjamin Tissoires |
| Headers | show |
| Series | [v2] HID: logitech-hidpp: add support for the Powerplay mat/receiver | expand |
On Tue, 2020-01-14 at 21:03 +1000, Benjamin Tissoires wrote: > Hi Filipe, > > On Mon, Jan 13, 2020 at 8:07 AM Filipe Laíns <lains@archlinux.org> wrote: > > The Logitech G Powerplay has a lightspeed receiver with a static HID++ > > device with ID 7 attached to it to. It is used to configure the led on > > the mat. For this reason I increased the max number of devices. > > > > I also marked all lightspeed devices as HID++ compatible. As the > > internal powerplay device does not have REPORT_TYPE_KEYBOARD or > > REPORT_TYPE_MOUSE it was not being marked as HID++ compatible in > > logi_hidpp_dev_conn_notif_equad. > > > > Signed-off-by: Filipe Laíns <lains@archlinux.org> > > --- > > drivers/hid/hid-logitech-dj.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c > > index bb50d6e7745b..732380b55b15 100644 > > --- a/drivers/hid/hid-logitech-dj.c > > +++ b/drivers/hid/hid-logitech-dj.c > > @@ -16,11 +16,11 @@ > > #include <asm/unaligned.h> > > #include "hid-ids.h" > > > > -#define DJ_MAX_PAIRED_DEVICES 6 > > +#define DJ_MAX_PAIRED_DEVICES 7 > > #define DJ_MAX_NUMBER_NOTIFS 8 > > #define DJ_RECEIVER_INDEX 0 > > #define DJ_DEVICE_INDEX_MIN 1 > > -#define DJ_DEVICE_INDEX_MAX 6 > > +#define DJ_DEVICE_INDEX_MAX 7 > > > > #define DJREPORT_SHORT_LENGTH 15 > > #define DJREPORT_LONG_LENGTH 32 > > @@ -971,7 +971,7 @@ static void logi_hidpp_recv_queue_notif(struct hid_device *hdev, > > case 0x0c: > > device_type = "eQUAD Lightspeed 1"; > > logi_hidpp_dev_conn_notif_equad(hdev, hidpp_report, &workitem); > > - workitem.reports_supported |= STD_KEYBOARD; > > + workitem.reports_supported |= STD_KEYBOARD | HIDPP; > > This feels wrong. The whole initial code was hack-ish, and now this is > even more hack-ish. > > I don't have a simple solution right now (at a conference this week), > but I will need to be convinced this is the only sane solution :/ > > Cheers, > Benjamin Yeah, I understand. I just addressed this in a reply to the previous patch: On Tue, 2020-01-14 at 00:55 +0000, Filipe Laíns wrote: > Okay, then is maybe better if I add HIDPP to reports_supported based > on > the device ID (7). This is the only product to my knowledge that > exports a device with ID 7. It's a better solution than setting HIDPP > on all lightspeed devices. > > I will send a new patch if you agree with this approach. What do you think? Maybe we will have to rework the driver but the method I proposed above doesn't seem that hack-ish, at least considering the current code :P > > break; > > case 0x0d: > > device_type = "eQUAD Lightspeed 1_1"; > > @@ -1850,6 +1850,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 powerplay mat/receiver (0xc539) */ > > + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, > > + 0xc53a), > > + .driver_data = recvr_type_gaming_hidpp}, > > { /* 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.24.1 > >
Hi Filipe, On Mon, Jan 13, 2020 at 8:07 AM Filipe Laíns <lains@archlinux.org> wrote: > > The Logitech G Powerplay has a lightspeed receiver with a static HID++ > device with ID 7 attached to it to. It is used to configure the led on > the mat. For this reason I increased the max number of devices. > > I also marked all lightspeed devices as HID++ compatible. As the > internal powerplay device does not have REPORT_TYPE_KEYBOARD or > REPORT_TYPE_MOUSE it was not being marked as HID++ compatible in > logi_hidpp_dev_conn_notif_equad. > > Signed-off-by: Filipe Laíns <lains@archlinux.org> > --- > drivers/hid/hid-logitech-dj.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c > index bb50d6e7745b..732380b55b15 100644 > --- a/drivers/hid/hid-logitech-dj.c > +++ b/drivers/hid/hid-logitech-dj.c > @@ -16,11 +16,11 @@ > #include <asm/unaligned.h> > #include "hid-ids.h" > > -#define DJ_MAX_PAIRED_DEVICES 6 > +#define DJ_MAX_PAIRED_DEVICES 7 > #define DJ_MAX_NUMBER_NOTIFS 8 > #define DJ_RECEIVER_INDEX 0 > #define DJ_DEVICE_INDEX_MIN 1 > -#define DJ_DEVICE_INDEX_MAX 6 > +#define DJ_DEVICE_INDEX_MAX 7 > > #define DJREPORT_SHORT_LENGTH 15 > #define DJREPORT_LONG_LENGTH 32 > @@ -971,7 +971,7 @@ static void logi_hidpp_recv_queue_notif(struct hid_device *hdev, > case 0x0c: > device_type = "eQUAD Lightspeed 1"; > logi_hidpp_dev_conn_notif_equad(hdev, hidpp_report, &workitem); > - workitem.reports_supported |= STD_KEYBOARD; > + workitem.reports_supported |= STD_KEYBOARD | HIDPP; This feels wrong. The whole initial code was hack-ish, and now this is even more hack-ish. I don't have a simple solution right now (at a conference this week), but I will need to be convinced this is the only sane solution :/ Cheers, Benjamin > break; > case 0x0d: > device_type = "eQUAD Lightspeed 1_1"; > @@ -1850,6 +1850,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 powerplay mat/receiver (0xc539) */ > + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, > + 0xc53a), > + .driver_data = recvr_type_gaming_hidpp}, > { /* 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.24.1 >
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c index bb50d6e7745b..732380b55b15 100644 --- a/drivers/hid/hid-logitech-dj.c +++ b/drivers/hid/hid-logitech-dj.c @@ -16,11 +16,11 @@ #include <asm/unaligned.h> #include "hid-ids.h" -#define DJ_MAX_PAIRED_DEVICES 6 +#define DJ_MAX_PAIRED_DEVICES 7 #define DJ_MAX_NUMBER_NOTIFS 8 #define DJ_RECEIVER_INDEX 0 #define DJ_DEVICE_INDEX_MIN 1 -#define DJ_DEVICE_INDEX_MAX 6 +#define DJ_DEVICE_INDEX_MAX 7 #define DJREPORT_SHORT_LENGTH 15 #define DJREPORT_LONG_LENGTH 32 @@ -971,7 +971,7 @@ static void logi_hidpp_recv_queue_notif(struct hid_device *hdev, case 0x0c: device_type = "eQUAD Lightspeed 1"; logi_hidpp_dev_conn_notif_equad(hdev, hidpp_report, &workitem); - workitem.reports_supported |= STD_KEYBOARD; + workitem.reports_supported |= STD_KEYBOARD | HIDPP; break; case 0x0d: device_type = "eQUAD Lightspeed 1_1"; @@ -1850,6 +1850,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 powerplay mat/receiver (0xc539) */ + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, + 0xc53a), + .driver_data = recvr_type_gaming_hidpp}, { /* 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},
The Logitech G Powerplay has a lightspeed receiver with a static HID++ device with ID 7 attached to it to. It is used to configure the led on the mat. For this reason I increased the max number of devices. I also marked all lightspeed devices as HID++ compatible. As the internal powerplay device does not have REPORT_TYPE_KEYBOARD or REPORT_TYPE_MOUSE it was not being marked as HID++ compatible in logi_hidpp_dev_conn_notif_equad. Signed-off-by: Filipe Laíns <lains@archlinux.org> --- drivers/hid/hid-logitech-dj.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)