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

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

[Hans de Goede]

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.

Cc: stable@vger.kernel.org
Fixes: 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard dock")
Reported-by: Zdeněk Rampas <zdenda.rampas@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 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)
+		return true;
+
+	intf = to_usb_interface(hdev->dev.parent);
+
+	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

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

[Benjamin Tissoires]

Hi Hans,

On Fri, Jan 31, 2020 at 1:46 PM Hans de Goede <hdegoede@redhat.com> 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@vger.kernel.org
> Fixes: 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard dock")
> Reported-by: Zdeněk Rampas <zdenda.rampas@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  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
>

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

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 6634 bytes --]

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@redhat.com> 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@vger.kernel.org
>> Fixes: 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard dock")
>> Reported-by: Zdeněk Rampas <zdenda.rampas@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   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.

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;
}

? (note untested)

Zdeněk  I have attached a new version of the patch which uses this
improved version of the match function, if you have a chance to test it
this weekend that would be great, otherwise I will test it on my own
sw5-012 on Monday.

> - 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.

Ugh, I can understand your desire for a test for this, but writing
tests is not really my thing. Anyways do you have an example test I
could use as a start ?

Regards,

Hans

[-- Attachment #2: 0001-HID-ite-Only-bind-to-keyboard-USB-interface-on-Acer-.patch --]
[-- Type: text/x-patch, Size: 3503 bytes --]

From f26ab52abab63d819c49db0460d2392cce1da733 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Fri, 31 Jan 2020 12:06:13 +0100
Subject: [PATCH v2] HID: ite: Only bind to keyboard USB interface on Acer
 SW5-012 keyboard dock
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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 not bind
to hid-devices which are marked as being win8 multi-touch devices, this
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.

Cc: stable@vger.kernel.org
Fixes: 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard dock")
Reported-by: Zdeněk Rampas <zdenda.rampas@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Check hdev->group instead of peeking at the USB descriptors (intf number)
---
 drivers/hid/hid-ite.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/hid/hid-ite.c b/drivers/hid/hid-ite.c
index c436e12feb23..db0f35be5a8b 100644
--- a/drivers/hid/hid-ite.c
+++ b/drivers/hid/hid-ite.c
@@ -37,6 +37,21 @@ 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)
+{
+	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;
+}
+
 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 +65,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

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

[Benjamin Tissoires]

On Fri, Jan 31, 2020 at 2:41 PM Hans de Goede <hdegoede@redhat.com> 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@redhat.com> 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@vger.kernel.org
> >> Fixes: 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard dock")
> >> Reported-by: Zdeněk Rampas <zdenda.rampas@gmail.com>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>   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 :)

>
> ? (note untested)
>
> Zdeněk  I have attached a new version of the patch which uses this
> improved version of the match function, if you have a chance to test it
> this weekend that would be great, otherwise I will test it on my own
> sw5-012 on Monday.
>
> > - 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.
>
> Ugh, I can understand your desire for a test for this, but writing
> tests is not really my thing. Anyways do you have an example test I
> could use as a start ?

So, if I am not wrong:
- keyboard docks with an ITE 8595 keyboard/touchpad controller are
registered in hid-ite.c, but should be really bound to hid-multitouch.
- there are at least 2 HID interfaces (sharing the same VID/PID), one
for the keyboard (hid-ite.c), and one for the touchpad
(hid-multitouch.c)
- what matters in the end, is that the multitouch interface gets
correctly mapped and handled.

So if that is somehow accurate, I think a test would be a matter of:
- duplicating https://gitlab.freedesktop.org/libevdev/hid-tools/blob/master/tests/test_multitouch.py#L1568-1570,
- add the report descriptors of the touchpad interface,
- and check that the unpatched kernel picks up hid-ite.c, which leads
to errors in the multitouch tests
- ideally, we should also add a test for the expected hid-ite.c part,
but I can do that later with the report descriptors.

So initially, it shouldn't be more than a 3 lines patch (hopefully).
Actually 4, because you also need to force the VID/PID with
`info=(0x3, 0xVID, 0xPID)` (see TestZytronic_14c8_0005 for an example,
in the same file)

Cheers,
Benjamin

>
> Regards,
>
> Hans

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

[Hans de Goede]

Hi,

On 1/31/20 2:54 PM, Benjamin Tissoires wrote:
> On Fri, Jan 31, 2020 at 2:41 PM Hans de Goede <hdegoede@redhat.com> 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@redhat.com> 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@vger.kernel.org
>>>> Fixes: 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard dock")
>>>> Reported-by: Zdeněk Rampas <zdenda.rampas@gmail.com>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>    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 (and now I don't need to write a test, which is always
a good motivation to come up with a cleaner solution :)

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.

2. Benjamin will you be at Fosdem too ?

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

[Benjamin Tissoires]

On Fri, Jan 31, 2020 at 3:04 PM Hans de Goede <hdegoede@redhat.com> 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@redhat.com> 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@redhat.com> 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@vger.kernel.org
> >>>> Fixes: 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard dock")
> >>>> Reported-by: Zdeněk Rampas <zdenda.rampas@gmail.com>
> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>> ---
> >>>>    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

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

