diff mbox series

[1/3] HID: logitech-dj: Handle quad/bluetooth keyboards with a builtin trackpad

Message ID 20201102133658.4410-1-hdegoede@redhat.com (mailing list archive)
State Mainlined
Commit ee5e58418a854755201eb4952b1230d873a457d5
Delegated to: Jiri Kosina
Headers show
Series [1/3] HID: logitech-dj: Handle quad/bluetooth keyboards with a builtin trackpad | expand

Commit Message

Hans de Goede Nov. 2, 2020, 1:36 p.m. UTC
Some quad/bluetooth keyboards, such as the Dinovo Edge (Y-RAY81) have a
builtin touchpad. In this case when asking the receiver for paired devices,
we get only 1 paired device with a device_type of REPORT_TYPE_KEYBOARD.

This means that we do not instantiate a second dj_hiddev for the mouse
(as we normally would) and thus there is no place for us to forward the
mouse input reports to, causing the touchpad part of the keyboard to not
work.

There is no way for us to detect these keyboards, so this commit adds
an array with device-ids for such keyboards and when a keyboard is on
this list it adds STD_MOUSE to the reports_supported bitmap for the
dj_hiddev created for the keyboard fixing the touchpad not working.

Using a list of device-ids for this is not ideal, but there are only
very few such keyboards so this should be fine. Besides the Dinovo Edge,
other known wireless Logitech keyboards with a builtin touchpad are:

* Dinovo Mini (TODO add its device-id to the list)
* K400 (uses a unifying receiver so is not affected)
* K600 (uses a unifying receiver so is not affected)

Cc: stable@vger.kernel.org
BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1811424
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-logitech-dj.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Hans de Goede Nov. 2, 2020, 1:44 p.m. UTC | #1
Hi,

On 11/2/20 2:36 PM, Hans de Goede wrote:
> Some quad/bluetooth keyboards, such as the Dinovo Edge (Y-RAY81) have a
> builtin touchpad. In this case when asking the receiver for paired devices,
> we get only 1 paired device with a device_type of REPORT_TYPE_KEYBOARD.
> 
> This means that we do not instantiate a second dj_hiddev for the mouse
> (as we normally would) and thus there is no place for us to forward the
> mouse input reports to, causing the touchpad part of the keyboard to not
> work.
> 
> There is no way for us to detect these keyboards, so this commit adds
> an array with device-ids for such keyboards and when a keyboard is on
> this list it adds STD_MOUSE to the reports_supported bitmap for the
> dj_hiddev created for the keyboard fixing the touchpad not working.
> 
> Using a list of device-ids for this is not ideal, but there are only
> very few such keyboards so this should be fine. Besides the Dinovo Edge,
> other known wireless Logitech keyboards with a builtin touchpad are:
> 
> * Dinovo Mini (TODO add its device-id to the list)

Benjamin, you have a Dinovo Mini, right ?

It looks like that is using the same quad/bluetooth combo receiver
as the Dinovo Edge, but then with slightly different USB ids, which
means that atm we are not using the logitech-dj driver for it.

But the dongles appear to be interchangeable I can pair the Dinovo
Edge with both the MX5000 and the MX5500 dongles which I have, so
someone who mixes up dongles (or gets a spare one) could end up
using the Dinovo Mini with a dongle which is already handled by
the logitech-dj driver.

As such it would be good if you can add the Dinovo Mini to the
device-id list this patch introduces (or if you tell me the device-id
I can do a v2 adding it depending on the timing).

Also I think you should probably add the USB-ids for your
Dinovo dongle to the logitech-dj driver. This will allow you
to verify that adding the device-id is necessary and also
will give you battery status reporting while used in USB HID
proxy mode.

Last you may want to check battery-status reporting in Bluetooth
mode, and maybe also make the logitech-hidpp driver handle the
Dinovo Mini in bluetooth mode, as at least on the Dinovo Edge
the standard HID battery reporting done in bluetooth mode
(and not in HID proxy mode interesting enough) seems to be
broken.

Regards,

Hans



> * K400 (uses a unifying receiver so is not affected)
> * K600 (uses a unifying receiver so is not affected)
> 
> Cc: stable@vger.kernel.org
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1811424
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/hid/hid-logitech-dj.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index ea1e40530f85..9ed7260b9593 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -867,11 +867,23 @@ static void logi_dj_recv_queue_notification(struct dj_receiver_dev *djrcv_dev,
>  	schedule_work(&djrcv_dev->work);
>  }
>  
> +/*
> + * Some quad/bluetooth keyboards have a builtin touchpad in this case we see
> + * only 1 paired device with a device_type of REPORT_TYPE_KEYBOARD. For the
> + * touchpad to work we must also forward mouse input reports to the dj_hiddev
> + * created for the keyboard (instead of forwarding them to a second paired
> + * device with a device_type of REPORT_TYPE_MOUSE as we normally would).
> + */
> +static const u16 kbd_builtin_touchpad_ids[] = {
> +	0xb309, /* Dinovo Edge */
> +};
> +
>  static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev,
>  					    struct hidpp_event *hidpp_report,
>  					    struct dj_workitem *workitem)
>  {
>  	struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev);
> +	int i, id;
>  
>  	workitem->type = WORKITEM_TYPE_PAIRED;
>  	workitem->device_type = hidpp_report->params[HIDPP_PARAM_DEVICE_INFO] &
> @@ -883,6 +895,13 @@ static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev,
>  		workitem->reports_supported |= STD_KEYBOARD | MULTIMEDIA |
>  					       POWER_KEYS | MEDIA_CENTER |
>  					       HIDPP;
> +		id = (workitem->quad_id_msb << 8) | workitem->quad_id_lsb;
> +		for (i = 0; i < ARRAY_SIZE(kbd_builtin_touchpad_ids); i++) {
> +			if (id == kbd_builtin_touchpad_ids[i]) {
> +				workitem->reports_supported |= STD_MOUSE;
> +				break;
> +			}
> +		}
>  		break;
>  	case REPORT_TYPE_MOUSE:
>  		workitem->reports_supported |= STD_MOUSE | HIDPP;
>
Hans de Goede Nov. 10, 2020, 1:16 p.m. UTC | #2
Hi All,

