* [PATCH v6 2/2] HID: logitech: Support WirelessDeviceStatus connect events
@ 2019-10-14 18:36 Mazin Rezk
2019-10-18 15:38 ` Benjamin Tissoires
0 siblings, 1 reply; 3+ messages in thread
From: Mazin Rezk @ 2019-10-14 18:36 UTC (permalink / raw)
To: linux-input
Cc: benjamin.tissoires, jikos, linux-kernel, Filipe Laíns, mnrzk
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));
}
/**
@@ -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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v6 2/2] HID: logitech: Support WirelessDeviceStatus connect events
2019-10-14 18:36 [PATCH v6 2/2] HID: logitech: Support WirelessDeviceStatus connect events Mazin Rezk
@ 2019-10-18 15:38 ` Benjamin Tissoires
2019-10-19 2:03 ` Mazin Rezk
0 siblings, 1 reply; 3+ messages in thread
From: Benjamin Tissoires @ 2019-10-18 15:38 UTC (permalink / raw)
To: Mazin Rezk; +Cc: linux-input, jikos, linux-kernel, Filipe Laíns
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v6 2/2] HID: logitech: Support WirelessDeviceStatus connect events
2019-10-18 15:38 ` Benjamin Tissoires
@ 2019-10-19 2:03 ` Mazin Rezk
0 siblings, 0 replies; 3+ messages in thread
From: Mazin Rezk @ 2019-10-19 2:03 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: linux-input, jikos, linux-kernel, Filipe Laíns, mnrzk
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-10-19 2:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-14 18:36 [PATCH v6 2/2] HID: logitech: Support WirelessDeviceStatus connect events Mazin Rezk
2019-10-18 15:38 ` Benjamin Tissoires
2019-10-19 2:03 ` Mazin Rezk
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).