[Hans de Goede]

Hi,

On 1/31/20 4:38 PM, Z R wrote:
> Hi Benjamin,
> hid-record for keyboard and touchpad. With Commit 8f18eca9ebc5 reverted and from unmodified kernel.
> 
> I hope it is what you asked for :-)
> 
> Currently waiting for reworked patch from Hans.

I just send you the reworked patch.

Does the recordning include pressing of the wlan on/off key (Fn + F3 I believe) ?
That is the whole reason why the special hid-ite driver is necessary.

Benjamin about the wlan on/off key. AFAICR on a press + release of the key a
single hid input report for the generic-desktop  Wireless Radio Controls group
is send. This input-report only has the one button with usage code HID_GD_RFKILL_BTN
in there and it is always 0. It is as if the input-report is only send on release
and not on press. So the hid-ite code emulates a press + release whenever the
input-report is send.

IOW the receiving of the input report is (ab)used as indication of the button
having been pressed.

Regards,

Hans


> 
> Bye for now
> Zdeněk
> 
> pá 31. 1. 2020 v 15:12 odesílatel Benjamin Tissoires <benjamin.tissoires@redhat.com <mailto:benjamin.tissoires@redhat.com>> napsal:
> 
>     On Fri, Jan 31, 2020 at 3:04 PM Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> 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@redhat.com <mailto:hdegoede@redhat.com>> 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@redhat.com <mailto:hdegoede@redhat.com>> 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@vger.kernel.org <mailto:stable@vger.kernel.org>
>      > >>>> Fixes: 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard dock")
>      > >>>> Reported-by: Zdeněk Rampas <zdenda.rampas@gmail.com <mailto:zdenda.rampas@gmail.com>>
>      > >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>>
>      > >>>> ---
>      > >>>>    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
> 

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

[Benjamin Tissoires]

On Fri, Jan 31, 2020 at 5:09 PM Z R <zdenda.rampas@gmail.com> wrote:
>
> I believe I pressed wifi button on both replays for keyboard.

Yep, I can see that. Just to double check, on the last log, you
pressed the wifi button twice?

Anyway, thanks for all the logs, I should have enough to implement the
regression tests.

Cheers,
Benjamin

>
> With latest patch from Hans on top of v5.5.0 touchpads "two finger scrolling" is working again. Attaching current hid-record for keyboard with Wifi button pressed. Events in log appeared after f3 button was "released".
>
> Thanks
>
> Zdeněk
>
> pá 31. 1. 2020 v 16:45 odesílatel Hans de Goede <hdegoede@redhat.com> napsal:
>>
>> Hi,
>>
>> On 1/31/20 4:38 PM, Z R wrote:
>> > Hi Benjamin,
>> > hid-record for keyboard and touchpad. With Commit 8f18eca9ebc5 reverted and from unmodified kernel.
>> >
>> > I hope it is what you asked for :-)
>> >
>> > Currently waiting for reworked patch from Hans.
>>
>> I just send you the reworked patch.
>>
>> Does the recordning include pressing of the wlan on/off key (Fn + F3 I believe) ?
>> That is the whole reason why the special hid-ite driver is necessary.
>>
>> Benjamin about the wlan on/off key. AFAICR on a press + release of the key a
>> single hid input report for the generic-desktop  Wireless Radio Controls group
>> is send. This input-report only has the one button with usage code HID_GD_RFKILL_BTN
>> in there and it is always 0. It is as if the input-report is only send on release
>> and not on press. So the hid-ite code emulates a press + release whenever the
>> input-report is send.
>>
>> IOW the receiving of the input report is (ab)used as indication of the button
>> having been pressed.
>>
>> Regards,
>>
>> Hans
>>
>>
>> >
>> > Bye for now
>> > Zdeněk
>> >
>> > pá 31. 1. 2020 v 15:12 odesílatel Benjamin Tissoires <benjamin.tissoires@redhat.com <mailto:benjamin.tissoires@redhat.com>> napsal:
>> >
>> >     On Fri, Jan 31, 2020 at 3:04 PM Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> 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@redhat.com <mailto:hdegoede@redhat.com>> 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@redhat.com <mailto:hdegoede@redhat.com>> 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@vger.kernel.org <mailto:stable@vger.kernel.org>
>> >      > >>>> Fixes: 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard dock")
>> >      > >>>> Reported-by: Zdeněk Rampas <zdenda.rampas@gmail.com <mailto:zdenda.rampas@gmail.com>>
>> >      > >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>>
>> >      > >>>> ---
>> >      > >>>>    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
>> >
>>

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

[Benjamin Tissoires]

On Fri, Jan 31, 2020 at 6:17 PM Z R <zdenda.rampas@gmail.com> wrote:
>
> Hi Benjamin,
> in last log touchpad.log (omg, should be keyboard.log), I pressed fn-f3 multiple times. I got one two liner:
>
> # ReportID: 3 / Wireless Radio Button: 0 | #
> E: 000007.606583 2 03 00