On 11/2/20 2:36 PM, Hans de Goede wrote:
> Some quad/bluetooth keyboards, such as the Dinovo Edge (Y-RAY81) have a
> builtin touchpad. In this case when asking the receiver for paired devices,
> we get only 1 paired device with a device_type of REPORT_TYPE_KEYBOARD.
> 
> This means that we do not instantiate a second dj_hiddev for the mouse
> (as we normally would) and thus there is no place for us to forward the
> mouse input reports to, causing the touchpad part of the keyboard to not
> work.
> 
> There is no way for us to detect these keyboards, so this commit adds
> an array with device-ids for such keyboards and when a keyboard is on
> this list it adds STD_MOUSE to the reports_supported bitmap for the
> dj_hiddev created for the keyboard fixing the touchpad not working.
> 
> Using a list of device-ids for this is not ideal, but there are only
> very few such keyboards so this should be fine. Besides the Dinovo Edge,
> other known wireless Logitech keyboards with a builtin touchpad are:
> 
> * Dinovo Mini (TODO add its device-id to the list)
> * K400 (uses a unifying receiver so is not affected)
> * K600 (uses a unifying receiver so is not affected)
> 
> Cc: stable@vger.kernel.org
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1811424
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

ping? This is a bug fix for a regression caused by:

Commit f2113c3020ef ("HID: logitech-dj: add support for Logitech Bluetooth Mini-Receiver")

Specifically that commit caused the builtin touchpad to stop working on Logitech Dinovo
Edge keyboards and this fixes this.

I realize now that I forgot to add a:

Fixes: f2113c3020ef ("HID: logitech-dj: add support for Logitech Bluetooth Mini-Receiver")

Tag, let me know if you want a v2 for that.

Regardless since this is a bug fix, it would be good if we can get this
merged into one of the upcoming 5.10-rc#s. Even without the Dinovo Mini
id added this is still worthwhile to get the reported regression fixed
and we can add the Dinovo Mini id later.

Regards,

Hans




> ---
>  drivers/hid/hid-logitech-dj.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index ea1e40530f85..9ed7260b9593 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -867,11 +867,23 @@ static void logi_dj_recv_queue_notification(struct dj_receiver_dev *djrcv_dev,
>  	schedule_work(&djrcv_dev->work);
>  }
>  
> +/*
> + * Some quad/bluetooth keyboards have a builtin touchpad in this case we see
> + * only 1 paired device with a device_type of REPORT_TYPE_KEYBOARD. For the
> + * touchpad to work we must also forward mouse input reports to the dj_hiddev
> + * created for the keyboard (instead of forwarding them to a second paired
> + * device with a device_type of REPORT_TYPE_MOUSE as we normally would).
> + */
> +static const u16 kbd_builtin_touchpad_ids[] = {
> +	0xb309, /* Dinovo Edge */
> +};
> +
>  static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev,
>  					    struct hidpp_event *hidpp_report,
>  					    struct dj_workitem *workitem)
>  {
>  	struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev);
> +	int i, id;
>  
>  	workitem->type = WORKITEM_TYPE_PAIRED;
>  	workitem->device_type = hidpp_report->params[HIDPP_PARAM_DEVICE_INFO] &
> @@ -883,6 +895,13 @@ static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev,
>  		workitem->reports_supported |= STD_KEYBOARD | MULTIMEDIA |
>  					       POWER_KEYS | MEDIA_CENTER |
>  					       HIDPP;
> +		id = (workitem->quad_id_msb << 8) | workitem->quad_id_lsb;
> +		for (i = 0; i < ARRAY_SIZE(kbd_builtin_touchpad_ids); i++) {
> +			if (id == kbd_builtin_touchpad_ids[i]) {
> +				workitem->reports_supported |= STD_MOUSE;
> +				break;
> +			}
> +		}
>  		break;
>  	case REPORT_TYPE_MOUSE:
>  		workitem->reports_supported |= STD_MOUSE | HIDPP;
>
Benjamin Tissoires Nov. 10, 2020, 6:29 p.m. UTC | #3
Hi Hans,

On Tue, Nov 10, 2020 at 2:17 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> On 11/2/20 2:36 PM, Hans de Goede wrote:
> > Some quad/bluetooth keyboards, such as the Dinovo Edge (Y-RAY81) have a
> > builtin touchpad. In this case when asking the receiver for paired devices,
> > we get only 1 paired device with a device_type of REPORT_TYPE_KEYBOARD.
> >
> > This means that we do not instantiate a second dj_hiddev for the mouse
> > (as we normally would) and thus there is no place for us to forward the
> > mouse input reports to, causing the touchpad part of the keyboard to not
> > work.
> >
> > There is no way for us to detect these keyboards, so this commit adds
> > an array with device-ids for such keyboards and when a keyboard is on
> > this list it adds STD_MOUSE to the reports_supported bitmap for the
> > dj_hiddev created for the keyboard fixing the touchpad not working.
> >
> > Using a list of device-ids for this is not ideal, but there are only
> > very few such keyboards so this should be fine. Besides the Dinovo Edge,
> > other known wireless Logitech keyboards with a builtin touchpad are:
> >
> > * Dinovo Mini (TODO add its device-id to the list)
> > * K400 (uses a unifying receiver so is not affected)
> > * K600 (uses a unifying receiver so is not affected)
> >
> > Cc: stable@vger.kernel.org
> > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1811424
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> ping? This is a bug fix for a regression caused by:

Series looks good. I tried to enable the Dinovo Mini this afternoon (see
patch below), but it's not that clean and easy...

>
> Commit f2113c3020ef ("HID: logitech-dj: add support for Logitech Bluetooth Mini-Receiver")
>
> Specifically that commit caused the builtin touchpad to stop working on Logitech Dinovo
> Edge keyboards and this fixes this.
>
> I realize now that I forgot to add a:
>
> Fixes: f2113c3020ef ("HID: logitech-dj: add support for Logitech Bluetooth Mini-Receiver")
>
> Tag, let me know if you want a v2 for that.

I guess you want the tag on all 3 patches, not just the first.

If so, I can try to push it later today or tomorrow.


>
> Regardless since this is a bug fix, it would be good if we can get this
> merged into one of the upcoming 5.10-rc#s. Even without the Dinovo Mini
> id added this is still worthwhile to get the reported regression fixed
> and we can add the Dinovo Mini id later.

Yeah, the Dinovo Mini will come later.

My current WIP is the following:

---

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 1cafb65428b0..1c7857bf3290 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -84,6 +84,7 @@
  #define STD_MOUSE				BIT(2)
  #define MULTIMEDIA				BIT(3)
  #define POWER_KEYS				BIT(4)
+#define BT_MOUSE				BIT(5)
  #define MEDIA_CENTER				BIT(8)
  #define KBD_LEDS				BIT(14)
  /* Fake (bitnr > NUMBER_OF_HID_REPORTS) bit to track HID++ capability */
