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

[Benjamin Tissoires]

Hi,

On Wed, Jan 13, 2021 at 8:35 PM Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> wrote:
> On Wed, Jan 13, 2021 at 5:05 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > On Wed, Jan 13, 2021 at 7:09 AM Benjamin Tissoires
> > <benjamin.tissoires@xxxxxxxxxx> wrote:
> > >
> > > > 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.
> >
> > Can you give me any hints about what type of failure you're seeing?
> > Any logs?  I don't have an ACPI system to test with...
>
> I don't have any logs, just that the system comes back up. There is a
> chance we are not powering the device down correctly, which triggers
> an IRQ and which puts the system back on.
>
> >
> > Is there any chance that some type of userspace / udev rule is getting
> > tripped up by the driver being renamed?  We ran into something like
> > this recently on Chrome OS where we had a tool that was hardcoded to
> > look for "i2c-hid" and needed to be adapted to account for the new
> > driver name.  Often userspace tweaks with wakeup rules based on driver
> > name...
>
> I don't think there is anything like that on a regular desktop.
>
> >
> > I'll go stare at the code now and see if anything jumps out.
> >
>
> Thanks, but don't spend too much time on it, unless something really
> jumps out. I'll debug that tomorrow. It's much easier with an actual
> device than by just looking at the code.

Well, that's weird. Now suspend resume works reliably even with your
series. It could just have been that the lid sensor was too close to a
magnet or something like that. Though while testing the old version of
i2c-hid, it was working... Such a mystery :)

Anyway, while trying to dig up that now-non-issue, I got the following patch
that you likely want to squash into 1/4:

---

diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
index 0f86060f01b4..dd6d9f74e7e7 100644
--- a/drivers/hid/i2c-hid/i2c-hid-acpi.c
+++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
@@ -31,7 +31,6 @@
 struct i2c_hid_acpi {
        struct i2chid_ops ops;
        struct i2c_client *client;
-       bool power_fixed;
 };

 static const struct acpi_device_id i2c_hid_acpi_blacklist[] = {
@@ -75,25 +74,6 @@ static int i2c_hid_acpi_get_descriptor(struct i2c_client *client)
        return hid_descriptor_address;
 }

-static int i2c_hid_acpi_power_up(struct i2chid_ops *ops)
-{
-       struct i2c_hid_acpi *ihid_of =
-               container_of(ops, struct i2c_hid_acpi, ops);
-       struct device *dev = &ihid_of->client->dev;
-       struct acpi_device *adev;
-
-       /* Only need to call acpi_device_fix_up_power() the first time */
-       if (ihid_of->power_fixed)
-               return 0;
-       ihid_of->power_fixed = true;
-
-       adev = ACPI_COMPANION(dev);
-       if (adev)
-               acpi_device_fix_up_power(adev);
-
-       return 0;
-}
-
 static void i2c_hid_acpi_shutdown_tail(struct i2chid_ops *ops)
 {
        struct i2c_hid_acpi *ihid_of =
@@ -107,6 +87,7 @@ static int i2c_hid_acpi_probe(struct i2c_client *client,
 {
        struct device *dev = &client->dev;
        struct i2c_hid_acpi *ihid_acpi;
+       struct acpi_device *adev;
        u16 hid_descriptor_address;
        int ret;

@@ -115,7 +96,6 @@ static int i2c_hid_acpi_probe(struct i2c_client *client,
                return -ENOMEM;

        ihid_acpi->client = client;
-       ihid_acpi->ops.power_up = i2c_hid_acpi_power_up;
        ihid_acpi->ops.shutdown_tail = i2c_hid_acpi_shutdown_tail;

        ret = i2c_hid_acpi_get_descriptor(client);
@@ -123,6 +103,10 @@ static int i2c_hid_acpi_probe(struct i2c_client *client,
                return ret;
        hid_descriptor_address = ret;

+       adev = ACPI_COMPANION(dev);
+       if (adev)
+               acpi_device_fix_up_power(adev);
+
        if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
                device_set_wakeup_capable(dev, true);
                device_set_wakeup_enable(dev, false);


---

This allows to keep the powering ordering of the old i2c-hid module
(power up before setting device wakeup capable), and simplify the
not so obvious power_fixed field of struct i2c_hid_acpi.

(I can also send it as a followup on the series if you prefer).

Cheers,
Benjamin