great, thanks.

>
> every time i release f3. Does not matter what is happening with fn-key. (could be released already or still pushed down). This log was collected with last patch from Hans applied.

Actually, it doesn't really matter if the patch from Hans was applied
or not. I am looking at the raw event from the device before kernel
processing :)

May I ask you one more thing: can you run `libinput record` (as root)
on the touchpad? I need to know how many fingers the touchpad is
capable to detect, as right now, my tests are failing because of that.

>
> Sorry for the mess I caused :-) I still don't get how you guys manage to have your emails so well polished and readable.

No worries. Having a responsive user is much more valuable than
someone who takes ages to reply even in a clear and well polished
message :)

Cheers,
Benjamin

>
> Bye
> Zdeněk
>
> pá 31. 1. 2020 v 18:00 odesílatel Benjamin Tissoires <benjamin.tissoires@redhat.com> napsal:
>>
>> On Fri, Jan 31, 2020 at 5:09 PM Z R <zdenda.rampas@gmail.com> wrote:
>> >
>> > I believe I pressed wifi button on both replays for keyboard.
>>
>> Yep, I can see that. Just to double check, on the last log, you
>> pressed the wifi button twice?
>>
>> Anyway, thanks for all the logs, I should have enough to implement the
>> regression tests.
>>
>> Cheers,
>> Benjamin
>>
>> >
>> > With latest patch from Hans on top of v5.5.0 touchpads "two finger scrolling" is working again. Attaching current hid-record for keyboard with Wifi button pressed. Events in log appeared after f3 button was "released".
>> >
>> > Thanks
>> >
>> > Zdeněk
>> >
>> > pá 31. 1. 2020 v 16:45 odesílatel Hans de Goede <hdegoede@redhat.com> napsal:
>> >>
>> >> Hi,
>> >>
>> >> On 1/31/20 4:38 PM, Z R wrote:
>> >> > Hi Benjamin,
>> >> > hid-record for keyboard and touchpad. With Commit 8f18eca9ebc5 reverted and from unmodified kernel.
>> >> >
>> >> > I hope it is what you asked for :-)
>> >> >
>> >> > Currently waiting for reworked patch from Hans.
>> >>
>> >> I just send you the reworked patch.
>> >>
>> >> Does the recordning include pressing of the wlan on/off key (Fn + F3 I believe) ?
>> >> That is the whole reason why the special hid-ite driver is necessary.
>> >>
>> >> Benjamin about the wlan on/off key. AFAICR on a press + release of the key a
>> >> single hid input report for the generic-desktop  Wireless Radio Controls group
>> >> is send. This input-report only has the one button with usage code HID_GD_RFKILL_BTN
>> >> in there and it is always 0. It is as if the input-report is only send on release
>> >> and not on press. So the hid-ite code emulates a press + release whenever the
>> >> input-report is send.
>> >>
>> >> IOW the receiving of the input report is (ab)used as indication of the button
>> >> having been pressed.
>> >>
>> >> Regards,
>> >>
>> >> Hans
>> >>
>> >>
>> >> >
>> >> > Bye for now
>> >> > Zdeněk
>> >> >
>> >> > pá 31. 1. 2020 v 15:12 odesílatel Benjamin Tissoires <benjamin.tissoires@redhat.com <mailto:benjamin.tissoires@redhat.com>> napsal:
>> >> >
>> >> >     On Fri, Jan 31, 2020 at 3:04 PM Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> 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@redhat.com <mailto:hdegoede@redhat.com>> 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@redhat.com <mailto:hdegoede@redhat.com>> 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@vger.kernel.org <mailto:stable@vger.kernel.org>
>> >> >      > >>>> Fixes: 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard dock")
>> >> >      > >>>> Reported-by: Zdeněk Rampas <zdenda.rampas@gmail.com <mailto:zdenda.rampas@gmail.com>>
>> >> >      > >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>>
>> >> >      > >>>> ---
>> >> >      > >>>>    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
>> >> >
>> >>
>>

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

[Benjamin Tissoires]