@@ -333,6 +334,47 @@ static const char mse_bluetooth_descriptor[] = {
  	0xC0,			/*  END_COLLECTION                      */
  };
  
+/* Mouse descriptor (5) for Bluetooth receiver, low-res hwheel, 8 buttons */
+static const char mse5_bluetooth_descriptor[] = {
+	0x05, 0x01,		/*  USAGE_PAGE (Generic Desktop)        */
+	0x09, 0x02,		/*  Usage (Mouse)                       */
+	0xa1, 0x01,		/*  Collection (Application)            */
+	0x85, 0x05,		/*   Report ID (5)                      */
+	0x09, 0x01,		/*   Usage (Pointer)                    */
+	0xa1, 0x00,		/*   Collection (Physical)              */
+	0x05, 0x09,		/*    Usage Page (Button)               */
+	0x19, 0x01,		/*    Usage Minimum (1)                 */
+	0x29, 0x08,		/*    Usage Maximum (8)                 */
+	0x15, 0x00,		/*    Logical Minimum (0)               */
+	0x25, 0x01,		/*    Logical Maximum (1)               */
+	0x95, 0x08,		/*    Report Count (8)                  */
+	0x75, 0x01,		/*    Report Size (1)                   */
+	0x81, 0x02,		/*    Input (Data,Var,Abs)              */
+	0x05, 0x01,		/*    Usage Page (Generic Desktop)      */
+	0x16, 0x01, 0xf8,	/*    Logical Minimum (-2047)           */
+	0x26, 0xff, 0x07,	/*    Logical Maximum (2047)            */
+	0x75, 0x0c,		/*    Report Size (12)                  */
+	0x95, 0x02,		/*    Report Count (2)                  */
+	0x09, 0x30,		/*    Usage (X)                         */
+	0x09, 0x31,		/*    Usage (Y)                         */
+	0x81, 0x06,		/*    Input (Data,Var,Rel)              */
+	0x15, 0x81,		/*    Logical Minimum (-127)            */
+	0x25, 0x7f,		/*    Logical Maximum (127)             */
+	0x75, 0x08,		/*    Report Size (8)                   */
+	0x95, 0x01,		/*    Report Count (1)                  */
+	0x09, 0x38,		/*    Usage (Wheel)                     */
+	0x81, 0x06,		/*    Input (Data,Var,Rel)              */
+	0x05, 0x0c,		/*    Usage Page (Consumer Devices)     */
+	0x0a, 0x38, 0x02,	/*    Usage (AC Pan)                    */
+	0x15, 0x81,		/*    Logical Minimum (-127)            */
+	0x25, 0x7f,		/*    Logical Maximum (127)             */
+	0x75, 0x08,		/*    Report Size (8)                   */
+	0x95, 0x01,		/*    Report Count (1)                  */
+	0x81, 0x06,		/*    Input (Data,Var,Rel)              */
+	0xc0,			/*   End Collection                     */
+	0xc0,			/*  End Collection                      */
+};
+
  /* Gaming Mouse descriptor (2) */
  static const char mse_high_res_descriptor[] = {
  	0x05, 0x01,		/*  USAGE_PAGE (Generic Desktop)        */
@@ -877,6 +919,10 @@ static const u16 kbd_builtin_touchpad_ids[] = {
  	0xb309, /* Dinovo Edge */
  };
  
+static const u16 kbd_builtin_touchpad5_ids[] = {
+	0xb30c, /* Dinovo Mini */
+};
+
  static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev,
  					    struct hidpp_event *hidpp_report,
  					    struct dj_workitem *workitem)
@@ -901,6 +947,12 @@ static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev,
  				break;
  			}
  		}
+		for (i = 0; i < ARRAY_SIZE(kbd_builtin_touchpad5_ids); i++) {
+			if (id == kbd_builtin_touchpad5_ids[i]) {
+				workitem->reports_supported |= BT_MOUSE;
+				break;
+			}
+		}
  		break;
  	case REPORT_TYPE_MOUSE:
  		workitem->reports_supported |= STD_MOUSE | HIDPP;
@@ -1368,6 +1420,13 @@ static int logi_dj_ll_parse(struct hid_device *hid)
  			      sizeof(mse_descriptor));
  	}
  
+	if (djdev->reports_supported & BT_MOUSE) {
+		dbg_hid("%s: sending a mouse descriptor, reports_supported: %llx\n",
+			__func__, djdev->reports_supported);
+		rdcat(rdesc, &rsize, mse5_bluetooth_descriptor,
+		      sizeof(mse5_bluetooth_descriptor));
+	}
+
  	if (djdev->reports_supported & MULTIMEDIA) {
  		dbg_hid("%s: sending a multimedia report descriptor: %llx\n",
  			__func__, djdev->reports_supported);
@@ -1907,6 +1966,14 @@ static const struct hid_device_id logi_dj_receivers[] = {
  	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
  		0xc71c),
  	 .driver_data = recvr_type_bluetooth},
+	{ /* Logitech DiNovo Mini HID++ / bluetooth receiver mouse intf. */
+	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
+		0xc71e),
+	 .driver_data = recvr_type_bluetooth},
+	{ /* Logitech DiNovo Mini HID++ / bluetooth receiver keyboard intf. */
+	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
+		0xc71f),
+	 .driver_data = recvr_type_bluetooth},
  	{}
  };
  
---

And the keyboard is not sending the proper KEY_MEDIA like with the
hid-logitech.ko driver. So this WIP can not go into a stable tree.

Cheers,
Benjamin
Hans de Goede Nov. 11, 2020, 11:06 a.m. UTC | #4
Hi,

