| Message ID | uBbIS3nFJ1jdYNLHcqjW5wxQAwmZv0kmYEoeoPrxNhfzi6cHwmCOY-ewdqe7S1hNEj-p4Hd9D0_Y3PymUTdh_6WFXuMmIYUkV2xaKCPMYz0=@protonmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [v4,1/4] HID: logitech: Add MX Mice over Bluetooth | expand |
Hi Mazin, On Fri, Oct 11, 2019 at 2:57 AM Mazin Rezk <mnrzk@protonmail.com> wrote: > > On Saturday, October 5, 2019 9:04 PM, Mazin Rezk <mnrzk@protonmail.com> wrote: > > > This patch adds support for several MX mice over Bluetooth. The device IDs > > have been copied from the libratbag device database and their features > > have been based on their DJ device counterparts. > > No changes have been made to this patch in v4. However, it should be noted > that the only device that has been thoroughly tested in this patch is the > MX Master (b01e). Further testing for the other devices may be required. Thanks a lot for the series, but please amend your format-patch process: - The commit message should not contain the leading `>` characters, and checkpath.pl then complains about Possible unwrapped commit description (prefer a maximum 75 chars per line) - this description of the changes is very useful, but it should go after the first `---` so that we do not pull it while applying the patch. Also, this patch introduces a breakage in the bisectability of the devices it adds. If we were to bisect a breakage in one of those devices, the device will fail to work, and we could not detect where the error comes from. So please squash this patch with the next one. Last, if we need "Further testing for the other devices may be required", then I'd rather enable those device one by one when ewe get the confirmation they are working. Adding a new device costs, but not as much than breaking an existing one, especially when it gets detected later, when the kernel gets shipped in distributions. Note that I have the MX Master 0xB012, so you can safely keep that one on the list, I'll test it myself. Cheers, Benjamin > > Signed-off-by: Mazin Rezk <mnrzk@protonmail.com> > --- > drivers/hid/hid-logitech-hidpp.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c > index 0179f7ed77e5..85fd0c17cc2f 100644 > --- a/drivers/hid/hid-logitech-hidpp.c > +++ b/drivers/hid/hid-logitech-hidpp.c > @@ -3773,6 +3773,24 @@ static const struct hid_device_id hidpp_devices[] = { > { /* MX5500 keyboard over Bluetooth */ > HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb30b), > .driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS }, > + { /* MX Anywhere 2 mouse over Bluetooth */ > + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb013), > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb018), > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > + { /* MX Anywhere 2S mouse over Bluetooth */ > + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01a), > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > + { /* MX Master mouse over Bluetooth */ > + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb012), > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb017), > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01e), > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > + { /* MX Master 2S mouse over Bluetooth */ > + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb019), > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > {} > }; > > -- > 2.23.0 >
On Fri, 2019-10-11 at 00:57 +0000, Mazin Rezk wrote: > On Saturday, October 5, 2019 9:04 PM, Mazin Rezk < > mnrzk@protonmail.com> wrote: > > > This patch adds support for several MX mice over Bluetooth. The > > device IDs > > have been copied from the libratbag device database and their > > features > > have been based on their DJ device counterparts. > > No changes have been made to this patch in v4. However, it should be > noted > that the only device that has been thoroughly tested in this patch is > the > MX Master (b01e). Further testing for the other devices may be > required. > > Signed-off-by: Mazin Rezk <mnrzk@protonmail.com> > --- > drivers/hid/hid-logitech-hidpp.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid- > logitech-hidpp.c > index 0179f7ed77e5..85fd0c17cc2f 100644 > --- a/drivers/hid/hid-logitech-hidpp.c > +++ b/drivers/hid/hid-logitech-hidpp.c > @@ -3773,6 +3773,24 @@ static const struct hid_device_id > hidpp_devices[] = { > { /* MX5500 keyboard over Bluetooth */ > HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb30b), > .driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS }, > + { /* MX Anywhere 2 mouse over Bluetooth */ > + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb013), > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb018), > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > + { /* MX Anywhere 2S mouse over Bluetooth */ > + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01a), > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > + { /* MX Master mouse over Bluetooth */ > + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb012), > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb017), > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01e), > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > + { /* MX Master 2S mouse over Bluetooth */ > + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb019), > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > {} > }; > > -- > 2.23.0 > The series now looks great, thanks! Benjamin, I can confirm that up to now all BLE devices don't have short reports. I am not sure if you still want to only enable tested devices but from an architectural standpoint everything here should be fine. Mazin, you can have my Reviewed-by: Filipe Laíns <lains@archlinux.org> for the series. Thank you, Filipe Laíns
On Fri, Oct 11, 2019 at 10:49 AM Filipe Laíns <lains@archlinux.org> wrote: > > On Fri, 2019-10-11 at 00:57 +0000, Mazin Rezk wrote: > > On Saturday, October 5, 2019 9:04 PM, Mazin Rezk < > > mnrzk@protonmail.com> wrote: > > > > > This patch adds support for several MX mice over Bluetooth. The > > > device IDs > > > have been copied from the libratbag device database and their > > > features > > > have been based on their DJ device counterparts. > > > > No changes have been made to this patch in v4. However, it should be > > noted > > that the only device that has been thoroughly tested in this patch is > > the > > MX Master (b01e). Further testing for the other devices may be > > required. > > > > Signed-off-by: Mazin Rezk <mnrzk@protonmail.com> > > --- > > drivers/hid/hid-logitech-hidpp.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid- > > logitech-hidpp.c > > index 0179f7ed77e5..85fd0c17cc2f 100644 > > --- a/drivers/hid/hid-logitech-hidpp.c > > +++ b/drivers/hid/hid-logitech-hidpp.c > > @@ -3773,6 +3773,24 @@ static const struct hid_device_id > > hidpp_devices[] = { > > { /* MX5500 keyboard over Bluetooth */ > > HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb30b), > > .driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS }, > > + { /* MX Anywhere 2 mouse over Bluetooth */ > > + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb013), > > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb018), > > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > > + { /* MX Anywhere 2S mouse over Bluetooth */ > > + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01a), > > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > > + { /* MX Master mouse over Bluetooth */ > > + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb012), > > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb017), > > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01e), > > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > > + { /* MX Master 2S mouse over Bluetooth */ > > + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb019), > > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > > {} > > }; > > > > -- > > 2.23.0 > > > > The series now looks great, thanks! > > Benjamin, I can confirm that up to now all BLE devices don't have short > reports. I am not sure if you still want to only enable tested devices > but from an architectural standpoint everything here should be fine. Unfortunately yes, we need actual device tests: - this series enable 0x2121 on all of those devices (is it correct?) - we are not shielded from a FW error and something that goes wrong when enabling one of those mice with hid-logitech-hidpp.c. All of those mice works fine with hid-generic, and if we oversee one tiny bit, we'll regress for no good reasons. Cheers, Benjamin > > Mazin, you can have my > > Reviewed-by: Filipe Laíns <lains@archlinux.org> > > for the series. > > Thank you, > Filipe Laíns
On Fri, 2019-10-11 at 10:54 +0200, Benjamin Tissoires wrote: > On Fri, Oct 11, 2019 at 10:49 AM Filipe Laíns <lains@archlinux.org> > wrote: > > On Fri, 2019-10-11 at 00:57 +0000, Mazin Rezk wrote: > > > On Saturday, October 5, 2019 9:04 PM, Mazin Rezk < > > > mnrzk@protonmail.com> wrote: > > > > > > > This patch adds support for several MX mice over Bluetooth. The > > > > device IDs > > > > have been copied from the libratbag device database and their > > > > features > > > > have been based on their DJ device counterparts. > > > > > > No changes have been made to this patch in v4. However, it should > > > be > > > noted > > > that the only device that has been thoroughly tested in this > > > patch is > > > the > > > MX Master (b01e). Further testing for the other devices may be > > > required. > > > > > > Signed-off-by: Mazin Rezk <mnrzk@protonmail.com> > > > --- > > > drivers/hid/hid-logitech-hidpp.c | 18 ++++++++++++++++++ > > > 1 file changed, 18 insertions(+) > > > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid- > > > logitech-hidpp.c > > > index 0179f7ed77e5..85fd0c17cc2f 100644 > > > --- a/drivers/hid/hid-logitech-hidpp.c > > > +++ b/drivers/hid/hid-logitech-hidpp.c > > > @@ -3773,6 +3773,24 @@ static const struct hid_device_id > > > hidpp_devices[] = { > > > { /* MX5500 keyboard over Bluetooth */ > > > HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb30b), > > > .driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS }, > > > + { /* MX Anywhere 2 mouse over Bluetooth */ > > > + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb013), > > > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb018), > > > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > > > + { /* MX Anywhere 2S mouse over Bluetooth */ > > > + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01a), > > > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > > > + { /* MX Master mouse over Bluetooth */ > > > + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb012), > > > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb017), > > > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01e), > > > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > > > + { /* MX Master 2S mouse over Bluetooth */ > > > + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb019), > > > + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, > > > {} > > > }; > > > > > > -- > > > 2.23.0 > > > > > > > The series now looks great, thanks! > > > > Benjamin, I can confirm that up to now all BLE devices don't have > > short > > reports. I am not sure if you still want to only enable tested > > devices > > but from an architectural standpoint everything here should be > > fine. > > Unfortunately yes, we need actual device tests: > - this series enable 0x2121 on all of those devices (is it correct?) > - we are not shielded from a FW error and something that goes wrong > when enabling one of those mice with hid-logitech-hidpp.c. All of > those mice works fine with hid-generic, and if we oversee one tiny > bit, we'll regress for no good reasons. Okay, makes sense :) > Cheers, > Benjamin > > > Mazin, you can have my > > > > Reviewed-by: Filipe Laíns <lains@archlinux.org> > > > > for the series. > > > > Thank you, > > Filipe Laíns
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index 0179f7ed77e5..85fd0c17cc2f 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -3773,6 +3773,24 @@ static const struct hid_device_id hidpp_devices[] = { { /* MX5500 keyboard over Bluetooth */ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb30b), .driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS }, + { /* MX Anywhere 2 mouse over Bluetooth */ + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb013), + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb018), + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, + { /* MX Anywhere 2S mouse over Bluetooth */ + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01a), + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, + { /* MX Master mouse over Bluetooth */ + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb012), + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb017), + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01e), + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, + { /* MX Master 2S mouse over Bluetooth */ + HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb019), + .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 }, {} };