On Fri, Jan 31, 2020 at 6:51 PM Z R <zdenda.rampas@gmail.com> wrote:
>
> libiniput record touchpad - with one small two finger scroll:
>
> root@debswitch:~# libinput record
> Available devices:
> /dev/input/event0:      Video Bus
> /dev/input/event1:      Acer WMI hotkeys
> /dev/input/event2:      SYNA7508:00 06CB:10EB
> /dev/input/event3:      SYNA7508:00 06CB:10EB
> /dev/input/event4:      ITE Tech. Inc. ITE Device(8595) Keyboard
> /dev/input/event5:      ITE Tech. Inc. ITE Device(8595) Consumer Control
> /dev/input/event6:      ITE Tech. Inc. ITE Device(8595) Wireless Radio Control
> /dev/input/event7:      ITE Tech. Inc. ITE Device(8595)
> /dev/input/event8:      ITE Tech. Inc. ITE Device(8595) System Control
> /dev/input/event9:      PC Speaker
> /dev/input/event10:     ITE Tech. Inc. ITE Device(8595) Mouse
> /dev/input/event11:     Intel HDMI/DP LPE Audio HDMI/DP,pcm=0
> /dev/input/event12:     Intel HDMI/DP LPE Audio HDMI/DP,pcm=1
> /dev/input/event13:     SYNA7508:00 06CB:10EB Mouse
> /dev/input/event14:     ITE Tech. Inc. ITE Device(8595) Touchpad
> /dev/input/event15:     gpio-keys
> /dev/input/event16:     gpio-keys
> /dev/input/event17:     bytcr-rt5640 Headset
> Select the device event number: 14
> Recording to 'stdout'.
> version: 1
> ndevices: 1
> libinput:
>   version: "1.12.6"
>   git: "unknown"
> system:
>   kernel: "5.5.0-vanilla+switch+revert8f18eca-debconf55rc5"
>   dmi: "dmi:bvnINSYDECorp.:bvrV1.20:bd03/01/2016:svnAcer:pnAspireSW5-012:pvrV1.20:rvnAcer:rnFendi2:rvrV1.20:cvnAcer:ct10:cvrChassisVersion:"
> devices:
> - node: /dev/input/event14
>   evdev:
>     # Name: ITE Tech. Inc. ITE Device(8595) Touchpad
>     # ID: bus 0x3 vendor 0x6cb product 0x2968 version 0x110
>     # Size in mm: 87x47
>     # Supported Events:
>     # Event type 0 (EV_SYN)
>     # Event type 1 (EV_KEY)
>     #   Event code 272 (BTN_LEFT)
>     #   Event code 325 (BTN_TOOL_FINGER)
>     #   Event code 330 (BTN_TOUCH)
>     #   Event code 333 (BTN_TOOL_DOUBLETAP)
>     #   Event code 334 (BTN_TOOL_TRIPLETAP)
>     # Event type 3 (EV_ABS)
>     #   Event code 0 (ABS_X)
>     #       Value         237
>     #       Min             0
>     #       Max          1051
>     #       Fuzz            0
>     #       Flat            0
>     #       Resolution     12
>     #   Event code 1 (ABS_Y)
>     #       Value         166
>     #       Min             0
>     #       Max           571
>     #       Fuzz            0
>     #       Flat            0
>     #       Resolution     12
>     #   Event code 47 (ABS_MT_SLOT)
>     #       Value           0
>     #       Min             0
>     #       Max             2

Thanks!, that means you have only up to 2 fingers that can be reported.

FYI, first PR: https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/69
(I need to update it with this max_contact information)

Still working on the hid-ite.c regression tests.

Cheers,
Benjamin

