[PATCH v4 1/4] HID: logitech: Add MX Mice over Bluetooth

[PATCH v4 1/4] HID: logitech: Add MX Mice over Bluetooth

[Mazin Rezk]

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

Re: [PATCH v4 1/4] HID: logitech: Add MX Mice over Bluetooth

[Benjamin Tissoires]

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
>

Re: [PATCH v4 1/4] HID: logitech: Add MX Mice over Bluetooth

[Filipe Laíns]

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

Re: [PATCH v4 1/4] HID: logitech: Add MX Mice over Bluetooth

[Benjamin Tissoires]

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

Re: [PATCH v4 1/4] HID: logitech: Add MX Mice over Bluetooth

[Filipe Laíns]

[-- Attachment #1: Type: text/plain, Size: 3669 bytes --]

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

-- 
Filipe Laíns

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 862 bytes --]