Re: [PATCH] HID: ite: Only bind to keyboard USB interface on Acer SW5-012 keyboard dock

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

 



Hi Hans,

On Fri, Jan 31, 2020 at 1:46 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Commit 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard
> dock") added the USB id for the Acer SW5-012's keyboard dock to the
> hid-ite driver to fix the rfkill driver not working.
>
> Most keyboard docks with an ITE 8595 keyboard/touchpad controller have the
> "Wireless Radio Control" bits which need the special hid-ite driver on the
> second USB interface (the mouse interface) and their touchpad only supports
> mouse emulation, so using generic hid-input handling for anything but
> the "Wireless Radio Control" bits is fine. On these devices we simply bind
> to all USB interfaces.
>
> But unlike other ITE8595 using keyboard docks, the Acer Aspire Switch 10
> (SW5-012)'s touchpad not only does mouse emulation it also supports
> HID-multitouch and all the keys including the "Wireless Radio Control"
> bits have been moved to the first USB interface (the keyboard intf).
>
> So we need hid-ite to handle the first (keyboard) USB interface and have
> it NOT bind to the second (mouse) USB interface so that that can be
> handled by hid-multitouch.c and we get proper multi-touch support.
>
> This commit adds a match callback to hid-ite which makes it only
> match the first USB interface when running on the Acer SW5-012,
> fixing the regression to mouse-emulation mode introduced by adding the
> keyboard dock USB id.
>
> Note the match function only does the special only bind to the first
> USB interface on the Acer SW5-012, on other devices the hid-ite driver
> actually must bind to the second interface as that is where the
> "Wireless Radio Control" bits are.

This is not a full review, but a couple of things that popped out
while scrolling through the patch.

>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard dock")
> Reported-by: Zdeněk Rampas <zdenda.rampas@xxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
>  drivers/hid/hid-ite.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/drivers/hid/hid-ite.c b/drivers/hid/hid-ite.c
> index c436e12feb23..69a4ddfd033d 100644
> --- a/drivers/hid/hid-ite.c
> +++ b/drivers/hid/hid-ite.c
> @@ -8,9 +8,12 @@
>  #include <linux/input.h>
>  #include <linux/hid.h>
>  #include <linux/module.h>
> +#include <linux/usb.h>
>
>  #include "hid-ids.h"
>
> +#define ITE8595_KBD_USB_INTF           0
> +
>  static int ite_event(struct hid_device *hdev, struct hid_field *field,
>                      struct hid_usage *usage, __s32 value)
>  {
> @@ -37,6 +40,36 @@ static int ite_event(struct hid_device *hdev, struct hid_field *field,
>         return 0;
>  }
>
> +static bool ite_match(struct hid_device *hdev, bool ignore_special_driver)
> +{
> +       struct usb_interface *intf;
> +
> +       if (ignore_special_driver)
> +               return false;
> +
> +       /*
> +        * Most keyboard docks with an ITE 8595 keyboard/touchpad controller
> +        * have the "Wireless Radio Control" bits which need this special
> +        * driver on the second USB interface (the mouse interface). On
> +        * these devices we simply bind to all USB interfaces.
> +        *
> +        * The Acer Aspire Switch 10 (SW5-012) is special, its touchpad
> +        * not only does mouse emulation it also supports HID-multitouch
> +        * and all the keys including the "Wireless Radio Control" bits
> +        * have been moved to the first USB interface (the keyboard intf).
> +        *
> +        * We want the hid-multitouch driver to bind to the touchpad, so on
> +        * the Acer SW5-012 we should only bind to the keyboard USB intf.
> +        */
> +       if (hdev->bus != BUS_USB || hdev->vendor != USB_VENDOR_ID_SYNAPTICS ||
> +                    hdev->product != USB_DEVICE_ID_SYNAPTICS_ACER_SWITCH5_012)

Isn't there an existing matching function we can use here, instead of
checking each individual field?

> +               return true;
> +
> +       intf = to_usb_interface(hdev->dev.parent);

And this is oops-prone. You need:
- ensure hid_is_using_ll_driver(hdev, &usb_hid_driver) returns true.
- add a dependency on USBHID in the KConfig now that you are checking
on the USB transport layer.

That being said, I would love instead:
- to have a non USB version of this match, where you decide which
component needs to be handled based on the report descriptor
- have a regression test in
https://gitlab.freedesktop.org/libevdev/hid-tools for this particular
device, because I never intended the .match callback to be used by
anybody else than hid-generic, and opening this can of worms is prone
to introduce regressions in the future.

easy, no? :)

Cheers,
Benjamin

> +
> +       return intf->altsetting->desc.bInterfaceNumber == ITE8595_KBD_USB_INTF;
> +}
> +
>  static const struct hid_device_id ite_devices[] = {
>         { HID_USB_DEVICE(USB_VENDOR_ID_ITE, USB_DEVICE_ID_ITE8595) },
>         { HID_USB_DEVICE(USB_VENDOR_ID_258A, USB_DEVICE_ID_258A_6A88) },
> @@ -50,6 +83,7 @@ MODULE_DEVICE_TABLE(hid, ite_devices);
>  static struct hid_driver ite_driver = {
>         .name = "itetech",
>         .id_table = ite_devices,
> +       .match = ite_match,
>         .event = ite_event,
>  };
>  module_hid_driver(ite_driver);
> --
> 2.23.0
>





[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux