On Fri, Jan 31, 2020 at 5:09 PM Z R <zdenda.rampas@xxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx <mailto:benjamin.tissoires@xxxxxxxxxx>> napsal: >> > >> > On Fri, Jan 31, 2020 at 3:04 PM Hans de Goede <hdegoede@xxxxxxxxxx <mailto:hdegoede@xxxxxxxxxx>> wrote: >> > > >> > > Hi, >> > > >> > > On 1/31/20 2:54 PM, Benjamin Tissoires wrote: >> > > > On Fri, Jan 31, 2020 at 2:41 PM Hans de Goede <hdegoede@xxxxxxxxxx <mailto:hdegoede@xxxxxxxxxx>> wrote: >> > > >> >> > > >> Hi, >> > > >> >> > > >> On 1/31/20 2:10 PM, Benjamin Tissoires wrote: >> > > >>> Hi Hans, >> > > >>> >> > > >>> On Fri, Jan 31, 2020 at 1:46 PM Hans de Goede <hdegoede@xxxxxxxxxx <mailto:hdegoede@xxxxxxxxxx>> wrote: >> > > >>>> >> > > >>>> Commit 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard >> > > >>>> dock") added the USB id for the Acer SW5-012's keyboard dock to the >> > > >>>> hid-ite driver to fix the rfkill driver not working. >> > > >>>> >> > > >>>> Most keyboard docks with an ITE 8595 keyboard/touchpad controller have the >> > > >>>> "Wireless Radio Control" bits which need the special hid-ite driver on the >> > > >>>> second USB interface (the mouse interface) and their touchpad only supports >> > > >>>> mouse emulation, so using generic hid-input handling for anything but >> > > >>>> the "Wireless Radio Control" bits is fine. On these devices we simply bind >> > > >>>> to all USB interfaces. >> > > >>>> >> > > >>>> But unlike other ITE8595 using keyboard docks, the Acer Aspire Switch 10 >> > > >>>> (SW5-012)'s touchpad not only does mouse emulation it also supports >> > > >>>> HID-multitouch and all the keys including the "Wireless Radio Control" >> > > >>>> bits have been moved to the first USB interface (the keyboard intf). >> > > >>>> >> > > >>>> So we need hid-ite to handle the first (keyboard) USB interface and have >> > > >>>> it NOT bind to the second (mouse) USB interface so that that can be >> > > >>>> handled by hid-multitouch.c and we get proper multi-touch support. >> > > >>>> >> > > >>>> This commit adds a match callback to hid-ite which makes it only >> > > >>>> match the first USB interface when running on the Acer SW5-012, >> > > >>>> fixing the regression to mouse-emulation mode introduced by adding the >> > > >>>> keyboard dock USB id. >> > > >>>> >> > > >>>> Note the match function only does the special only bind to the first >> > > >>>> USB interface on the Acer SW5-012, on other devices the hid-ite driver >> > > >>>> actually must bind to the second interface as that is where the >> > > >>>> "Wireless Radio Control" bits are. >> > > >>> >> > > >>> This is not a full review, but a couple of things that popped out >> > > >>> while scrolling through the patch. >> > > >>> >> > > >>>> >> > > >>>> Cc: stable@xxxxxxxxxxxxxxx <mailto:stable@xxxxxxxxxxxxxxx> >> > > >>>> Fixes: 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard dock") >> > > >>>> Reported-by: Zdeněk Rampas <zdenda.rampas@xxxxxxxxx <mailto:zdenda.rampas@xxxxxxxxx>> >> > > >>>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx <mailto:hdegoede@xxxxxxxxxx>> >> > > >>>> --- >> > > >>>> drivers/hid/hid-ite.c | 34 ++++++++++++++++++++++++++++++++++ >> > > >>>> 1 file changed, 34 insertions(+) >> > > >>>> >> > > >>>> diff --git a/drivers/hid/hid-ite.c b/drivers/hid/hid-ite.c >> > > >>>> index c436e12feb23..69a4ddfd033d 100644 >> > > >>>> --- a/drivers/hid/hid-ite.c >> > > >>>> +++ b/drivers/hid/hid-ite.c >> > > >>>> @@ -8,9 +8,12 @@ >> > > >>>> #include <linux/input.h> >> > > >>>> #include <linux/hid.h> >> > > >>>> #include <linux/module.h> >> > > >>>> +#include <linux/usb.h> >> > > >>>> >> > > >>>> #include "hid-ids.h" >> > > >>>> >> > > >>>> +#define ITE8595_KBD_USB_INTF 0 >> > > >>>> + >> > > >>>> static int ite_event(struct hid_device *hdev, struct hid_field *field, >> > > >>>> struct hid_usage *usage, __s32 value) >> > > >>>> { >> > > >>>> @@ -37,6 +40,36 @@ static int ite_event(struct hid_device *hdev, struct hid_field *field, >> > > >>>> return 0; >> > > >>>> } >> > > >>>> >> > > >>>> +static bool ite_match(struct hid_device *hdev, bool ignore_special_driver) >> > > >>>> +{ >> > > >>>> + struct usb_interface *intf; >> > > >>>> + >> > > >>>> + if (ignore_special_driver) >> > > >>>> + return false; >> > > >>>> + >> > > >>>> + /* >> > > >>>> + * Most keyboard docks with an ITE 8595 keyboard/touchpad controller >> > > >>>> + * have the "Wireless Radio Control" bits which need this special >> > > >>>> + * driver on the second USB interface (the mouse interface). On >> > > >>>> + * these devices we simply bind to all USB interfaces. >> > > >>>> + * >> > > >>>> + * The Acer Aspire Switch 10 (SW5-012) is special, its touchpad >> > > >>>> + * not only does mouse emulation it also supports HID-multitouch >> > > >>>> + * and all the keys including the "Wireless Radio Control" bits >> > > >>>> + * have been moved to the first USB interface (the keyboard intf). >> > > >>>> + * >> > > >>>> + * We want the hid-multitouch driver to bind to the touchpad, so on >> > > >>>> + * the Acer SW5-012 we should only bind to the keyboard USB intf. >> > > >>>> + */ >> > > >>>> + if (hdev->bus != BUS_USB || hdev->vendor != USB_VENDOR_ID_SYNAPTICS || >> > > >>>> + hdev->product != USB_DEVICE_ID_SYNAPTICS_ACER_SWITCH5_012) >> > > >>> >> > > >>> Isn't there an existing matching function we can use here, instead of >> > > >>> checking each individual field? >> > > >> >> > > >> There is hid_match_one_id() but that is not exported (can be fixed) and it >> > > >> requires a struct hid_device_id, which either requires declaring an extra >> > > >> standalone struct hid_device_id for the SW5-012 kbd-dock, or hardcoding an >> > > >> index into the existing hid_device_id array for the driver (with the hardcoding >> > > >> being error prone, so not a good idea). >> > > >> >> > > >> Given the problems with using hid_match_one_id() I decided to just go with >> > > >> the above. >> > > > >> > > > right. An other solution would be to have a local macro/function that >> > > > does that. Because as soon as you start adding a quirk, an other comes >> > > > right after. >> > > > >> > > >> >> > > >> But see below. >> > > >> >> > > >>> >> > > >>>> + return true; >> > > >>>> + >> > > >>>> + intf = to_usb_interface(hdev->dev.parent); >> > > >>> >> > > >>> And this is oops-prone. You need: >> > > >>> - ensure hid_is_using_ll_driver(hdev, &usb_hid_driver) returns true. >> > > >>> - add a dependency on USBHID in the KConfig now that you are checking >> > > >>> on the USB transport layer. >> > > >>> >> > > >>> That being said, I would love instead: >> > > >>> - to have a non USB version of this match, where you decide which >> > > >>> component needs to be handled based on the report descriptor >> > > >> >> > > >> Actually your idea to use the desciptors is not bad, but since what >> > > >> we really want is to not bind to the interface which is marked for the >> > > >> hid-multitouch driver I just realized we can just check that. >> > > >> >> > > >> So how about: >> > > >> >> > > >> static bool ite_match(struct hid_device *hdev, bool ignore_special_driver) >> > > >> { >> > > >> if (ignore_special_driver) >> > > >> return false; >> > > >> >> > > >> /* >> > > >> * Some keyboard docks with an ITE 8595 keyboard/touchpad controller >> > > >> * support the HID multitouch protocol for the touchpad, in that >> > > >> * case the "Wireless Radio Control" bits which we care about are >> > > >> * on the other interface; and we should not bind to the multitouch >> > > >> * capable interface as that breaks multitouch support. >> > > >> */ >> > > >> return hdev->group != HID_GROUP_MULTITOUCH_WIN_8; >> > > >> } >> > > > >> > > > Yep, I like that very much :) >> > > >> > > Actually if we want to check the group and there are only 2 interfaces we do >> > > not need to use the match callback at all, w e can simply match on the >> > > group of the interface which we do want: >> > > >> > > diff --git a/drivers/hid/hid-ite.c b/drivers/hid/hid-ite.c >> > > index db0f35be5a8b..21bd48f16033 100644 >> > > --- a/drivers/hid/hid-ite.c >> > > +++ b/drivers/hid/hid-ite.c >> > > @@ -56,8 +56,9 @@ static const struct hid_device_id ite_devices[] = { >> > > { HID_USB_DEVICE(USB_VENDOR_ID_ITE, USB_DEVICE_ID_ITE8595) }, >> > > { HID_USB_DEVICE(USB_VENDOR_ID_258A, USB_DEVICE_ID_258A_6A88) }, >> > > /* ITE8595 USB kbd ctlr, with Synaptics touchpad connected to it. */ >> > > - { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, >> > > - USB_DEVICE_ID_SYNAPTICS_ACER_SWITCH5_012) }, >> > > + { HID_DEVICE(BUS_USB, HID_GROUP_GENERIC, >> > > + USB_VENDOR_ID_SYNAPTICS, >> > > + USB_DEVICE_ID_SYNAPTICS_ACER_SWITCH5_012) }, >> > > { } >> > > }; >> > > MODULE_DEVICE_TABLE(hid, ite_devices); >> > > >> > > Much cleaner >> > >> > yep >> > >> > > (and now I don't need to write a test, which is always >> > > a good motivation to come up with a cleaner solution :) >> > >> > Hehe, too bad, you already picked up my curiosity on this one, and I >> > really would like to see the report descriptors and some events of the >> > keys that are fixed by hid-ite.c. >> > <with a low voice>This will be a hard requirement to accept this patch </joke>. >> > >> > More seriously, Zdeněk, can you run hid-recorder from >> > https://gitlab.freedesktop.org/libevdev/hid-tools/ and provide me the >> > report descriptor for all of your ITE HID devices? I'll add the >> > matching tests in hid-tools and be sure we do not regress in the >> > future. >> > >> > > >> > > Let me turn this into a proper patch and then I will send that to >> > > Zdeněk (off-list) for him to test (note don't worry if you do >> > > not have time to test this weekend, then I'll do it on Monday). >> > > >> > > Regards, >> > > >> > > Hans >> > > >> > > p.s. >> > > >> > > 1. My train is approaching Brussels (Fosdem) so my email response >> > > time will soon become irregular. >> > >> > How dare you? :) >> > >> > > >> > > 2. Benjamin will you be at Fosdem too ? >> > > >> > >> > Unfortunately no. Already got my quota of meeting people for this year >> > between Kernel Recipes in September, XDC in October and LCA last week. >> > So I need to keep in a quiet environment for a little bit :) >> > >> > Cheers, >> > Benjamin >> > >>
