From: Hans de Goede <hdegoede@redhat.com>
To: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Jiri Kosina <jikos@kernel.org>,
"open list:HID CORE LAYER" <linux-input@vger.kernel.org>
Subject: Re: [RFC 3/3] HID: logitech-dj: Handle newer quad/bt2.0 receivers in HID proxy mode
Date: Mon, 16 Nov 2020 09:43:13 +0100 [thread overview]
Message-ID: <ac6e7134-9c26-d1c3-6905-00415928890a@redhat.com> (raw)
In-Reply-To: <CAO-hwJKBHvheKWRKXbKH=0Lgy5v_7-HQeCrJijrPAvNLCZtRdQ@mail.gmail.com>
Hi,
On 11/16/20 9:30 AM, Benjamin Tissoires wrote:
> Hi Hans,
>
> Not full review but...
>
> On Sat, Nov 14, 2020 at 10:21 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> The Dinovo Edge and Dinovo Mini keyboards with builtin touchpad come with
>> a different version of the quad/bt2.0 combo receivers shipped with the
>> MX5000 and MX5500 keyboards. These receivers are compatible with one
>> another, e.g. the Dinovo Edge keyboard can be paired with the MX5000
>> receiver.
>>
>> Like the MX5x00 receivers in HID proxy mode these receivers present
>> themselves as a hub with multiple USB-HID devices, one for the keyboard
>> and one for the mouse.
>>
>> Where they differ is that the mouse USB-device has 2 input reports for
>> reporting mice events. It has the exact same INPUT(2) report as the
>> MX5x00 receivers, but it also has a second INPUT(5) mouse report which
>> is different; and when the Dinovo receivers are paired with the Dinovo
>> keyboards the second INPUT(5) mouse report is actually used for events
>> on the builtin touchpad.
>>
>> Add support for handling the Dinovo quad/bluetooth-2.0 combo receivers
>> in HID proxy mode to logitech-dj, like we already do for the similar
>> MX5000 and MX5500 receivers.
>>
>> This adds battery monitoring functionality (through logitech-hidpp) and
>> fixes the Phone (Fn + F1) and "[A]" - "[D]" (Fn + F9 - F12) hotkeys not
>> working on the Dinovo Edge.
>>
>> Note these receivers present themselves as a hub with 2 separate USB
>> devices for the keyboard and mouse; and the logitech-dj code needs to
>> bind to both devices (just as with the MX5x00 receivers).
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> drivers/hid/hid-ids.h | 6 ++-
>> drivers/hid/hid-lg.c | 24 ---------
>> drivers/hid/hid-logitech-dj.c | 91 +++++++++++++++++++++++++++++++++--
>> drivers/hid/hid-quirks.c | 2 -
>> 4 files changed, 92 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>> index 108081ab5ae3..e17f252ba60f 100644
>> --- a/drivers/hid/hid-ids.h
>> +++ b/drivers/hid/hid-ids.h
>> @@ -797,12 +797,14 @@
>> #define USB_DEVICE_ID_SPACETRAVELLER 0xc623
>> #define USB_DEVICE_ID_SPACENAVIGATOR 0xc626
>> #define USB_DEVICE_ID_DINOVO_DESKTOP 0xc704
>> -#define USB_DEVICE_ID_DINOVO_EDGE 0xc714
>> -#define USB_DEVICE_ID_DINOVO_MINI 0xc71f
>> #define USB_DEVICE_ID_MX5000_RECEIVER_MOUSE_DEV 0xc70a
>> #define USB_DEVICE_ID_MX5000_RECEIVER_KBD_DEV 0xc70e
>> +#define USB_DEVICE_ID_DINOVO_EDGE_RECEIVER_KBD_DEV 0xc713
>> +#define USB_DEVICE_ID_DINOVO_EDGE_RECEIVER_MOUSE_DEV 0xc714
>> #define USB_DEVICE_ID_MX5500_RECEIVER_KBD_DEV 0xc71b
>> #define USB_DEVICE_ID_MX5500_RECEIVER_MOUSE_DEV 0xc71c
>> +#define USB_DEVICE_ID_DINOVO_MINI_RECEIVER_KBD_DEV 0xc71e
>> +#define USB_DEVICE_ID_DINOVO_MINI_RECEIVER_MOUSE_DEV 0xc71f
>> #define USB_DEVICE_ID_LOGITECH_MOMO_WHEEL2 0xca03
>> #define USB_DEVICE_ID_LOGITECH_VIBRATION_WHEEL 0xca04
>>
>> diff --git a/drivers/hid/hid-lg.c b/drivers/hid/hid-lg.c
>> index 0dc7cdfc56f7..d40af911df63 100644
>> --- a/drivers/hid/hid-lg.c
>> +++ b/drivers/hid/hid-lg.c
>> @@ -568,22 +568,6 @@ static int lg_ultrax_remote_mapping(struct hid_input *hi,
>> return 1;
>> }
>>
>> -static int lg_dinovo_mapping(struct hid_input *hi, struct hid_usage *usage,
>> - unsigned long **bit, int *max)
>> -{
>> - if ((usage->hid & HID_USAGE_PAGE) != HID_UP_LOGIVENDOR)
>> - return 0;
>> -
>> - switch (usage->hid & HID_USAGE) {
>> -
>> - case 0x00d: lg_map_key_clear(KEY_MEDIA); break;
>> - default:
>> - return 0;
>> -
>> - }
>> - return 1;
>> -}
>> -
>
> I am not sure these changes on hid-logitech-hidpp.c
This is not a change to hid-logitech-hidpp.c, it it a change to hid-lg.c,
which after this patch will no longer handle the Dinovo Mini ever, since its
USB-ID is dropped from its id-table. Also note that the above code was
only run on an USB-ID match, so with the USB-ID dropped the hid-lg.c
copy of this is dead code (also see below).
> are something you
> wanted to put in. Looks like the patch reverts the Dinovo Mini in 1/3.
1/3 actually duplicates this bit from hid-lg.c to hid-logitech-hidpp.c
so that the Dinovo Mino Media key will work properly independent of it
being paired with a receiver handled by hid-lg.c or hid-logitech-dj.c.
This patch moves all receiver handling over to hid-logitech-dj.c, after
which the Dinovo Mini handling in hid-lg.c is just dead code.
I did things this way (with the temporary code duplication) so that 1/3
can go out as a fix for stable kernels while 2 + 3 are for 5.11.
Regards,
Hans
>> static int lg_wireless_mapping(struct hid_input *hi, struct hid_usage *usage,
>> unsigned long **bit, int *max)
>> {
>> @@ -668,10 +652,6 @@ static int lg_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>> lg_ultrax_remote_mapping(hi, usage, bit, max))
>> return 1;
>>
>> - if (hdev->product == USB_DEVICE_ID_DINOVO_MINI &&
>> - lg_dinovo_mapping(hi, usage, bit, max))
>> - return 1;
>> -
>> if ((drv_data->quirks & LG_WIRELESS) && lg_wireless_mapping(hi, usage, bit, max))
>> return 1;
>>
>> @@ -879,10 +859,6 @@ static const struct hid_device_id lg_devices[] = {
>>
>> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_DINOVO_DESKTOP),
>> .driver_data = LG_DUPLICATE_USAGES },
>> - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_DINOVO_EDGE),
>> - .driver_data = LG_DUPLICATE_USAGES },
>> - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_DINOVO_MINI),
>> - .driver_data = LG_DUPLICATE_USAGES },
>>
>> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_ELITE_KBD),
>> .driver_data = LG_IGNORE_DOUBLED_WHEEL | LG_EXPANDED_KEYMAP },
>> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
>> index eaaedbb84a8d..e9e294b66e59 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 KBD_MOUSE BIT(5)
>> #define MEDIA_CENTER BIT(8)
>> #define KBD_LEDS BIT(14)
>> /* Fake (bitnr > NUMBER_OF_HID_REPORTS) bit to track HID++ capability */
>> @@ -118,6 +119,7 @@ enum recvr_type {
>> recvr_type_mouse_only,
>> recvr_type_27mhz,
>> recvr_type_bluetooth,
>> + recvr_type_dinovo,
>> };
>>
>> struct dj_report {
>> @@ -334,6 +336,47 @@ static const char mse_bluetooth_descriptor[] = {
>> 0xC0, /* END_COLLECTION */
>> };
>>
>> +/* Mouse descriptor (5) for Bluetooth receiver, normal-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) */
>> @@ -481,6 +524,7 @@ static const char hidpp_descriptor[] = {
>> #define MAX_RDESC_SIZE \
>> (sizeof(kbd_descriptor) + \
>> sizeof(mse_bluetooth_descriptor) + \
>> + sizeof(mse5_bluetooth_descriptor) + \
>> sizeof(consumer_descriptor) + \
>> sizeof(syscontrol_descriptor) + \
>> sizeof(media_descriptor) + \
>> @@ -518,6 +562,11 @@ static void delayedwork_callback(struct work_struct *work);
>> static LIST_HEAD(dj_hdev_list);
>> static DEFINE_MUTEX(dj_hdev_list_lock);
>>
>> +static bool recvr_type_is_bluetooth(enum recvr_type type)
>> +{
>> + return type == recvr_type_bluetooth || type == recvr_type_dinovo;
>> +}
>> +
>> /*
>> * dj/HID++ receivers are really a single logical entity, but for BIOS/Windows
>> * compatibility they have multiple USB interfaces. On HID++ receivers we need
>> @@ -535,7 +584,7 @@ static struct dj_receiver_dev *dj_find_receiver_dev(struct hid_device *hdev,
>> * The bluetooth receiver contains a built-in hub and has separate
>> * USB-devices for the keyboard and mouse interfaces.
>> */
>> - sep = (type == recvr_type_bluetooth) ? '.' : '/';
>> + sep = recvr_type_is_bluetooth(type) ? '.' : '/';
>>
>> /* Try to find an already-probed interface from the same device */
>> list_for_each_entry(djrcv_dev, &dj_hdev_list, list) {
>> @@ -873,6 +922,14 @@ static void logi_dj_recv_queue_notification(struct dj_receiver_dev *djrcv_dev,
>> * 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).
>> + *
>> + * On Dinovo receivers the keyboard's touchpad and an optional paired actual
>> + * mouse send separate input reports, INPUT(2) aka STD_MOUSE for the mouse
>> + * and INPUT(5) aka KBD_MOUSE for the keyboard's touchpad.
>> + *
>> + * On MX5x00 receivers (which can also be paired with a Dinovo keyboard)
>> + * INPUT(2) is used for both an optional paired actual mouse and for the
>> + * keyboard's touchpad.
>> */
>> static const u16 kbd_builtin_touchpad_ids[] = {
>> 0xb309, /* Dinovo Edge */
>> @@ -899,7 +956,10 @@ static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev,
>> 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;
>> + if (djrcv_dev->type == recvr_type_dinovo)
>> + workitem->reports_supported |= KBD_MOUSE;
>> + else
>> + workitem->reports_supported |= STD_MOUSE;
>> break;
>> }
>> }
>> @@ -1367,7 +1427,7 @@ static int logi_dj_ll_parse(struct hid_device *hid)
>> 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)
>> + else if (recvr_type_is_bluetooth(djdev->dj_receiver_dev->type))
>> rdcat(rdesc, &rsize, mse_bluetooth_descriptor,
>> sizeof(mse_bluetooth_descriptor));
>> else
>> @@ -1375,6 +1435,13 @@ static int logi_dj_ll_parse(struct hid_device *hid)
>> sizeof(mse_descriptor));
>> }
>>
>> + if (djdev->reports_supported & KBD_MOUSE) {
>> + dbg_hid("%s: sending a kbd-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);
>> @@ -1692,6 +1759,7 @@ static int logi_dj_probe(struct hid_device *hdev,
>> case recvr_type_mouse_only: no_dj_interfaces = 2; break;
>> case recvr_type_27mhz: no_dj_interfaces = 2; break;
>> case recvr_type_bluetooth: no_dj_interfaces = 2; break;
>> + case recvr_type_dinovo: no_dj_interfaces = 2; break;
>> }
>> if (hid_is_using_ll_driver(hdev, &usb_hid_driver)) {
>> intf = to_usb_interface(hdev->dev.parent);
>> @@ -1920,6 +1988,23 @@ static const struct hid_device_id logi_dj_receivers[] = {
>> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
>> USB_DEVICE_ID_MX5500_RECEIVER_MOUSE_DEV),
>> .driver_data = recvr_type_bluetooth},
>> +
>> + { /* Logitech Dinovo Edge HID++ / bluetooth receiver keyboard intf. (0xc713) */
>> + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
>> + USB_DEVICE_ID_DINOVO_EDGE_RECEIVER_KBD_DEV),
>> + .driver_data = recvr_type_dinovo},
>> + { /* Logitech Dinovo Edge HID++ / bluetooth receiver mouse intf. (0xc714) */
>> + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
>> + USB_DEVICE_ID_DINOVO_EDGE_RECEIVER_MOUSE_DEV),
>> + .driver_data = recvr_type_dinovo},
>> + { /* Logitech DiNovo Mini HID++ / bluetooth receiver mouse intf. (0xc71e) */
>> + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
>> + USB_DEVICE_ID_DINOVO_MINI_RECEIVER_KBD_DEV),
>> + .driver_data = recvr_type_dinovo},
>> + { /* Logitech DiNovo Mini HID++ / bluetooth receiver keyboard intf. (0xc71f) */
>> + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
>> + USB_DEVICE_ID_DINOVO_MINI_RECEIVER_MOUSE_DEV),
>> + .driver_data = recvr_type_dinovo},
>> {}
>> };
>>
>> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
>> index 7a2be0205dfd..8fce238f120d 100644
>> --- a/drivers/hid/hid-quirks.c
>> +++ b/drivers/hid/hid-quirks.c
>> @@ -439,8 +439,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
>> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RECEIVER) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_DINOVO_DESKTOP) },
>> - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_DINOVO_EDGE) },
>> - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_DINOVO_MINI) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_ELITE_KBD) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_CORDLESS_DESKTOP_LX500) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_EXTREME_3D) },
>> --
>> 2.28.0
>>
>
next prev parent reply other threads:[~2020-11-16 9:08 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-14 21:20 [RFC 0/3] HID: logitech-dj: Dinovo keyboard fixes and improvements Hans de Goede
2020-11-14 21:20 ` [RFC 1/3] HID: logitech-dj: Fix Dinovo Mini when paired with a MX5x00 receiver Hans de Goede
2020-11-14 21:20 ` [RFC 2/3] HID: logitech-dj: Use hid-ids.h defines for USB device-ids for all supported devices Hans de Goede
2020-11-14 21:20 ` [RFC 3/3] HID: logitech-dj: Handle newer quad/bt2.0 receivers in HID proxy mode Hans de Goede
2020-11-16 8:30 ` Benjamin Tissoires
2020-11-16 8:43 ` Hans de Goede [this message]
2020-11-19 15:25 ` [RFC 0/3] HID: logitech-dj: Dinovo keyboard fixes and improvements Benjamin Tissoires
2020-11-19 15:47 ` Hans de Goede
2020-11-19 15:52 ` Benjamin Tissoires
2020-11-19 15:54 ` Hans de Goede
2020-12-07 10:32 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ac6e7134-9c26-d1c3-6905-00415928890a@redhat.com \
--to=hdegoede@redhat.com \
--cc=benjamin.tissoires@redhat.com \
--cc=jikos@kernel.org \
--cc=linux-input@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.