On 11/10/20 7:29 PM, Benjamin Tissoires wrote:
> Hi Hans,
> 
> On Tue, Nov 10, 2020 at 2:17 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi All,
>>
>> On 11/2/20 2:36 PM, Hans de Goede wrote:
>> > Some quad/bluetooth keyboards, such as the Dinovo Edge (Y-RAY81) have a
>> > builtin touchpad. In this case when asking the receiver for paired devices,
>> > we get only 1 paired device with a device_type of REPORT_TYPE_KEYBOARD.
>> >
>> > This means that we do not instantiate a second dj_hiddev for the mouse
>> > (as we normally would) and thus there is no place for us to forward the
>> > mouse input reports to, causing the touchpad part of the keyboard to not
>> > work.
>> >
>> > There is no way for us to detect these keyboards, so this commit adds
>> > an array with device-ids for such keyboards and when a keyboard is on
>> > this list it adds STD_MOUSE to the reports_supported bitmap for the
>> > dj_hiddev created for the keyboard fixing the touchpad not working.
>> >
>> > Using a list of device-ids for this is not ideal, but there are only
>> > very few such keyboards so this should be fine. Besides the Dinovo Edge,
>> > other known wireless Logitech keyboards with a builtin touchpad are:
>> >
>> > * Dinovo Mini (TODO add its device-id to the list)
>> > * K400 (uses a unifying receiver so is not affected)
>> > * K600 (uses a unifying receiver so is not affected)
>> >
>> > Cc: stable@vger.kernel.org
>> > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1811424
>> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> ping? This is a bug fix for a regression caused by:
> 
> Series looks good. I tried to enable the Dinovo Mini this afternoon (see
> patch below), but it's not that clean and easy...
> 
>>
>> Commit f2113c3020ef ("HID: logitech-dj: add support for Logitech Bluetooth Mini-Receiver")
>>
>> Specifically that commit caused the builtin touchpad to stop working on Logitech Dinovo
>> Edge keyboards and this fixes this.
>>
>> I realize now that I forgot to add a:
>>
>> Fixes: f2113c3020ef ("HID: logitech-dj: add support for Logitech Bluetooth Mini-Receiver")
>>
>> Tag, let me know if you want a v2 for that.
> 
> I guess you want the tag on all 3 patches, not just the first.

Patch 2 and 3 do not fix a regression:

Patch 2 enables extra functionality (non working A-D and phone keys)
Patch 3 fixes there being 2 batteries under /sys/class/power when the kbd
is in bluetooth mode, this problem is introduced by patch 2.

I think that taking patch 2/3 for 5.10-rc# is fine, but they don't really
fix anything related to commit f2113c3020ef. If anything patch 3 should have
a fixes tag for the final commit hash of patch 2 (or maybe just squash them?)

Note not taking patch 2/3 for 5.10-rc# is fine too.

> If so, I can try to push it later today or tomorrow.

Sounds good, thank you.

>> Regardless since this is a bug fix, it would be good if we can get this
>> merged into one of the upcoming 5.10-rc#s. Even without the Dinovo Mini
>> id added this is still worthwhile to get the reported regression fixed
>> and we can add the Dinovo Mini id later.
> 
> Yeah, the Dinovo Mini will come later.
> 
> My current WIP is the following:

Oh interesting, and good timing, let me explain:

I bought a 2nd hand Dinovo Edge to debug the reported regression (*), but
that came without a receiver. So I paired it with the MX5000 receiver which
I already had (and which has the same USB-ids as the reporters receiver)
and that works fine with this patch.

But I did want to have a complete set, so I found some US store on amazon
selling spare Dinovo Edge dongles and shipping them to Europe in a letter
(so no crazy shipping costs). That dongle arrived yesterday and I did a
quick test run. It has usb-ids of c713 for the keyboard usb-device and
c714 for the mouse usb-device (the dongle has a builtin hub and presents
2 separate usb devices, like the MX5000 / MX5500 dongles).

So I added these ids to hid-logitech-dj.c (and dropped one of them from hid-lg.c)
after that the mousepad on the Dinovo Edge however stopped working again
when paired with this dongle. So I already guessed that the mouse descriptor
would be different, but I did not get around to actually checking this.

Note this has an important implication for your patch, you assume the mouse-report
changes based on the paired device. But that is not the case it is simple that
some (newer? at least a somewhat higher usb-dev-id) quad/bt2.0 combo dongles
have a different mouse report.

At least with the Dinovo Edge the mouse report changes when it is paired with
a different dongle, so it is the dongle which determines the mouse report, not
the paired device.

So we need a "recvr_type_bluetooth_v2" and then this new report can just be added
to the big:

                if (djdev->dj_receiver_dev->type == recvr_type_gaming_hidpp ||
                    djdev->dj_receiver_dev->type == recvr_type_mouse_only)
                        rdcat(rdesc, &rsize, mse_high_res_descriptor,
                              sizeof(mse_high_res_descriptor));
                else if (djdev->dj_receiver_dev->type == recvr_type_27mhz)
                        rdcat(rdesc, &rsize, mse_27mhz_descriptor,
                              sizeof(mse_27mhz_descriptor));
                else if (djdev->dj_receiver_dev->type == recvr_type_bluetooth)
                        rdcat(rdesc, &rsize, mse_bluetooth_descriptor,
                              sizeof(mse_bluetooth_descriptor));
                else
                        rdcat(rdesc, &rsize, mse_descriptor,
                              sizeof(mse_descriptor));

block.

Hmm, I also see that the new descriptor has a report-id of 5, so it looks like
we do need the BT_MOUSE thing, but then set it based on the receiver usb-id instead?

Do the original HID descriptors of the receiver perhaps have both a report 2 and
a report 5 and we should add both ?

Note I also see that you add a separate id-array for the dinovo-mini because
of this given my experience that the behavior changes based on the used
receiver, I don't think that is necessary. Instead we need to add or not
add the BT_MOUSE bit to the keyboards reports_supported based on the
receiver-type (I think).

Anyways this definitely needs some more work, so as you said lets move
forward with the fix for the MX5000 receiver usb-ids as is.

Although adding the dinovo-mini device-id to the kbd_builtin_touchpad_ids[]
in case it gets paired with sat the MX5000 receiver probably cannot hurt.

Regards,

Hans




*) and I'm glad I did I don't think I would have enjoyed debugging this remotely