>     #       Fuzz            0
>     #       Flat            0
>     #       Resolution      0
>     #   Event code 53 (ABS_MT_POSITION_X)
>     #       Value           0
>     #       Min             0
>     #       Max          1051
>     #       Fuzz            0
>     #       Flat            0
>     #       Resolution     12
>     #   Event code 54 (ABS_MT_POSITION_Y)
>     #       Value           0
>     #       Min             0
>     #       Max           571
>     #       Fuzz            0
>     #       Flat            0
>     #       Resolution     12
>     #   Event code 55 (ABS_MT_TOOL_TYPE)
>     #       Value           0
>     #       Min             0
>     #       Max             2
>     #       Fuzz            0
>     #       Flat            0
>     #       Resolution      0
>     #   Event code 57 (ABS_MT_TRACKING_ID)
>     #       Value           0
>     #       Min             0
>     #       Max         65535
>     #       Fuzz            0
>     #       Flat            0
>     #       Resolution      0
>     # Event type 4 (EV_MSC)
>     #   Event code 5 (MSC_TIMESTAMP)
>     # Properties:
>     #    Property 0 (INPUT_PROP_POINTER)
>     #    Property 2 (INPUT_PROP_BUTTONPAD)
>     name: "ITE Tech. Inc. ITE Device(8595) Touchpad"
>     id: [3, 1739, 10600, 272]
>     codes:
>       0: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15] # EV_SYN
>       1: [272, 325, 330, 333, 334] # EV_KEY
>       3: [0, 1, 47, 53, 54, 55, 57] # EV_ABS
>       4: [5] # EV_MSC
>     absinfo:
>       0: [0, 1051, 0, 0, 12]
>       1: [0, 571, 0, 0, 12]
>       47: [0, 2, 0, 0, 0]
>       53: [0, 1051, 0, 0, 12]
>       54: [0, 571, 0, 0, 12]
>       55: [0, 2, 0, 0, 0]
>       57: [0, 65535, 0, 0, 0]
>     properties: [0, 2]
>   udev:
>     properties:
>     - ID_INPUT=1
>     - ID_INPUT_HEIGHT_MM=47
>     - ID_INPUT_TOUCHPAD=1
>     - ID_INPUT_TOUCHPAD_INTEGRATION=internal
>     - ID_INPUT_WIDTH_MM=87
>     - LIBINPUT_DEVICE_GROUP=3/6cb/2968:usb-0000:00:14.0-1
>   quirks:
>   events:
>   - evdev:
>     - [  0,      0,   3,  57,    80] # EV_ABS / ABS_MT_TRACKING_ID     80
>     - [  0,      0,   3,  53,   608] # EV_ABS / ABS_MT_POSITION_X     608
>     - [  0,      0,   3,  54,   255] # EV_ABS / ABS_MT_POSITION_Y     255
>     - [  0,      0,   1, 330,     1] # EV_KEY / BTN_TOUCH               1
>     - [  0,      0,   1, 325,     1] # EV_KEY / BTN_TOOL_FINGER         1
>     - [  0,      0,   3,   0,   608] # EV_ABS / ABS_X                 608
>     - [  0,      0,   3,   1,   255] # EV_ABS / ABS_Y                 255
>     - [  0,      0,   4,   5,     0] # EV_MSC / MSC_TIMESTAMP           0
>     - [  0,      0,   0,   0,     0] # ------------ SYN_REPORT (0) ---------- +0ms
>   - evdev:
>     - [  0,   9967,   3,  47,     1] # EV_ABS / ABS_MT_SLOT             1
>     - [  0,   9967,   3,  57,    81] # EV_ABS / ABS_MT_TRACKING_ID     81
>     - [  0,   9967,   3,  53,   326] # EV_ABS / ABS_MT_POSITION_X     326
>     - [  0,   9967,   3,  54,   324] # EV_ABS / ABS_MT_POSITION_Y     324
>     - [  0,   9967,   3,  47,     0] # EV_ABS / ABS_MT_SLOT             0
>     - [  0,   9967,   3,  53,   614] # EV_ABS / ABS_MT_POSITION_X     614
>     - [  0,   9967,   3,  54,   250] # EV_ABS / ABS_MT_POSITION_Y     250
>     - [  0,   9967,   1, 325,     0] # EV_KEY / BTN_TOOL_FINGER         0
>     - [  0,   9967,   1, 333,     1] # EV_KEY / BTN_TOOL_DOUBLETAP      1
>     - [  0,   9967,   3,   0,   614] # EV_ABS / ABS_X                 614
>     - [  0,   9967,   3,   1,   250] # EV_ABS / ABS_Y                 250
>     - [  0,   9967,   4,   5,  7200] # EV_MSC / MSC_TIMESTAMP        7200
>     - [  0,   9967,   0,   0,     0] # ------------ SYN_REPORT (0) ---------- +9ms
>   - evdev:
>     - [  0,  16723,   3,  47,     1] # EV_ABS / ABS_MT_SLOT             1
>     - [  0,  16723,   3,  53,   323] # EV_ABS / ABS_MT_POSITION_X     323
>     - [  0,  16723,   3,  54,   309] # EV_ABS / ABS_MT_POSITION_Y     309
>     - [  0,  16723,   3,  47,     0] # EV_ABS / ABS_MT_SLOT             0
>     - [  0,  16723,   3,  53,   612] # EV_ABS / ABS_MT_POSITION_X     612
>     - [  0,  16723,   3,  54,   240] # EV_ABS / ABS_MT_POSITION_Y     240
>     - [  0,  16723,   3,   0,   612] # EV_ABS / ABS_X                 612
>     - [  0,  16723,   3,   1,   240] # EV_ABS / ABS_Y                 240
>     - [  0,  16723,   4,   5, 14500] # EV_MSC / MSC_TIMESTAMP        14500
>     - [  0,  16723,   0,   0,     0] # ------------ SYN_REPORT (0) ---------- +7ms
>   - evdev:
>     - [  0,  24982,   3,  47,     1] # EV_ABS / ABS_MT_SLOT             1
>     - [  0,  24982,   3,  53,   321] # EV_ABS / ABS_MT_POSITION_X     321
>     - [  0,  24982,   3,  54,   294] # EV_ABS / ABS_MT_POSITION_Y     294
>     - [  0,  24982,   3,  47,     0] # EV_ABS / ABS_MT_SLOT             0
>     - [  0,  24982,   3,  53,   614] # EV_ABS / ABS_MT_POSITION_X     614
>     - [  0,  24982,   3,  54,   228] # EV_ABS / ABS_MT_POSITION_Y     228
>     - [  0,  24982,   3,   0,   614] # EV_ABS / ABS_X                 614
>     - [  0,  24982,   3,   1,   228] # EV_ABS / ABS_Y                 228
>     - [  0,  24982,   4,   5, 21900] # EV_MSC / MSC_TIMESTAMP        21900
>     - [  0,  24982,   0,   0,     0] # ------------ SYN_REPORT (0) ---------- +8ms
>   - evdev:
>     - [  0,  32006,   3,  47,     1] # EV_ABS / ABS_MT_SLOT             1
>     - [  0,  32006,   3,  53,   320] # EV_ABS / ABS_MT_POSITION_X     320
>     - [  0,  32006,   3,  54,   286] # EV_ABS / ABS_MT_POSITION_Y     286
>     - [  0,  32006,   3,  47,     0] # EV_ABS / ABS_MT_SLOT             0
>     - [  0,  32006,   3,  54,   220] # EV_ABS / ABS_MT_POSITION_Y     220
>     - [  0,  32006,   3,   1,   220] # EV_ABS / ABS_Y                 220
>     - [  0,  32006,   4,   5, 29200] # EV_MSC / MSC_TIMESTAMP        29200
>     - [  0,  32006,   0,   0,     0] # ------------ SYN_REPORT (0) ---------- +8ms
>   - evdev:
>     - [  0,  38703,   3,  47,     1] # EV_ABS / ABS_MT_SLOT             1
>     - [  0,  38703,   3,  53,   317] # EV_ABS / ABS_MT_POSITION_X     317
>     - [  0,  38703,   3,  54,   276] # EV_ABS / ABS_MT_POSITION_Y     276
>     - [  0,  38703,   3,  47,     0] # EV_ABS / ABS_MT_SLOT             0
>     - [  0,  38703,   3,  54,   215] # EV_ABS / ABS_MT_POSITION_Y     215
>     - [  0,  38703,   3,   1,   215] # EV_ABS / ABS_Y                 215
>     - [  0,  38703,   4,   5, 36400] # EV_MSC / MSC_TIMESTAMP        36400
>     - [  0,  38703,   0,   0,     0] # ------------ SYN_REPORT (0) ---------- +6ms
>   - evdev:
>     - [  0,  46798,   3,  47,     1] # EV_ABS / ABS_MT_SLOT             1
>     - [  0,  46798,   3,  53,   315] # EV_ABS / ABS_MT_POSITION_X     315
>     - [  0,  46798,   3,  54,   268] # EV_ABS / ABS_MT_POSITION_Y     268
>     - [  0,  46798,   3,  47,     0] # EV_ABS / ABS_MT_SLOT             0
>     - [  0,  46798,   3,  54,   207] # EV_ABS / ABS_MT_POSITION_Y     207
>     - [  0,  46798,   3,   1,   207] # EV_ABS / ABS_Y                 207
>     - [  0,  46798,   4,   5, 43700] # EV_MSC / MSC_TIMESTAMP        43700
>     - [  0,  46798,   0,   0,     0] # ------------ SYN_REPORT (0) ---------- +8ms
>   - evdev:
>     - [  0,  53969,   3,  47,     1] # EV_ABS / ABS_MT_SLOT             1
>     - [  0,  53969,   3,  53,   313] # EV_ABS / ABS_MT_POSITION_X     313
>     - [  0,  53969,   3,  54,   258] # EV_ABS / ABS_MT_POSITION_Y     258
>     - [  0,  53969,   3,  47,     0] # EV_ABS / ABS_MT_SLOT             0
>     - [  0,  53969,   3,  54,   201] # EV_ABS / ABS_MT_POSITION_Y     201
>     - [  0,  53969,   3,   1,   201] # EV_ABS / ABS_Y                 201
>     - [  0,  53969,   4,   5, 50900] # EV_MSC / MSC_TIMESTAMP        50900
>     - [  0,  53969,   0,   0,     0] # ------------ SYN_REPORT (0) ---------- +7ms
>   - evdev:
>     - [  0,  60969,   3,  47,     1] # EV_ABS / ABS_MT_SLOT             1
>     - [  0,  60969,   3,  53,   312] # EV_ABS / ABS_MT_POSITION_X     312
>     - [  0,  60969,   3,  54,   255] # EV_ABS / ABS_MT_POSITION_Y     255
>     - [  0,  60969,   3,  47,     0] # EV_ABS / ABS_MT_SLOT             0
>     - [  0,  60969,   3,  54,   197] # EV_ABS / ABS_MT_POSITION_Y     197
>     - [  0,  60969,   3,   1,   197] # EV_ABS / ABS_Y                 197
>     - [  0,  60969,   4,   5, 58100] # EV_MSC / MSC_TIMESTAMP        58100
>     - [  0,  60969,   0,   0,     0] # ------------ SYN_REPORT (0) ---------- +7ms
>   - evdev:
>     - [  0,  69142,   3,  47,     1] # EV_ABS / ABS_MT_SLOT             1
>     - [  0,  69142,   3,  53,   310] # EV_ABS / ABS_MT_POSITION_X     310
>     - [  0,  69142,   3,  54,   251] # EV_ABS / ABS_MT_POSITION_Y     251
>     - [  0,  69142,   3,  47,     0] # EV_ABS / ABS_MT_SLOT             0
>     - [  0,  69142,   3,  54,   193] # EV_ABS / ABS_MT_POSITION_Y     193
>     - [  0,  69142,   3,   1,   193] # EV_ABS / ABS_Y                 193
>     - [  0,  69142,   4,   5, 65400] # EV_MSC / MSC_TIMESTAMP        65400
>     - [  0,  69142,   0,   0,     0] # ------------ SYN_REPORT (0) ---------- +9ms
>   - evdev:
>     - [  0,  76007,   3,  47,     1] # EV_ABS / ABS_MT_SLOT             1
>     - [  0,  76007,   3,  54,   246] # EV_ABS / ABS_MT_POSITION_Y     246
>     - [  0,  76007,   3,  47,     0] # EV_ABS / ABS_MT_SLOT             0
>     - [  0,  76007,   3,  53,   613] # EV_ABS / ABS_MT_POSITION_X     613
>     - [  0,  76007,   3,   0,   613] # EV_ABS / ABS_X                 613
>     - [  0,  76007,   4,   5, 72600] # EV_MSC / MSC_TIMESTAMP        72600
>     - [  0,  76007,   0,   0,     0] # ------------ SYN_REPORT (0) ---------- +7ms
>   - evdev:
>     - [  0,  83070,   3,  47,     1] # EV_ABS / ABS_MT_SLOT             1
>     - [  0,  83070,   3,  54,   241] # EV_ABS / ABS_MT_POSITION_Y     241
>     - [  0,  83070,   3,  47,     0] # EV_ABS / ABS_MT_SLOT             0
>     - [  0,  83070,   3,  54,   192] # EV_ABS / ABS_MT_POSITION_Y     192
>     - [  0,  83070,   3,   1,   192] # EV_ABS / ABS_Y                 192
>     - [  0,  83070,   4,   5, 79900] # EV_MSC / MSC_TIMESTAMP        79900
>     - [  0,  83070,   0,   0,     0] # ------------ SYN_REPORT (0) ---------- +7ms
>   - evdev:
>     - [  0,  89724,   3,  47,     1] # EV_ABS / ABS_MT_SLOT             1
>     - [  0,  89724,   3,  57,    -1] # EV_ABS / ABS_MT_TRACKING_ID     -1
>     - [  0,  89724,   3,  47,     0] # EV_ABS / ABS_MT_SLOT             0
>     - [  0,  89724,   3,  57,    -1] # EV_ABS / ABS_MT_TRACKING_ID     -1
>     - [  0,  89724,   1, 330,     0] # EV_KEY / BTN_TOUCH               0
>     - [  0,  89724,   1, 333,     0] # EV_KEY / BTN_TOOL_DOUBLETAP      0
>     - [  0,  89724,   4,   5, 87100] # EV_MSC / MSC_TIMESTAMP        87100
>     - [  0,  89724,   0,   0,     0] # ------------ SYN_REPORT (0) ---------- +6ms
>                                      # Touch device in neutral state
>

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

