On Fri, Jan 31, 2020 at 3:04 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 1/31/20 2:54 PM, Benjamin Tissoires wrote:
> > On Fri, Jan 31, 2020 at 2:41 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>
> >> Hi,
> >>
> >> On 1/31/20 2:10 PM, Benjamin Tissoires wrote:
> >>> 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?
> >>
> >> There is hid_match_one_id() but that is not exported (can be fixed) and it
> >> requires a struct hid_device_id, which either requires declaring an extra
> >> standalone struct hid_device_id for the SW5-012 kbd-dock, or hardcoding an
> >> index into the existing hid_device_id array for the driver (with the hardcoding
> >> being error prone, so not a good idea).
> >>
> >> Given the problems with using hid_match_one_id() I decided to just go with
> >> the above.
> >
> > right. An other solution would be to have a local macro/function that
> > does that. Because as soon as you start adding a quirk, an other comes
> > right after.
> >
> >>
> >> But see below.
> >>
> >>>
> >>>> + 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
> >>
> >> Actually your idea to use the desciptors is not bad, but since what
> >> we really want is to not bind to the interface which is marked for the
> >> hid-multitouch driver I just realized we can just check that.
> >>
> >> So how about:
> >>
> >> static bool ite_match(struct hid_device *hdev, bool ignore_special_driver)
> >> {
> >> if (ignore_special_driver)
> >> return false;
> >>
> >> /*
> >> * Some keyboard docks with an ITE 8595 keyboard/touchpad controller
> >> * support the HID multitouch protocol for the touchpad, in that
> >> * case the "Wireless Radio Control" bits which we care about are
> >> * on the other interface; and we should not bind to the multitouch
> >> * capable interface as that breaks multitouch support.
> >> */
> >> return hdev->group != HID_GROUP_MULTITOUCH_WIN_8;
> >> }
> >
> > Yep, I like that very much :)
>
> Actually if we want to check the group and there are only 2 interfaces we do
> not need to use the match callback at all, w e can simply match on the
> group of the interface which we do want:
>
> diff --git a/drivers/hid/hid-ite.c b/drivers/hid/hid-ite.c
> index db0f35be5a8b..21bd48f16033 100644
> --- a/drivers/hid/hid-ite.c
> +++ b/drivers/hid/hid-ite.c
> @@ -56,8 +56,9 @@ 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) },
> /* ITE8595 USB kbd ctlr, with Synaptics touchpad connected to it. */
> - { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS,
> - USB_DEVICE_ID_SYNAPTICS_ACER_SWITCH5_012) },
> + { HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
> + USB_VENDOR_ID_SYNAPTICS,
> + USB_DEVICE_ID_SYNAPTICS_ACER_SWITCH5_012) },
> { }
> };
> MODULE_DEVICE_TABLE(hid, ite_devices);
>
> Much cleaner
yep
> (and now I don't need to write a test, which is always
> a good motivation to come up with a cleaner solution :)
Hehe, too bad, you already picked up my curiosity on this one, and I
really would like to see the report descriptors and some events of the
keys that are fixed by hid-ite.c.
<with a low voice>This will be a hard requirement to accept this patch </joke>.
More seriously, Zdeněk, can you run hid-recorder from
https://gitlab.freedesktop.org/libevdev/hid-tools/ and provide me the
report descriptor for all of your ITE HID devices? I'll add the
matching tests in hid-tools and be sure we do not regress in the
future.
>
> Let me turn this into a proper patch and then I will send that to
> Zdeněk (off-list) for him to test (note don't worry if you do
> not have time to test this weekend, then I'll do it on Monday).
>
> Regards,
>
> Hans
>
> p.s.
>
> 1. My train is approaching Brussels (Fosdem) so my email response
> time will soon become irregular.
How dare you? :)
>
> 2. Benjamin will you be at Fosdem too ?
>
Unfortunately no. Already got my quota of meeting people for this year
between Kernel Recipes in September, XDC in October and LCA last week.
So I need to keep in a quiet environment for a little bit :)
Cheers,
Benjamin