> ---
> 
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index 1cafb65428b0..1c7857bf3290 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -84,6 +84,7 @@
>  #define STD_MOUSE                BIT(2)
>  #define MULTIMEDIA                BIT(3)
>  #define POWER_KEYS                BIT(4)
> +#define BT_MOUSE                BIT(5)
>  #define MEDIA_CENTER                BIT(8)
>  #define KBD_LEDS                BIT(14)
>  /* Fake (bitnr > NUMBER_OF_HID_REPORTS) bit to track HID++ capability */
> @@ -333,6 +334,47 @@ static const char mse_bluetooth_descriptor[] = {
>      0xC0,            /*  END_COLLECTION                      */
>  };
>  
> +/* Mouse descriptor (5) for Bluetooth receiver, low-res hwheel, 8 buttons */
> +static const char mse5_bluetooth_descriptor[] = {
> +    0x05, 0x01,        /*  USAGE_PAGE (Generic Desktop)        */
> +    0x09, 0x02,        /*  Usage (Mouse)                       */
> +    0xa1, 0x01,        /*  Collection (Application)            */
> +    0x85, 0x05,        /*   Report ID (5)                      */
> +    0x09, 0x01,        /*   Usage (Pointer)                    */
> +    0xa1, 0x00,        /*   Collection (Physical)              */
> +    0x05, 0x09,        /*    Usage Page (Button)               */
> +    0x19, 0x01,        /*    Usage Minimum (1)                 */
> +    0x29, 0x08,        /*    Usage Maximum (8)                 */
> +    0x15, 0x00,        /*    Logical Minimum (0)               */
> +    0x25, 0x01,        /*    Logical Maximum (1)               */
> +    0x95, 0x08,        /*    Report Count (8)                  */
> +    0x75, 0x01,        /*    Report Size (1)                   */
> +    0x81, 0x02,        /*    Input (Data,Var,Abs)              */
> +    0x05, 0x01,        /*    Usage Page (Generic Desktop)      */
> +    0x16, 0x01, 0xf8,    /*    Logical Minimum (-2047)           */
> +    0x26, 0xff, 0x07,    /*    Logical Maximum (2047)            */
> +    0x75, 0x0c,        /*    Report Size (12)                  */
> +    0x95, 0x02,        /*    Report Count (2)                  */
> +    0x09, 0x30,        /*    Usage (X)                         */
> +    0x09, 0x31,        /*    Usage (Y)                         */
> +    0x81, 0x06,        /*    Input (Data,Var,Rel)              */
> +    0x15, 0x81,        /*    Logical Minimum (-127)            */
> +    0x25, 0x7f,        /*    Logical Maximum (127)             */
> +    0x75, 0x08,        /*    Report Size (8)                   */
> +    0x95, 0x01,        /*    Report Count (1)                  */
> +    0x09, 0x38,        /*    Usage (Wheel)                     */
> +    0x81, 0x06,        /*    Input (Data,Var,Rel)              */
> +    0x05, 0x0c,        /*    Usage Page (Consumer Devices)     */
> +    0x0a, 0x38, 0x02,    /*    Usage (AC Pan)                    */
> +    0x15, 0x81,        /*    Logical Minimum (-127)            */
> +    0x25, 0x7f,        /*    Logical Maximum (127)             */
> +    0x75, 0x08,        /*    Report Size (8)                   */
> +    0x95, 0x01,        /*    Report Count (1)                  */
> +    0x81, 0x06,        /*    Input (Data,Var,Rel)              */
> +    0xc0,            /*   End Collection                     */
> +    0xc0,            /*  End Collection                      */
> +};
> +
>  /* Gaming Mouse descriptor (2) */
>  static const char mse_high_res_descriptor[] = {
>      0x05, 0x01,        /*  USAGE_PAGE (Generic Desktop)        */
> @@ -877,6 +919,10 @@ static const u16 kbd_builtin_touchpad_ids[] = {
>      0xb309, /* Dinovo Edge */
>  };
>  
> +static const u16 kbd_builtin_touchpad5_ids[] = {
> +    0xb30c, /* Dinovo Mini */
> +};
> +
>  static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev,
>                          struct hidpp_event *hidpp_report,
>                          struct dj_workitem *workitem)
> @@ -901,6 +947,12 @@ static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev,
>                  break;
>              }
>          }
> +        for (i = 0; i < ARRAY_SIZE(kbd_builtin_touchpad5_ids); i++) {
> +            if (id == kbd_builtin_touchpad5_ids[i]) {
> +                workitem->reports_supported |= BT_MOUSE;
> +                break;
> +            }
> +        }
>          break;
>      case REPORT_TYPE_MOUSE:
>          workitem->reports_supported |= STD_MOUSE | HIDPP;
> @@ -1368,6 +1420,13 @@ static int logi_dj_ll_parse(struct hid_device *hid)
>                    sizeof(mse_descriptor));
>      }
>  
> +    if (djdev->reports_supported & BT_MOUSE) {
> +        dbg_hid("%s: sending a mouse descriptor, reports_supported: %llx\n",
> +            __func__, djdev->reports_supported);
> +        rdcat(rdesc, &rsize, mse5_bluetooth_descriptor,
> +              sizeof(mse5_bluetooth_descriptor));
> +    }
> +
>      if (djdev->reports_supported & MULTIMEDIA) {
>          dbg_hid("%s: sending a multimedia report descriptor: %llx\n",
>              __func__, djdev->reports_supported);
> @@ -1907,6 +1966,14 @@ static const struct hid_device_id logi_dj_receivers[] = {
>        HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
>          0xc71c),
>       .driver_data = recvr_type_bluetooth},
> +    { /* Logitech DiNovo Mini HID++ / bluetooth receiver mouse intf. */
> +      HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> +        0xc71e),
> +     .driver_data = recvr_type_bluetooth},
> +    { /* Logitech DiNovo Mini HID++ / bluetooth receiver keyboard intf. */
> +      HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> +        0xc71f),
> +     .driver_data = recvr_type_bluetooth},
>      {}
>  };
>  
> ---
> 
> And the keyboard is not sending the proper KEY_MEDIA like with the
> hid-logitech.ko driver. So this WIP can not go into a stable tree.
> 
> Cheers,
> Benjamin
>
Benjamin Tissoires Nov. 12, 2020, 3:49 p.m. UTC | #5
On Wed, Nov 11, 2020 at 12:07 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 11/10/20 7:29 PM, Benjamin Tissoires wrote:
> > Hi Hans,
> >
> > On Tue, Nov 10, 2020 at 2:17 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi All,
> >>
> >> On 11/2/20 2:36 PM, Hans de Goede wrote:
> >> > Some quad/bluetooth keyboards, such as the Dinovo Edge (Y-RAY81) have a
> >> > builtin touchpad. In this case when asking the receiver for paired devices,
> >> > we get only 1 paired device with a device_type of REPORT_TYPE_KEYBOARD.
> >> >
> >> > This means that we do not instantiate a second dj_hiddev for the mouse
> >> > (as we normally would) and thus there is no place for us to forward the
> >> > mouse input reports to, causing the touchpad part of the keyboard to not
> >> > work.
> >> >
> >> > There is no way for us to detect these keyboards, so this commit adds
> >> > an array with device-ids for such keyboards and when a keyboard is on
> >> > this list it adds STD_MOUSE to the reports_supported bitmap for the
> >> > dj_hiddev created for the keyboard fixing the touchpad not working.
> >> >
> >> > Using a list of device-ids for this is not ideal, but there are only
> >> > very few such keyboards so this should be fine. Besides the Dinovo Edge,
> >> > other known wireless Logitech keyboards with a builtin touchpad are:
> >> >
> >> > * Dinovo Mini (TODO add its device-id to the list)
> >> > * K400 (uses a unifying receiver so is not affected)
> >> > * K600 (uses a unifying receiver so is not affected)
> >> >
> >> > Cc: stable@vger.kernel.org
> >> > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1811424
> >> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>
> >> ping? This is a bug fix for a regression caused by:
> >
> > Series looks good. I tried to enable the Dinovo Mini this afternoon (see
> > patch below), but it's not that clean and easy...
> >
> >>
> >> Commit f2113c3020ef ("HID: logitech-dj: add support for Logitech Bluetooth Mini-Receiver")
> >>
> >> Specifically that commit caused the builtin touchpad to stop working on Logitech Dinovo
> >> Edge keyboards and this fixes this.
> >>
> >> I realize now that I forgot to add a:
> >>
> >> Fixes: f2113c3020ef ("HID: logitech-dj: add support for Logitech Bluetooth Mini-Receiver")
> >>
> >> Tag, let me know if you want a v2 for that.
> >
> > I guess you want the tag on all 3 patches, not just the first.
>
> Patch 2 and 3 do not fix a regression:
>
> Patch 2 enables extra functionality (non working A-D and phone keys)
> Patch 3 fixes there being 2 batteries under /sys/class/power when the kbd
> is in bluetooth mode, this problem is introduced by patch 2.
>
> I think that taking patch 2/3 for 5.10-rc# is fine, but they don't really
> fix anything related to commit f2113c3020ef. If anything patch 3 should have
> a fixes tag for the final commit hash of patch 2 (or maybe just squash them?)
>
> Note not taking patch 2/3 for 5.10-rc# is fine too.
>
> > If so, I can try to push it later today or tomorrow.
>
> Sounds good, thank you.

