| Message ID | 20200112205021.3004703-1-lains@archlinux.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: logitech-hidpp: add support for the Powerplay mat/receiver | expand |
On Sun, 2020-01-12 at 20:50 +0000, Filipe Laíns 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_KEYBOARD it was not being marked as HID++ compatible in ^^^^^^^^^^^^^^^^^^^^ REPORT_TYPE_MOUSE Err, should I send another patch? > 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; > 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},
Hi Filipe, On Mon, Jan 13, 2020 at 6:54 AM Filipe Laíns <lains@archlinux.org> wrote: > > On Sun, 2020-01-12 at 20:50 +0000, Filipe Laíns 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_KEYBOARD it was not being marked as HID++ compatible in > ^^^^^^^^^^^^^^^^^^^^ > REPORT_TYPE_MOUSE > > Err, should I send another patch? Yes please. There are a few reasons for that: - it takes more time to actually edit your commit message when applying it, and this way I am actually testing your commit, not the edited one - it also means I have to remember that I need to fix up the commit message (while if you send an updated version, you ensure that I will not forget about it) - it's a one patch series, so we can just have a look at the v2 (it would have been a 20 patch series, things would have been different because sending a v2 for one typo means you are sending 20 emails in other maintainers mailbox) - it's more efficient (you don't lose one hour waiting for me to answer) to just send the v2 and a message telling us to discard the v1 :) Cheers, Benjamin - > > > 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; > > 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}, > > -- > Filipe Laíns
On Sun, 2020-01-12 at 20:50 +0000, Filipe Laíns wrote: > I also marked all lightspeed devices as HID++ compatible. As the > internal powerplay device does not have REPORT_TYPE_KEYBOARD or > REPORT_TYPE_KEYBOARD it was not being marked as HID++ compatible in > logi_hidpp_dev_conn_notif_equad. Actually I had another look at the code and I don't understand why we are manually setting |= HIDPP in logi_hidpp_dev_conn_notif_equad/logi_hidpp_dev_conn_notif_27mhz. We should set it in logi_dj_hidpp_event as it is triggered by receiving a HID++ packet. What do you think Benjamin? -- Filipe Laíns
On Tue, 2020-01-14 at 20:48 +1000, Benjamin Tissoires wrote: > On Tue, Jan 14, 2020 at 1:31 AM Filipe Laíns <lains@archlinux.org> > wrote: > > On Sun, 2020-01-12 at 20:50 +0000, Filipe Laíns wrote: > > > I also marked all lightspeed devices as HID++ compatible. As the > > > internal powerplay device does not have REPORT_TYPE_KEYBOARD or > > > REPORT_TYPE_KEYBOARD it was not being marked as HID++ compatible > > > in > > > logi_hidpp_dev_conn_notif_equad. > > > > Actually I had another look at the code and I don't understand why > > we > > are manually setting |= HIDPP in > > logi_hidpp_dev_conn_notif_equad/logi_hidpp_dev_conn_notif_27mhz. We > > should set it in logi_dj_hidpp_event as it is triggered by > > receiving a > > HID++ packet. > > long story short: nope :) > > The whole purpose of setting the workitem->reports_supported is to be > able to create the matching report descriptor in the new virtual > device. So having this set in a callback will add an operation for > nothing every time we get an event, and will also not ensure a proper > separation of concerns. > > Cheers, > Benjamin > > > What do you think Benjamin? > > > > -- > > Filipe Laíns 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.
On Tue, Jan 14, 2020 at 1:31 AM Filipe Laíns <lains@archlinux.org> wrote: > > On Sun, 2020-01-12 at 20:50 +0000, Filipe Laíns wrote: > > I also marked all lightspeed devices as HID++ compatible. As the > > internal powerplay device does not have REPORT_TYPE_KEYBOARD or > > REPORT_TYPE_KEYBOARD it was not being marked as HID++ compatible in > > logi_hidpp_dev_conn_notif_equad. > > Actually I had another look at the code and I don't understand why we > are manually setting |= HIDPP in > logi_hidpp_dev_conn_notif_equad/logi_hidpp_dev_conn_notif_27mhz. We > should set it in logi_dj_hidpp_event as it is triggered by receiving a > HID++ packet. long story short: nope :) The whole purpose of setting the workitem->reports_supported is to be able to create the matching report descriptor in the new virtual device. So having this set in a callback will add an operation for nothing every time we get an event, and will also not ensure a proper separation of concerns. Cheers, Benjamin > > What do you think Benjamin? > > -- > Filipe Laíns
On Tue, Jan 14, 2020 at 10:55 AM Filipe Laíns <lains@archlinux.org> wrote: > > On Tue, 2020-01-14 at 20:48 +1000, Benjamin Tissoires wrote: > > On Tue, Jan 14, 2020 at 1:31 AM Filipe Laíns <lains@archlinux.org> > > wrote: > > > On Sun, 2020-01-12 at 20:50 +0000, Filipe Laíns wrote: > > > > I also marked all lightspeed devices as HID++ compatible. As the > > > > internal powerplay device does not have REPORT_TYPE_KEYBOARD or > > > > REPORT_TYPE_KEYBOARD it was not being marked as HID++ compatible > > > > in > > > > logi_hidpp_dev_conn_notif_equad. > > > > > > Actually I had another look at the code and I don't understand why > > > we > > > are manually setting |= HIDPP in > > > logi_hidpp_dev_conn_notif_equad/logi_hidpp_dev_conn_notif_27mhz. We > > > should set it in logi_dj_hidpp_event as it is triggered by > > > receiving a > > > HID++ packet. > > > > long story short: nope :) > > > > The whole purpose of setting the workitem->reports_supported is to be > > able to create the matching report descriptor in the new virtual > > device. So having this set in a callback will add an operation for > > nothing every time we get an event, and will also not ensure a proper > > separation of concerns. > > > > Cheers, > > Benjamin > > > > > What do you think Benjamin? > > > > > > -- > > > Filipe Laíns > > 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. Yep, looks better. > > I will send a new patch if you agree with this approach. thanks. Cheers, Benjamin > > -- > Filipe Laíns
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_KEYBOARD 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(-)