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