I have now applied the 3 patches to the for-5.10/upstream-fixes branch.
I also added the Fixes tag to the first commit only.

>
> >> Regardless since this is a bug fix, it would be good if we can get this
> >> merged into one of the upcoming 5.10-rc#s. Even without the Dinovo Mini
> >> id added this is still worthwhile to get the reported regression fixed
> >> and we can add the Dinovo Mini id later.
> >
> > Yeah, the Dinovo Mini will come later.
> >
> > My current WIP is the following:
>
> Oh interesting, and good timing, let me explain:
>
> I bought a 2nd hand Dinovo Edge to debug the reported regression (*), but
> that came without a receiver. So I paired it with the MX5000 receiver which
> I already had (and which has the same USB-ids as the reporters receiver)
> and that works fine with this patch.
>
> But I did want to have a complete set, so I found some US store on amazon
> selling spare Dinovo Edge dongles and shipping them to Europe in a letter
> (so no crazy shipping costs). That dongle arrived yesterday and I did a
> quick test run. It has usb-ids of c713 for the keyboard usb-device and
> c714 for the mouse usb-device (the dongle has a builtin hub and presents
> 2 separate usb devices, like the MX5000 / MX5500 dongles).
>
> So I added these ids to hid-logitech-dj.c (and dropped one of them from hid-lg.c)
> after that the mousepad on the Dinovo Edge however stopped working again
> when paired with this dongle. So I already guessed that the mouse descriptor
> would be different, but I did not get around to actually checking this.
>
> Note this has an important implication for your patch, you assume the mouse-report
> changes based on the paired device. But that is not the case it is simple that
> some (newer? at least a somewhat higher usb-dev-id) quad/bt2.0 combo dongles
> have a different mouse report.
>
> At least with the Dinovo Edge the mouse report changes when it is paired with
> a different dongle, so it is the dongle which determines the mouse report, not
> the paired device.

Glad this is the case. My patch was just trying to get things around
while not breaking too many things. So there will be refinements to do
later.

>
> So we need a "recvr_type_bluetooth_v2" and then this new report can just be added
> to the big:
>
>                 if (djdev->dj_receiver_dev->type == recvr_type_gaming_hidpp ||
>                     djdev->dj_receiver_dev->type == recvr_type_mouse_only)
>                         rdcat(rdesc, &rsize, mse_high_res_descriptor,
>                               sizeof(mse_high_res_descriptor));
>                 else if (djdev->dj_receiver_dev->type == recvr_type_27mhz)
>                         rdcat(rdesc, &rsize, mse_27mhz_descriptor,
>                               sizeof(mse_27mhz_descriptor));
>                 else if (djdev->dj_receiver_dev->type == recvr_type_bluetooth)
>                         rdcat(rdesc, &rsize, mse_bluetooth_descriptor,
>                               sizeof(mse_bluetooth_descriptor));
>                 else
>                         rdcat(rdesc, &rsize, mse_descriptor,
>                               sizeof(mse_descriptor));
>
> block.

That would seem like a good solution, yes.

>
> Hmm, I also see that the new descriptor has a report-id of 5, so it looks like
> we do need the BT_MOUSE thing, but then set it based on the receiver usb-id instead?

Right.

>
> Do the original HID descriptors of the receiver perhaps have both a report 2 and
> a report 5 and we should add both ?

No, I think it only has report ID 5.

>
> Note I also see that you add a separate id-array for the dinovo-mini because
> of this given my experience that the behavior changes based on the used
> receiver, I don't think that is necessary. Instead we need to add or not
> add the BT_MOUSE bit to the keyboards reports_supported based on the
> receiver-type (I think).

Ack

>
> Anyways this definitely needs some more work, so as you said lets move
> forward with the fix for the MX5000 receiver usb-ids as is.
>
> Although adding the dinovo-mini device-id to the kbd_builtin_touchpad_ids[]
> in case it gets paired with sat the MX5000 receiver probably cannot hurt.

Sure. Feel free to send any followup patches.

Cheers,
Benjamin