[Hans de Goede]

Hi,

On 1/31/20 6:17 PM, Z R wrote:
> Hi Benjamin,
> in last log touchpad.log (omg, should be keyboard.log), I pressed fn-f3 multiple times. I got one two liner:
> 
> # ReportID: 3 / Wireless Radio Button: 0 | #
> E: 000007.606583 2 03 00
> 
> every time i release f3. Does not matter what is happening with fn-key. (could be released already or still pushed down). This log was collected with last patch from Hans applied.
> 
> Sorry for the mess I caused :-) I still don't get how you guys manage to have your emails so well polished and readable.

I don't think you've, caused a mess. Thank you both for the bug report and for your
help in testing this.

One last test request, can you run evemu-record on a kernel with my latest simplified patch,
select the keyboard device and then press (and release) the "rfkill" key and see it generates
a RF_KILL key press + release evdev event?

What would also be helpful is the output of:

ls -l /sys/bus/hid/devices/0003*/driver

Regards,

Hans





> pá 31. 1. 2020 v 18:00 odesílatel Benjamin Tissoires <benjamin.tissoires@redhat.com <mailto:benjamin.tissoires@redhat.com>> napsal:
> 
>     On Fri, Jan 31, 2020 at 5:09 PM Z R <zdenda.rampas@gmail.com <mailto:zdenda.rampas@gmail.com>> wrote:
>      >
>      > I believe I pressed wifi button on both replays for keyboard.
> 
>     Yep, I can see that. Just to double check, on the last log, you
>     pressed the wifi button twice?
> 
>     Anyway, thanks for all the logs, I should have enough to implement the
>     regression tests.
> 
>     Cheers,
>     Benjamin
> 
>      >
>      > With latest patch from Hans on top of v5.5.0 touchpads "two finger scrolling" is working again. Attaching current hid-record for keyboard with Wifi button pressed. Events in log appeared after f3 button was "released".
>      >
>      > Thanks
>      >
>      > Zdeněk
>      >
>      > pá 31. 1. 2020 v 16:45 odesílatel Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> napsal:
>      >>
>      >> Hi,
>      >>
>      >> On 1/31/20 4:38 PM, Z R wrote:
>      >> > Hi Benjamin,
>      >> > hid-record for keyboard and touchpad. With Commit 8f18eca9ebc5 reverted and from unmodified kernel.
>      >> >
>      >> > I hope it is what you asked for :-)
>      >> >
>      >> > Currently waiting for reworked patch from Hans.
>      >>
>      >> I just send you the reworked patch.
>      >>
>      >> Does the recordning include pressing of the wlan on/off key (Fn + F3 I believe) ?
>      >> That is the whole reason why the special hid-ite driver is necessary.
>      >>
>      >> Benjamin about the wlan on/off key. AFAICR on a press + release of the key a
>      >> single hid input report for the generic-desktop  Wireless Radio Controls group
>      >> is send. This input-report only has the one button with usage code HID_GD_RFKILL_BTN
>      >> in there and it is always 0. It is as if the input-report is only send on release
>      >> and not on press. So the hid-ite code emulates a press + release whenever the
>      >> input-report is send.
>      >>
>      >> IOW the receiving of the input report is (ab)used as indication of the button
>      >> having been pressed.
>      >>
>      >> Regards,
>      >>
>      >> Hans
>      >>
>      >>
>      >> >
>      >> > Bye for now
>      >> > Zdeněk
>      >> >
>      >> > pá 31. 1. 2020 v 15:12 odesílatel Benjamin Tissoires <benjamin.tissoires@redhat.com <mailto:benjamin.tissoires@redhat.com> <mailto:benjamin.tissoires@redhat.com <mailto:benjamin.tissoires@redhat.com>>> napsal:
>      >> >
>      >> >     On Fri, Jan 31, 2020 at 3:04 PM Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com> <mailto:hdegoede@redhat.com <mailto:hdegoede@redhat.com>>> 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@redhat.com <mailto:hdegoede@redhat.com> <mailto:hdegoede@redhat.com <mailto:hdegoede@redhat.com>>> 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@redhat.com <mailto:hdegoede@redhat.com> <mailto:hdegoede@redhat.com <mailto:hdegoede@redhat.com>>> 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@vger.kernel.org <mailto:stable@vger.kernel.org> <mailto:stable@vger.kernel.org <mailto:stable@vger.kernel.org>>
>      >> >      > >>>> Fixes: 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard dock")
>      >> >      > >>>> Reported-by: Zdeněk Rampas <zdenda.rampas@gmail.com <mailto:zdenda.rampas@gmail.com> <mailto:zdenda.rampas@gmail.com <mailto:zdenda.rampas@gmail.com>>>
>      >> >      > >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com> <mailto:hdegoede@redhat.com <mailto:hdegoede@redhat.com>>>
>      >> >      > >>>> ---
>      >> >      > >>>>    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
>      >> >
>      >>
> 

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

