Re: [RFC] HID: Use SET_REPORT request on control endpoint in hid-rmi for Acer Switch 3 and 5 cover keyboards

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Tobias,

On Thu, Apr 11, 2019 at 7:50 PM Tobias Auerochs <tobi291019@xxxxxxxxx> wrote:
>
> The touchpad on the cover keyboard for the Acer Switch 3 and 5 does not
> work as-is under Linux. Both devices have the same usb id for the cover
> keyboard.
>
> The kernel correctly assigns the hid-rmi driver to the device using usbhid
> for transport.
> Any attempts of hid-rmi to talk to the device using hid_hw_output_report
> fail however as usbhid does not have a working urbout due to the lack of
> any out endpoints.
>
> Looking through Wireshark usbmon recordings from the Windows Synaptics
> driver for this computer running inside of QEMU shows that it should be
> using SET_REPORT requests instead.
>
> The supplied patch replaces the hid_hw_output_report in hid-rmi with a
> hid_hw_raw_request for this device, which is at least enough to enable
> the kernel to get working multi-touch input.
> Any feedback on how to integrate such a fix into the kernel in a more
> preferred way would be much appreciated. (Hence this is sent as an RFC)

First feedback, your commit message is almost perfect, you should just
remove this last paragraph (starting with "Any feedback on...") for
the final submission :)

>
> There are still further issues with the touchpad, such as occasional poor
> responsiveness and the physical buttons being stuck once pressed, however
> those issues do not seem related directly to this particular issue.
>
> The problem has been reported previously in a variety of places:
> https://www.spinics.net/lists/linux-input/msg58433.html
> https://bugzilla.kernel.org/show_bug.cgi?id=95851
> https://ubuntuforums.org/showthread.php?t=2403167
> https://community.acer.com/en/discussion/542762/acer-switch-5-linux-mint-touchpad-not-working
>
> Signed-off-by: Tobias Auerochs <tobi291019@xxxxxxxxx>
> ---
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index b6d93f4ad037..3fc369477d52 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -1083,6 +1083,7 @@
>  #define USB_DEVICE_ID_SYNAPTICS_HD     0x0ac3
>  #define USB_DEVICE_ID_SYNAPTICS_QUAD_HD        0x1ac3
>  #define USB_DEVICE_ID_SYNAPTICS_TP_V103        0x5710
> +#define USB_DEVICE_ID_SYNAPTICS_ACER_SWITCH5   0x81a7
>
>  #define USB_VENDOR_ID_TEXAS_INSTRUMENTS        0x2047
>  #define USB_DEVICE_ID_TEXAS_INSTRUMENTS_LENOVO_YOGA    0x0855
> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> index 953908f2267c..67b75665db37 100644
> --- a/drivers/hid/hid-quirks.c
> +++ b/drivers/hid/hid-quirks.c
> @@ -158,6 +158,7 @@ static const struct hid_device_id hid_quirks[] = {
>         { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_LTS2), HID_QUIRK_NO_INIT_REPORTS },
>         { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_QUAD_HD), HID_QUIRK_NO_INIT_REPORTS },
>         { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_TP_V103), HID_QUIRK_NO_INIT_REPORTS },
> +       { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_ACER_SWITCH5), HID_QUIRK_RMI_OUTPUT_SET_REPORT },

This is a driver specific quirk so it should be defined and set in
hid-rmi.c (under the /* device flags */).

>         { HID_USB_DEVICE(USB_VENDOR_ID_TOPMAX, USB_DEVICE_ID_TOPMAX_COBRAPAD), HID_QUIRK_BADPAD },
>         { HID_USB_DEVICE(USB_VENDOR_ID_TOUCHPACK, USB_DEVICE_ID_TOUCHPACK_RTS), HID_QUIRK_MULTI_INPUT },
>         { HID_USB_DEVICE(USB_VENDOR_ID_TPV, USB_DEVICE_ID_TPV_OPTICAL_TOUCHSCREEN_8882), HID_QUIRK_NOGET },
> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
> index 9e33165250a3..50d7955e91b1 100644
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c
> @@ -169,7 +169,18 @@ static int rmi_write_report(struct hid_device *hdev, u8 *report, int len)
>  {
>         int ret;
>
> -       ret = hid_hw_output_report(hdev, (void *)report, len);
> +       if (hdev->quirks & HID_QUIRK_RMI_OUTPUT_SET_REPORT) {
> +               /*
> +                * Talk to device by using SET_REPORT requests instead.
> +                * The report id is already in report[0],
> +                * so pass that to make usbhid not mess with it.

I'd drop the last sentence as this is how the API of
hid_hw_raw_request() is supposed to be used.

> +                */
> +               ret = hid_hw_raw_request(hdev, report[0], report,
> +                               len, HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
> +       } else {
> +               ret = hid_hw_output_report(hdev, (void *)report, len);
> +       }
> +
>         if (ret < 0) {
>                 dev_err(&hdev->dev, "failed to write hid report (%d)\n", ret);
>                 return ret;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index f9707d1dcb58..b39a32c874f1 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -359,6 +359,7 @@ struct hid_item {
>  #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP BIT(18)
>  #define HID_QUIRK_HAVE_SPECIAL_DRIVER          BIT(19)
>  #define HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE BIT(20)
> +#define HID_QUIRK_RMI_OUTPUT_SET_REPORT                BIT(21)
>  #define HID_QUIRK_FULLSPEED_INTERVAL           BIT(28)
>  #define HID_QUIRK_NO_INIT_REPORTS              BIT(29)
>  #define HID_QUIRK_NO_IGNORE                    BIT(30)
>
>

Summary: not a lot of modifications to do to have your patch accepted.
Please resend it with the changes I mentioned, and we should be able
to push it soon.

Cheers,
Benjamin



[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux