diff mbox series

[v6,2/2] HID: logitech: Support WirelessDeviceStatus connect events

Message ID _1Ewv9AvBhbWTNcFOkmvCyjVph73eQIz23Plyv5ffgaWWHnmPBTbSIJhs47AnYatJsmDWu4JlMjcsKE8Cf31lvmwQipYEu47YglNfroyJtM=@protonmail.com (mailing list archive)
State Superseded
Delegated to: Benjamin Tissoires
Headers show
Series [v6,1/2] HID: logitech: Add MX Master over Bluetooth | expand

Commit Message

Mazin Rezk Oct. 14, 2019, 6:36 p.m. UTC 
This patch allows WirelessDeviceStatus (0x1d4b) events to be detected as
connection events in the hid-logitech-hidpp module.

Devices with HIDPP_QUIRK_WIRELESS_DEVICE_STATUS use WirelessDeviceStatus
instead of traditional connect events. Since all Bluetooth LE devices seem
to act this way, HIDPP_QUIRK_CLASS_BLUETOOTH_LE aliases this quirk.

Signed-off-by: Mazin Rezk <mnrzk@protonmail.com>
---
 drivers/hid/hid-logitech-hidpp.c | 42 ++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 5 deletions(-)

--
2.23.0

Comments

Benjamin Tissoires Oct. 18, 2019, 3:38 p.m. UTC | #1 
On Mon, Oct 14, 2019 at 8:36 PM Mazin Rezk <mnrzk@protonmail.com> wrote:
>
> This patch allows WirelessDeviceStatus (0x1d4b) events to be detected as
> connection events in the hid-logitech-hidpp module.
>
> Devices with HIDPP_QUIRK_WIRELESS_DEVICE_STATUS use WirelessDeviceStatus
> instead of traditional connect events. Since all Bluetooth LE devices seem
> to act this way, HIDPP_QUIRK_CLASS_BLUETOOTH_LE aliases this quirk.
>
> Signed-off-by: Mazin Rezk <mnrzk@protonmail.com>
> ---
>  drivers/hid/hid-logitech-hidpp.c | 42 ++++++++++++++++++++++++++++----
>  1 file changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 997b1056850a..9b3df57ca857 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -60,6 +60,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
>  #define HIDPP_QUIRK_CLASS_K750                 BIT(4)
>
>  /* bits 2..15 are reserved for classes */
> +#define HIDPP_QUIRK_WIRELESS_DEVICE_STATUS     BIT(19)
>  #define HIDPP_QUIRK_MISSING_SHORT_REPORTS      BIT(20)
>  /* #define HIDPP_QUIRK_CONNECT_EVENTS          BIT(21) disabled */
>  #define HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS       BIT(22)
> @@ -82,7 +83,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_CLASS_BLUETOOTH_LE (HIDPP_QUIRK_MISSING_SHORT_REPORTS | \
> +                                        HIDPP_QUIRK_WIRELESS_DEVICE_STATUS)
>
>  #define HIDPP_QUIRK_DELAYED_INIT               HIDPP_QUIRK_NO_HIDINPUT
>
> @@ -189,6 +191,8 @@ struct hidpp_device {
>
>         struct hidpp_battery battery;
>         struct hidpp_scroll_counter vertical_wheel_counter;
> +
> +       u8 wireless_feature_index;
>  };
>
>  /* HID++ 1.0 error codes */
> @@ -402,10 +406,14 @@ static inline bool hidpp_match_error(struct hidpp_report *question,
>             (answer->fap.params[0] == question->fap.funcindex_clientid);
>  }
>
> -static inline bool hidpp_report_is_connect_event(struct hidpp_report *report)
> +static inline bool hidpp_report_is_connect_event(struct hidpp_device *hidpp,
> +                                                struct hidpp_report *report)
>  {
> -       return (report->report_id == REPORT_ID_HIDPP_SHORT) &&
> -               (report->rap.sub_id == 0x41);
> +       return ((hidpp->quirks & HIDPP_QUIRK_WIRELESS_DEVICE_STATUS) &&
> +               (report->fap.feature_index == hidpp->wireless_feature_index)) ||
> +             (((report->report_id == REPORT_ID_HIDPP_SHORT) ||
> +               (hidpp->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS)) &&
> +               (report->rap.sub_id == 0x41));

I wonder if we need a quirk after all: if
hidpp->wireless_feature_index is non null, that means that we have the
quirk.
Unless the feature is present for non BLE devices, in which case, we
might want to enable them one by one, for now.

Cheers,
Benjamin

>  }
>
>  /**
> @@ -1282,6 +1290,24 @@ static int hidpp_battery_get_property(struct power_supply *psy,
>         return ret;
>  }
>
> +/* -------------------------------------------------------------------------- */
> +/* 0x1d4b: Wireless device status                                             */
> +/* -------------------------------------------------------------------------- */
> +#define HIDPP_PAGE_WIRELESS_DEVICE_STATUS                      0x1d4b
> +
> +static int hidpp_set_wireless_feature_index(struct hidpp_device *hidpp)
> +{
> +       u8 feature_type;
> +       int ret;
> +
> +       ret = hidpp_root_get_feature(hidpp,
> +                                    HIDPP_PAGE_WIRELESS_DEVICE_STATUS,
> +                                    &hidpp->wireless_feature_index,
> +                                    &feature_type);
> +
> +       return ret;
> +}
> +
>  /* -------------------------------------------------------------------------- */
>  /* 0x2120: Hi-resolution scrolling                                            */
>  /* -------------------------------------------------------------------------- */
> @@ -3077,7 +3103,7 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
>                 }
>         }
>
> -       if (unlikely(hidpp_report_is_connect_event(report))) {
> +       if (unlikely(hidpp_report_is_connect_event(hidpp, report))) {
>                 atomic_set(&hidpp->connected,
>                                 !(report->rap.params[0] & (1 << 6)));
>                 if (schedule_work(&hidpp->work) == 0)
> @@ -3624,6 +3650,12 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                 hidpp_overwrite_name(hdev);
>         }
>
> +       if (connected && (hidpp->quirks & HIDPP_QUIRK_WIRELESS_DEVICE_STATUS)) {
> +               ret = hidpp_set_wireless_feature_index(hidpp);
> +               if (ret)
> +                       goto hid_hw_init_fail;
> +       }
> +
>         if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
>                 ret = wtp_get_config(hidpp);
>                 if (ret)
> --
> 2.23.0
>
Mazin Rezk Oct. 19, 2019, 2:03 a.m. UTC | #2 
On Friday, October 18, 2019 11:38 AM, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:

> On Mon, Oct 14, 2019 at 8:36 PM Mazin Rezk mnrzk@protonmail.com wrote:
>
> > This patch allows WirelessDeviceStatus (0x1d4b) events to be detected as
> > connection events in the hid-logitech-hidpp module.
> > Devices with HIDPP_QUIRK_WIRELESS_DEVICE_STATUS use WirelessDeviceStatus
> > instead of traditional connect events. Since all Bluetooth LE devices seem
> > to act this way, HIDPP_QUIRK_CLASS_BLUETOOTH_LE aliases this quirk.
> >
> > Signed-off-by: Mazin Rezk mnrzk@protonmail.com
> >
> > -----------------------------------------------
> >
> > drivers/hid/hid-logitech-hidpp.c | 42 ++++++++++++++++++++++++++++----
> > 1 file changed, 37 insertions(+), 5 deletions(-)
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 997b1056850a..9b3df57ca857 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -60,6 +60,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
> > #define HIDPP_QUIRK_CLASS_K750 BIT(4)
> > /* bits 2..15 are reserved for classes /
> > +#define HIDPP_QUIRK_WIRELESS_DEVICE_STATUS BIT(19)
> > #define HIDPP_QUIRK_MISSING_SHORT_REPORTS BIT(20)
> > / #define HIDPP_QUIRK_CONNECT_EVENTS BIT(21) disabled */#define HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS BIT(22)
> > @@ -82,7 +83,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_CLASS_BLUETOOTH_LE (HIDPP_QUIRK_MISSING_SHORT_REPORTS | \
> >
> > -                                          HIDPP_QUIRK_WIRELESS_DEVICE_STATUS)
> >
> >
> >
> > #define HIDPP_QUIRK_DELAYED_INIT HIDPP_QUIRK_NO_HIDINPUT
> > @@ -189,6 +191,8 @@ struct hidpp_device {
> >
> >         struct hidpp_battery battery;
> >         struct hidpp_scroll_counter vertical_wheel_counter;
> >
> >
> > -
> > -         u8 wireless_feature_index;
> >
> >
> >
> > };
> > /* HID++ 1.0 error codes */
> > @@ -402,10 +406,14 @@ static inline bool hidpp_match_error(struct hidpp_report *question,
> > (answer->fap.params[0] == question->fap.funcindex_clientid);
> > }
> > -static inline bool hidpp_report_is_connect_event(struct hidpp_report *report)
> > +static inline bool hidpp_report_is_connect_event(struct hidpp_device *hidpp,
> >
> > -                                                  struct hidpp_report *report)
> >
> >
> >
> > {
> >
> > -         return (report->report_id == REPORT_ID_HIDPP_SHORT) &&
> >
> >
> > -                 (report->rap.sub_id == 0x41);
> >
> >
> >
> > -         return ((hidpp->quirks & HIDPP_QUIRK_WIRELESS_DEVICE_STATUS) &&
> >
> >
> > -                 (report->fap.feature_index == hidpp->wireless_feature_index)) ||
> >
> >
> > -               (((report->report_id == REPORT_ID_HIDPP_SHORT) ||
> >
> >
> > -                 (hidpp->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS)) &&
> >
> >
> > -                 (report->rap.sub_id == 0x41));
> >
> >
>
> I wonder if we need a quirk after all: if
> hidpp->wireless_feature_index is non null, that means that we have the
> quirk.
> Unless the feature is present for non BLE devices, in which case, we
> might want to enable them one by one, for now.
>
> Cheers,
> Benjamin

Come to think of it, it does seem redundant. I'll likely extend the
WirelessDeviceStatus check to all HID++ 2.0 devices and just forgo the
added quirks entirely.

Thanks,
Mazin

>
> > }
> > /**
> > @@ -1282,6 +1290,24 @@ static int hidpp_battery_get_property(struct power_supply *psy,
> > return ret;
> > }
> > +/* -------------------------------------------------------------------------- /
> > +/ 0x1d4b: Wireless device status /
> > +/ -------------------------------------------------------------------------- */+#define HIDPP_PAGE_WIRELESS_DEVICE_STATUS 0x1d4b
> > +
> > +static int hidpp_set_wireless_feature_index(struct hidpp_device *hidpp)
> > +{
> >
> > -         u8 feature_type;
> >
> >
> > -         int ret;
> >
> >
> > -
> > -         ret = hidpp_root_get_feature(hidpp,
> >
> >
> > -                                      HIDPP_PAGE_WIRELESS_DEVICE_STATUS,
> >
> >
> > -                                      &hidpp->wireless_feature_index,
> >
> >
> > -                                      &feature_type);
> >
> >
> > -
> > -         return ret;
> >
> >
> >
> > +}
> > +
> > /* -------------------------------------------------------------------------- /
> > / 0x2120: Hi-resolution scrolling /
> > / -------------------------------------------------------------------------- */@@ -3077,7 +3103,7 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
> > }
> > }
> >
> > -         if (unlikely(hidpp_report_is_connect_event(report))) {
> >
> >
> >
> > -         if (unlikely(hidpp_report_is_connect_event(hidpp, report))) {
> >                   atomic_set(&hidpp->connected,
> >                                   !(report->rap.params[0] & (1 << 6)));
> >                   if (schedule_work(&hidpp->work) == 0)
> >
> >
> >
> > @@ -3624,6 +3650,12 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > hidpp_overwrite_name(hdev);
> > }
> >
> > -         if (connected && (hidpp->quirks & HIDPP_QUIRK_WIRELESS_DEVICE_STATUS)) {
> >
> >
> > -                 ret = hidpp_set_wireless_feature_index(hidpp);
> >
> >
> > -                 if (ret)
> >
> >
> > -                         goto hid_hw_init_fail;
> >
> >
> > -         }
> >
> >
> > -         if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
> >                   ret = wtp_get_config(hidpp);
> >                   if (ret)
> >
> >
> >
> > --
> > 2.23.0
diff mbox series

Patch

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 997b1056850a..9b3df57ca857 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -60,6 +60,7 @@  MODULE_PARM_DESC(disable_tap_to_click,
 #define HIDPP_QUIRK_CLASS_K750			BIT(4)

 /* bits 2..15 are reserved for classes */
+#define HIDPP_QUIRK_WIRELESS_DEVICE_STATUS	BIT(19)
 #define HIDPP_QUIRK_MISSING_SHORT_REPORTS	BIT(20)
 /* #define HIDPP_QUIRK_CONNECT_EVENTS		BIT(21) disabled */
 #define HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS	BIT(22)
@@ -82,7 +83,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_CLASS_BLUETOOTH_LE	(HIDPP_QUIRK_MISSING_SHORT_REPORTS | \
+					 HIDPP_QUIRK_WIRELESS_DEVICE_STATUS)

 #define HIDPP_QUIRK_DELAYED_INIT		HIDPP_QUIRK_NO_HIDINPUT

@@ -189,6 +191,8 @@  struct hidpp_device {

 	struct hidpp_battery battery;
 	struct hidpp_scroll_counter vertical_wheel_counter;
+
+	u8 wireless_feature_index;
 };

 /* HID++ 1.0 error codes */
@@ -402,10 +406,14 @@  static inline bool hidpp_match_error(struct hidpp_report *question,
 	    (answer->fap.params[0] == question->fap.funcindex_clientid);
 }

-static inline bool hidpp_report_is_connect_event(struct hidpp_report *report)
+static inline bool hidpp_report_is_connect_event(struct hidpp_device *hidpp,
+						 struct hidpp_report *report)
 {
-	return (report->report_id == REPORT_ID_HIDPP_SHORT) &&
-		(report->rap.sub_id == 0x41);
+	return ((hidpp->quirks & HIDPP_QUIRK_WIRELESS_DEVICE_STATUS) &&
+		(report->fap.feature_index == hidpp->wireless_feature_index)) ||
+	      (((report->report_id == REPORT_ID_HIDPP_SHORT) ||
+		(hidpp->quirks & HIDPP_QUIRK_MISSING_SHORT_REPORTS)) &&
+		(report->rap.sub_id == 0x41));
 }

 /**
@@ -1282,6 +1290,24 @@  static int hidpp_battery_get_property(struct power_supply *psy,
 	return ret;
 }

+/* -------------------------------------------------------------------------- */
+/* 0x1d4b: Wireless device status                                             */
+/* -------------------------------------------------------------------------- */
+#define HIDPP_PAGE_WIRELESS_DEVICE_STATUS			0x1d4b
+
+static int hidpp_set_wireless_feature_index(struct hidpp_device *hidpp)
+{
+	u8 feature_type;
+	int ret;
+
+	ret = hidpp_root_get_feature(hidpp,
+				     HIDPP_PAGE_WIRELESS_DEVICE_STATUS,
+				     &hidpp->wireless_feature_index,
+				     &feature_type);
+
+	return ret;
+}
+
 /* -------------------------------------------------------------------------- */
 /* 0x2120: Hi-resolution scrolling                                            */
 /* -------------------------------------------------------------------------- */
@@ -3077,7 +3103,7 @@  static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
 		}
 	}

-	if (unlikely(hidpp_report_is_connect_event(report))) {
+	if (unlikely(hidpp_report_is_connect_event(hidpp, report))) {
 		atomic_set(&hidpp->connected,
 				!(report->rap.params[0] & (1 << 6)));
 		if (schedule_work(&hidpp->work) == 0)
@@ -3624,6 +3650,12 @@  static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		hidpp_overwrite_name(hdev);
 	}

+	if (connected && (hidpp->quirks & HIDPP_QUIRK_WIRELESS_DEVICE_STATUS)) {
+		ret = hidpp_set_wireless_feature_index(hidpp);
+		if (ret)
+			goto hid_hw_init_fail;
+	}
+
 	if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
 		ret = wtp_get_config(hidpp);
 		if (ret)