[Hans de Goede]

Hi,

On 2/1/20 11:58 AM, Z R wrote:
> Good morning guys,
> ls -l /sys/bus/hid/devices/0003*/driver
> lrwxrwxrwx 1 root root 0 Feb  1 11:44 /sys/bus/hid/devices/0003:06CB:2968.0002/driver -> ../../../../../../../../bus/hid/drivers/itetech
> lrwxrwxrwx 1 root root 0 Feb  1 11:44 /sys/bus/hid/devices/0003:06CB:2968.0003/driver -> ../../../../../../../../bus/hid/drivers/hid-multitouch
> 
> ################################
> #      Waiting for events      #
> ################################
> E: 0.000001 0004 0004 458792    # EV_MSC / MSC_SCAN             458792
> E: 0.000001 0001 001c 0000      # EV_KEY / KEY_ENTER            0
> E: 0.000001 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +0ms
> E: 4.068903 0001 00f7 0001      # EV_KEY / KEY_RFKILL           1
> E: 4.068903 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +4068ms
> E: 4.068997 0001 00f7 0000      # EV_KEY / KEY_RFKILL           0
> E: 4.068997 0000 0000 0000      # ------------ SYN_REPORT (0) ---------- +0ms
> E: 17.695302 0001 00f7 0001     # EV_KEY / KEY_RFKILL           1
> E: 17.695302 0000 0000 0000     # ------------ SYN_REPORT (0) ---------- +13627ms
> E: 17.695395 0001 00f7 0000     # EV_KEY / KEY_RFKILL           0
> E: 17.695395 0000 0000 0000     # ------------ SYN_REPORT (0) ---------- +0ms
> E: 24.508532 0004 0004 458976   # EV_MSC / MSC_SCAN             458976
> E: 24.508532 0001 001d 0001     # EV_KEY / KEY_LEFTCTRL         1
> E: 24.508532 0000 0000 0000     # ------------ SYN_REPORT (0) ---------- +6813ms
> E: 24.744600 0004 0004 458758   # EV_MSC / MSC_SCAN             458758
> E: 24.744600 0001 002e 0001     # EV_KEY / KEY_C                1
> E: 24.744600 0000 0000 0000     # ------------ SYN_REPORT (0) ---------- +236ms
> 
> I pressed fn-f3 2x ... Wifi disabled, wifi enabled, on kernel patched with:
> -       { 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) },

Great, so everything is working as it should with the simplified patch, I will go and
submit that to Benjamin then so that we can get this regression fixed.

Regards,

Hans