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

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

Hi Doug,

On Tue, Dec 1, 2020 at 10:12 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Nov 11, 2020 at 4:41 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.
> >
> > 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
> >
> >  .../bindings/input/goodix,gt7375p.yaml        |  65 +++++
> >  arch/arm64/configs/defconfig                  |   3 +-
> >  drivers/hid/Makefile                          |   2 +-
> >  drivers/hid/i2c-hid/Kconfig                   |  47 +++-
> >  drivers/hid/i2c-hid/Makefile                  |   6 +-
> >  drivers/hid/i2c-hid/i2c-hid-acpi.c            | 159 +++++++++++
> >  drivers/hid/i2c-hid/i2c-hid-core.c            | 254 +++---------------
> >  drivers/hid/i2c-hid/i2c-hid-of-goodix.c       | 116 ++++++++
> >  drivers/hid/i2c-hid/i2c-hid-of.c              | 143 ++++++++++
> >  drivers/hid/i2c-hid/i2c-hid.h                 |  22 ++
> >  include/linux/platform_data/i2c-hid.h         |  41 ---
> >  11 files changed, 596 insertions(+), 262 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> >  create mode 100644 drivers/hid/i2c-hid/i2c-hid-acpi.c
> >  create mode 100644 drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> >  create mode 100644 drivers/hid/i2c-hid/i2c-hid-of.c
> >  delete mode 100644 include/linux/platform_data/i2c-hid.h
>
> Are there any additional changes that folks would like with this
> series?  It's not crazy urgent to get it in, but it touches enough
> lines of code that it'd be nice to get it in before other patches land
> and it gets merge conflicts.

Sorry for the delay. I was having an internal deadline last week. I
just re-read the code, and I am quite happy with it. I also just
tested it on the i2c-hid w/ acpi machine I have here, and it seems OK.

So other than that, do we need to have approvals for patch 2/4
(arch/arm64/configs/defconfig)? I can easily take that in the HID
tree, but I prefer having the approval from the maintainers first.
Catalin, Will?

>
> Hrm, I just checked and there actually is already one merge conflict
> with commit afdd34c5fa40 ("HID: i2c-hid: show the error when failing
> to fetch the HID descriptor") but that commit (and thus the
> resolution) is trivial.  If there are no other comments I can re-post
> atop that patch.  ...or I'm also happy if a maintainer is OK w/
> resolving when landing my series.  Just let me know!

If I can quickly get the approval from the arm64/config maintainers, I
can try to apply it. Though, I wouldn't be against you sending a clean
and conflict-free series :)

>
> ...or, if you want me to just shut up for a while and wait until your
> tryptophan hangover wears off, that's fine too--just let me know.
>

Heh. Sorry, I have a tendency to have my inbox flooded, and some time
gets distracted to do other important work I am paid for (too). I
don't mind a gentle nudge from time to time, that helps figuring out
the priorities :)

Cheers,
Benjamin