>
> Regards,
>
> Hans
>
>
>
>
> *) and I'm glad I did I don't think I would have enjoyed debugging this remotely
>
>
>
>
> > ---
> >
> > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> > index 1cafb65428b0..1c7857bf3290 100644
> > --- a/drivers/hid/hid-logitech-dj.c
> > +++ b/drivers/hid/hid-logitech-dj.c
> > @@ -84,6 +84,7 @@
> >  #define STD_MOUSE                BIT(2)
> >  #define MULTIMEDIA                BIT(3)
> >  #define POWER_KEYS                BIT(4)
> > +#define BT_MOUSE                BIT(5)
> >  #define MEDIA_CENTER                BIT(8)
> >  #define KBD_LEDS                BIT(14)
> >  /* Fake (bitnr > NUMBER_OF_HID_REPORTS) bit to track HID++ capability */
> > @@ -333,6 +334,47 @@ static const char mse_bluetooth_descriptor[] = {
> >      0xC0,            /*  END_COLLECTION                      */
> >  };
> >
> > +/* Mouse descriptor (5) for Bluetooth receiver, low-res hwheel, 8 buttons */
> > +static const char mse5_bluetooth_descriptor[] = {
> > +    0x05, 0x01,        /*  USAGE_PAGE (Generic Desktop)        */
> > +    0x09, 0x02,        /*  Usage (Mouse)                       */
> > +    0xa1, 0x01,        /*  Collection (Application)            */
> > +    0x85, 0x05,        /*   Report ID (5)                      */
> > +    0x09, 0x01,        /*   Usage (Pointer)                    */
> > +    0xa1, 0x00,        /*   Collection (Physical)              */
> > +    0x05, 0x09,        /*    Usage Page (Button)               */
> > +    0x19, 0x01,        /*    Usage Minimum (1)                 */
> > +    0x29, 0x08,        /*    Usage Maximum (8)                 */
> > +    0x15, 0x00,        /*    Logical Minimum (0)               */
> > +    0x25, 0x01,        /*    Logical Maximum (1)               */
> > +    0x95, 0x08,        /*    Report Count (8)                  */
> > +    0x75, 0x01,        /*    Report Size (1)                   */
> > +    0x81, 0x02,        /*    Input (Data,Var,Abs)              */
> > +    0x05, 0x01,        /*    Usage Page (Generic Desktop)      */
> > +    0x16, 0x01, 0xf8,    /*    Logical Minimum (-2047)           */
> > +    0x26, 0xff, 0x07,    /*    Logical Maximum (2047)            */
> > +    0x75, 0x0c,        /*    Report Size (12)                  */
> > +    0x95, 0x02,        /*    Report Count (2)                  */
> > +    0x09, 0x30,        /*    Usage (X)                         */
> > +    0x09, 0x31,        /*    Usage (Y)                         */
> > +    0x81, 0x06,        /*    Input (Data,Var,Rel)              */
> > +    0x15, 0x81,        /*    Logical Minimum (-127)            */
> > +    0x25, 0x7f,        /*    Logical Maximum (127)             */
> > +    0x75, 0x08,        /*    Report Size (8)                   */
> > +    0x95, 0x01,        /*    Report Count (1)                  */
> > +    0x09, 0x38,        /*    Usage (Wheel)                     */
> > +    0x81, 0x06,        /*    Input (Data,Var,Rel)              */
> > +    0x05, 0x0c,        /*    Usage Page (Consumer Devices)     */
> > +    0x0a, 0x38, 0x02,    /*    Usage (AC Pan)                    */
> > +    0x15, 0x81,        /*    Logical Minimum (-127)            */
> > +    0x25, 0x7f,        /*    Logical Maximum (127)             */
> > +    0x75, 0x08,        /*    Report Size (8)                   */
> > +    0x95, 0x01,        /*    Report Count (1)                  */
> > +    0x81, 0x06,        /*    Input (Data,Var,Rel)              */
> > +    0xc0,            /*   End Collection                     */
> > +    0xc0,            /*  End Collection                      */
> > +};
> > +
> >  /* Gaming Mouse descriptor (2) */
> >  static const char mse_high_res_descriptor[] = {
> >      0x05, 0x01,        /*  USAGE_PAGE (Generic Desktop)        */
> > @@ -877,6 +919,10 @@ static const u16 kbd_builtin_touchpad_ids[] = {
> >      0xb309, /* Dinovo Edge */
> >  };
> >
> > +static const u16 kbd_builtin_touchpad5_ids[] = {
> > +    0xb30c, /* Dinovo Mini */
> > +};
> > +
> >  static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev,
> >                          struct hidpp_event *hidpp_report,
> >                          struct dj_workitem *workitem)
> > @@ -901,6 +947,12 @@ static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev,
> >                  break;
> >              }
> >          }
> > +        for (i = 0; i < ARRAY_SIZE(kbd_builtin_touchpad5_ids); i++) {
> > +            if (id == kbd_builtin_touchpad5_ids[i]) {
> > +                workitem->reports_supported |= BT_MOUSE;
> > +                break;
> > +            }
> > +        }
> >          break;
> >      case REPORT_TYPE_MOUSE:
> >          workitem->reports_supported |= STD_MOUSE | HIDPP;
> > @@ -1368,6 +1420,13 @@ static int logi_dj_ll_parse(struct hid_device *hid)
> >                    sizeof(mse_descriptor));
> >      }
> >
> > +    if (djdev->reports_supported & BT_MOUSE) {
> > +        dbg_hid("%s: sending a mouse descriptor, reports_supported: %llx\n",
> > +            __func__, djdev->reports_supported);
> > +        rdcat(rdesc, &rsize, mse5_bluetooth_descriptor,
> > +              sizeof(mse5_bluetooth_descriptor));
> > +    }
> > +
> >      if (djdev->reports_supported & MULTIMEDIA) {
> >          dbg_hid("%s: sending a multimedia report descriptor: %llx\n",
> >              __func__, djdev->reports_supported);
> > @@ -1907,6 +1966,14 @@ static const struct hid_device_id logi_dj_receivers[] = {
> >        HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> >          0xc71c),
> >       .driver_data = recvr_type_bluetooth},
> > +    { /* Logitech DiNovo Mini HID++ / bluetooth receiver mouse intf. */
> > +      HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> > +        0xc71e),
> > +     .driver_data = recvr_type_bluetooth},
> > +    { /* Logitech DiNovo Mini HID++ / bluetooth receiver keyboard intf. */
> > +      HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> > +        0xc71f),
> > +     .driver_data = recvr_type_bluetooth},
> >      {}
> >  };
> >
> > ---
> >
> > And the keyboard is not sending the proper KEY_MEDIA like with the
> > hid-logitech.ko driver. So this WIP can not go into a stable tree.
> >
> > Cheers,
> > Benjamin
> >
>
Hans de Goede Nov. 14, 2020, 11:03 a.m. UTC | #6
Hi,

On 11/12/20 4:49 PM, Benjamin Tissoires wrote:
> On Wed, Nov 11, 2020 at 12:07 PM Hans de Goede <hdegoede@redhat.com> wrote:

