Re: [PATCH v8 0/4] HID: i2c-hid: Reorganize to allow supporting goodix,gt7375p

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

On Fri, Jan 8, 2021 at 6:52 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Hi Doug,
>
> On Wed, Jan 6, 2021 at 2:35 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Benjamin,
> >
> > On Fri, Dec 11, 2020 at 2:24 PM Douglas Anderson <dianders@chromium.org> wrote:
> > >
> > > The goal of this series is to support the Goodix GT7375P touchscreen.
> > > This touchscreen is special because it has power sequencing
> > > requirements that necessitate driving a reset GPIO.
> > >
> > > To do this, we totally rejigger the way i2c-hid is organized so that
> > > it's easier to jam the Goodix support in there.
> > >
> > > This series was:
> > > - Tested on a device that uses normal i2c-hid.
> > > - Tested on a device that has a Goodix i2c-hid device.
> > > - Tested on an ACPI device, but an earlier version of the series.
> > >
> > > I believe the plan is for Benjamin to land the whole series.  Will
> > > said this about the arm64 defconfig change (and provided his Ack):
> > > > ...there are a few things I really care about
> > > > in defconfig (e.g. things like page size!), generally speaking we don't
> > > > need to Ack everything that changes in there.
> > > >
> > > > That said, might be worth checking whether arm-soc have any defconfig
> > > > changes queued in -next so you don't end up with conflicts.
> > >
> > > Changes in v8:
> > > - Mark suspend/resume as static as per patches robot.
> > >
> > > Changes in v7:
> > > - Rebase atop commit afdd34c5fa40 ("HID: i2c-hid: show the error ...")
> > >
> > > Changes in v6:
> > > - ACPI probe function should have been "static"
> > > - Don't export suspend/resume, just export dev_pm_ops from core.
> > > - Fixed crash in ACPI module (missing init of "client")
> > > - No need for regulator include in the core.
> > > - Removed i2c_device_id table from ACPI module.
> > > - Suspend/resume are no longer exported from the core.
> > >
> > > Changes in v5:
> > > - Add shutdown_tail op and use it in ACPI.
> > > - Added mention of i2c-hid in the yaml itself as per Rob.
> > > - Adjusted subject as per Rob.
> > > - i2chid_subclass_data => i2chid_ops.
> > > - power_up_device => power_up (same with power_down).
> > > - subclass => ops.
> > >
> > > Changes in v4:
> > > - ("arm64: defconfig: Update config names for i2c-hid rejigger") new for v4.
> > > - Fully rejigger so ACPI and OF are full subclasses.
> > > - Totally redid based on the new subclass system.
> > >
> > > Changes in v3:
> > > - Fixed compatible in example.
> > > - Removed Benjamin as a maintainer.
> > > - Rework to use subclassing.
> > > - Updated description.
> > >
> > > Changes in v2:
> > > - ("dt-bindings: HID: i2c-hid: Introduce bindings for the Goodix GT7375P") new in v2.
> > > - Get timings based on the compatible string.
> > > - Use a separate compatible string for this new touchscreen.
> > >
> > > Douglas Anderson (4):
> > >   HID: i2c-hid: Reorganize so ACPI and OF are separate modules
> > >   arm64: defconfig: Update config names for i2c-hid rejigger
> > >   dt-bindings: input: HID: i2c-hid: Introduce bindings for the Goodix
> > >     GT7375P
> > >   HID: i2c-hid: Introduce goodix-i2c-hid using i2c-hid core
> >
> > I think this series is ready to land.  The "defconfig" has a trivial
> > conflict with commit 74b87103b3d0 ("arm64: defconfig: Enable HID
> > multitouch") against linuxnext, but it's so simple that hopefully
> > folks will be OK with that when it lands.
> >
> > Please let me know if there's anything else you need me to do.  :-)
> >
>
> I wanted to apply the series yesterday, but for these kinds of changes
> I like giving it a spin on actual hardware. Turns out that my XPS-13
> can not boot to v5.11-rc2, which makes testing the new branch slightly
> more difficult.
>
> I'll give it a spin next week, but I think I should be able to land it for 5.12.
>
> Regarding the defconfig conflict, no worries, we can handle it with
> Stephen and Linus.
>

After 2 full kernel bisects (I messed up the first because I am an
idiot and inverted good and bad after the first reboot), I found my
culprit, and I was able to test the series today.

The series works fine regarding enumeration and removing of devices,
but it prevents my system from being suspended. If I rmmod
i2c-hid-acpi, suspend works fine, but if it is present, it immediately
comes back, which makes me think that something must be wrong.

I also just reverted the series and confirmed that suspend/resume now
works, meaning that patch 1/4 needs to be checked.

Cheers,
Benjamin