Re: [PATCH 00/12] i2c-hid: fixes for unnumbered reports and other improvements

[Benjamin Tissoires]

On Wed, Feb 2, 2022 at 6:59 PM Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxxx> wrote:
>
> On Wed, Feb 2, 2022 at 2:56 PM Jiri Kosina <jikos@xxxxxxxxxx> wrote:
> >
> > On Mon, 17 Jan 2022, Dmitry Torokhov wrote:
> >
> > > Hi,
> > >
> > > This series came about after I reviewed Angela's patch that fixed issues
> > > with incorrect handling of high-numbered reports (15 and above) in
> > > i2c-hid driver:
> > >
> > > - it appears to me that the driver did not handle unnumbered reports
> > >   correctly as the kernel internally expects unnumbered reports to be
> > >   still prepended with report number 0, but i2c_hid_get_raw_report() and
> > >   i2c_hid_output_raw_report() only expected report ID to be present for
> > >   numbered reports.
> > >
> > > - the driver tried to consolidate both command handling and sending
> > >   output reports in __i2c_hid_command() but the rules for different
> > >   commands vary significantly and because of that the logic was hard to
> > >   follow and it bled out from __i2c_hid_command() to callers. I decided
> > >   to introduce a few simple helpers and have the data encoding for
> > >   individual commands done at the call site. I believe this made it
> > >   easier to validate the rules and the logic and allowed to remove
> > >   special handling for the HID descriptor retrieval, among other things.
> > >
> > > - the driver does too many copying of data; encoding the data for
> > >   commands at the call site allowed to only copy data once into the
> > >   transfer buffers.
> > >
> > > I tested this on a couple of Chromebooks with i2c-hid touchscreens, but
> > > it would be great if other folks tried it out as well.
> >
> > Benjamin,
> >
> > is this something you could feed into your testing machinery as well? (not
> > sure whether it's handling i2c-hid specifics).
>
> Not really. I mean I have a sample touchpad connected on an USB to I2C
> bridge that I included in the automated tests (though it may be
> failing in the latest release because I need to update the patch that
> enables it), but it would probably not catch all the corner cases.
>
> I can add this series to my local tree and test on my XPS with both
> and Elan touchpad and a Wacom touchscreen/panel. It will still add a
> few more tests :)

OK, So I applied the series on my development laptop.
I had to apply it on top of v5.16 and then rebase on top of
hid.git/for-next because there is a minor conflict.

I changed the register as mentioned in 5/12, and gave it a try.
Both the Elan touchpad and the Wacom panel on my XPS-13 are behaving
properly, suspend/resume works also as expected.

Tested-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>

I gave a quick look at the individual patches, they all seem sane to
me, but haven't dug into enough detail to be able to formally give my
reviewed-by.
Note that I have a small comment on patch 2, but if you want to apply
it nevertheless Jiri (with the change in 5/12) it should be fine.

Cheers,
Benjamin

> I'll try to report that tomorrow now that I think I fixed my tablet series.
>
> Cheers,
> Benjamin
>
> >
> > Thanks!
> >
> > --
> > Jiri Kosina
> > SUSE Labs
> >