<snip>

>>> If so, I can try to push it later today or tomorrow.
>>
>> Sounds good, thank you.
> 
> I have now applied the 3 patches to the for-5.10/upstream-fixes branch.
> I also added the Fixes tag to the first commit only.

Great thank you.

<snip>

>>> Yeah, the Dinovo Mini will come later.

So I've been working on this today and I hope to have a patch for you
ready to test soon. So no need for you to spend time on this now,
that would just be us doing double work.

<snip>

>> Do the original HID descriptors of the receiver perhaps have both a report 2 and
>> a report 5 and we should add both ?
> 
> No, I think it only has report ID 5.

So I checked and there are actually both INPUT(2) and INPUT(5) descriptors
on the mouse USB-device's HID intf.

And looking at the USB ids again, this is not new vs old. This is Dinovo
vs non Dinovo. It seems that even though the air protocol is the same,
the Dinovo receivers have some special handling for the kbd touchpad where
as the non Dinovo receivers just send the same HID reports as for the normal
mouse.

I paired both the Dinovo Edge + a BT mouse (you can pair any BT mouse,
just press the pair button on the receiver and then on the mouse) with:

1. The MX5000 receiver as the RH bugzilla reporter is doing

In this case both the kbd-touchpad and the actual mouse send INPUT(2)
reports (there is no INPUT(5) in the descriptors).

2. The Dinovo Edge receiver. In this case the kbd-touchpad sends
INPUT(5) reports where as the actual mouse sends INPUT(2) reports.

I can see 2 reasons why Logitech did this:
1. tell apart kbd-touchpad events from actual mouse events
2. -127 - 127 hwheel range instead of -7 - 7, better for the infinite scrolling feature
  of the round touchpad (in horizontal direction)

No idea why they did not just enable INPUT(5) reporting on all
receivers though.

Anyways with the above knowledge I think I can write a nice patch
for this; and I've also found the hid-lg.c bit which needs to move
to hid-logitech-hidpp.c to keep the MEDIA key working on the Dinovo
Mini.

I hope to have a patch ready for your testing for this soon.

Regards,

Hans
Hans de Goede Nov. 14, 2020, 2:45 p.m. UTC | #7
Hi,

On 11/14/20 12:03 PM, Hans de Goede wrote:
> Hi,
> 
> On 11/12/20 4:49 PM, Benjamin Tissoires wrote:
>> On Wed, Nov 11, 2020 at 12:07 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> <snip>
> 
>>>> If so, I can try to push it later today or tomorrow.
>>>
>>> Sounds good, thank you.
>>
>> I have now applied the 3 patches to the for-5.10/upstream-fixes branch.
>> I also added the Fixes tag to the first commit only.
> 
> Great thank you.
> 
> <snip>
> 
>>>> Yeah, the Dinovo Mini will come later.
> 
> So I've been working on this today and I hope to have a patch for you
> ready to test soon. So no need for you to spend time on this now,
> that would just be us doing double work.
> 
> <snip>
> 
>>> Do the original HID descriptors of the receiver perhaps have both a report 2 and
>>> a report 5 and we should add both ?
>>
>> No, I think it only has report ID 5.
> 
> So I checked and there are actually both INPUT(2) and INPUT(5) descriptors
> on the mouse USB-device's HID intf.
> 
> And looking at the USB ids again, this is not new vs old. This is Dinovo
> vs non Dinovo. It seems that even though the air protocol is the same,
> the Dinovo receivers have some special handling for the kbd touchpad where
> as the non Dinovo receivers just send the same HID reports as for the normal
> mouse.
> 
> I paired both the Dinovo Edge + a BT mouse (you can pair any BT mouse,
> just press the pair button on the receiver and then on the mouse) with:
> 
> 1. The MX5000 receiver as the RH bugzilla reporter is doing
> 
> In this case both the kbd-touchpad and the actual mouse send INPUT(2)
> reports (there is no INPUT(5) in the descriptors).
> 
> 2. The Dinovo Edge receiver. In this case the kbd-touchpad sends
> INPUT(5) reports where as the actual mouse sends INPUT(2) reports.

p.s.

There being 2 mouse reports on the mouse intf. also explains why
the hid-logitech-lg.c code needs the special LG_DUPLICATE_USAGES
for the Dinovo receivers. We won't have this issue in the logitech-dj
code since there (with the patch I'm working on) the INPUT(5) reports
will be send to the keyboard hidpp-device where as the INPUT(2) reports
will keep being send to the mouse hidpp-device as usual. So there will
not be a single hidpp device which receives both mouse-reports and
thus no need for something like the LG_DUPLICATE_USAGES quirk.

Regards,

Hans
diff mbox series

Patch

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index ea1e40530f85..9ed7260b9593 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -867,11 +867,23 @@  static void logi_dj_recv_queue_notification(struct dj_receiver_dev *djrcv_dev,
 	schedule_work(&djrcv_dev->work);
 }
 
+/*
+ * Some quad/bluetooth keyboards have a builtin touchpad in this case we see
+ * only 1 paired device with a device_type of REPORT_TYPE_KEYBOARD. For the
+ * touchpad to work we must also forward mouse input reports to the dj_hiddev
+ * created for the keyboard (instead of forwarding them to a second paired
+ * device with a device_type of REPORT_TYPE_MOUSE as we normally would).
+ */
+static const u16 kbd_builtin_touchpad_ids[] = {
+	0xb309, /* Dinovo Edge */
+};
+
 static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev,
 					    struct hidpp_event *hidpp_report,
 					    struct dj_workitem *workitem)
 {
 	struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev);
+	int i, id;
 
 	workitem->type = WORKITEM_TYPE_PAIRED;
 	workitem->device_type = hidpp_report->params[HIDPP_PARAM_DEVICE_INFO] &
@@ -883,6 +895,13 @@  static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev,
 		workitem->reports_supported |= STD_KEYBOARD | MULTIMEDIA |
 					       POWER_KEYS | MEDIA_CENTER |
 					       HIDPP;
+		id = (workitem->quad_id_msb << 8) | workitem->quad_id_lsb;
+		for (i = 0; i < ARRAY_SIZE(kbd_builtin_touchpad_ids); i++) {
+			if (id == kbd_builtin_touchpad_ids[i]) {
+				workitem->reports_supported |= STD_MOUSE;
+				break;
+			}
+		}
 		break;
 	case REPORT_TYPE_MOUSE:
 		workitem->reports_supported |= STD_MOUSE | HIDPP;