On Friday, October 18, 2019 11:36 AM, Benjamin Tissoires [off-list ref] wrote:

Hi Mazin,

On Mon, Oct 14, 2019 at 8:36 PM Mazin Rezk mnrzk@protonmail.com wrote:

quoted

This patch adds support for the MX Master (b01e and b012) and also adds
foundational code for other Bluetooth LE HID++ devices to be added.
Some devices do not support short reports and thus have a quirk
(HIDPP_QUIRK_MISSING_SHORT_REPORTS) that forces short reports to be sent as
long reports. Since all Bluetooth LE HID++ devices seem to act this way,
HIDPP_QUIRK_CLASS_BLUETOOTH_LE aliases this quirk.
To allow for some space for future quirks, I changed the comment that
defines the bits reserved for classes from 2...20 to 2..15.

Signed-off-by: Mazin Rezk mnrzk@protonmail.com

-----------------------------------------------

drivers/hid/hid-logitech-hidpp.c | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0179f7ed77e5..997b1056850a 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c

@@ -59,7 +59,8 @@ MODULE_PARM_DESC(disable_tap_to_click,

#define HIDPP_QUIRK_CLASS_G920 BIT(3)
#define HIDPP_QUIRK_CLASS_K750 BIT(4)
-/* bits 2..20 are reserved for classes /
+/ bits 2..15 are reserved for classes /
+#define HIDPP_QUIRK_MISSING_SHORT_REPORTS BIT(20)
/ #define HIDPP_QUIRK_CONNECT_EVENTS BIT(21) disabled */#define HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS BIT(22)
#define HIDPP_QUIRK_NO_HIDINPUT BIT(23)

@@ -81,6 +82,8 @@ MODULE_PARM_DESC(disable_tap_to_click,

HIDPP_QUIRK_HI_RES_SCROLL_X2120 | \
HIDPP_QUIRK_HI_RES_SCROLL_X2121)
+#define HIDPP_QUIRK_CLASS_BLUETOOTH_LE HIDPP_QUIRK_MISSING_SHORT_REPORTS
+
#define HIDPP_QUIRK_DELAYED_INIT HIDPP_QUIRK_NO_HIDINPUT
#define HIDPP_CAPABILITY_HIDPP10_BATTERY BIT(0)

@@ -340,6 +343,12 @@ static int hidpp_send_rap_command_sync(struct hidpp_device *hidpp_dev,

struct hidpp_report *message;
int ret, max_count;

-         /* Force long reports on devices that do not support short reports */


-         if (hidpp_dev->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS &&


-             report_id == REPORT_ID_HIDPP_SHORT)


-                 report_id = REPORT_ID_HIDPP_LONG;

Wouldn't it be faster to just store which report needs to be put here
in struct hidpp_device?

quoted

-
-         switch (report_id) {
          case REPORT_ID_HIDPP_SHORT:
                  max_count = HIDPP_REPORT_SHORT_LENGTH - 4;

@@ -3482,6 +3491,12 @@ static bool hidpp_validate_report(struct hid_device *hdev, int id,

static bool hidpp_validate_device(struct hid_device *hdev)
{

-         struct hidpp_device *hidpp = hid_get_drvdata(hdev);


-         /* Skip the short report check if the device does not support it */


-         if (hidpp->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS)


-                 return hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,


-                                              HIDPP_REPORT_LONG_LENGTH, false);


-

I just merged Andrey's report detection, which means you will need to
update this hunk:
https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?h=for-5.4/upstream-fixes&id=905d754c53a522aacf806ea1d3e7c929148c1910

The good thing, is that now you can simply auto-detect if the short
report is missing. If the returned report_length is null, you know
that the report is missing (and thus you can remember to set the
quirk/which report id is needed).

Cheers,
Benjamin

Thank you, I'll try to rework this patch based on the changes introduced in
that commit.

Thanks,
Mazin

quoted

        return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT,
                                     HIDPP_REPORT_SHORT_LENGTH, false) &&
               hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,

@@ -3773,6 +3788,13 @@ 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 Master mouse over Bluetooth */


-           HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb012),


-           .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |


-                          HIDPP_QUIRK_CLASS_BLUETOOTH_LE },


-         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01e),


-           .driver_data = HIDPP_QUIRK_HI_RES_SCROLL_X2121 |


-                          HIDPP_QUIRK_CLASS_BLUETOOTH_LE },
          {}



};
--
2.23.0