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

[Benjamin Tissoires]

On Mon, Oct 14, 2019 at 8:36 PM Mazin Rezk <mnrzk@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> ---
>  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
>