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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
>> > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1811424
>> > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>>
>> 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
> 




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux