i2c: ACPI: Do not check ACPI_FADT_LOW_POWER_S0

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The ACPI_FADT_LOW_POWER_S0 flag merely means that it is better to
use low-power S0 idle on the given platform than S3 (provided that
the latter is supported) and it doesn't preclude using either of
them (which of them will be used depends on the choices made by user
space).

Because of that, ACPI_FADT_LOW_POWER_S0 is generally not sufficient
for making decisions in device drivers and so i2c_hid_acpi_probe()
should not use it.

Moreover, Linux always supports suspend-to-idle, so if a given
device can wake up the system from suspend-to-idle, then it can be
marked as wakeup capable unconditionally, so make that happen in
i2c_hid_acpi_probe().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/hid/i2c-hid/i2c-hid-acpi.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

So after tracing a bunch of code, I finally got a solution that I
think will work. I just uploaded the patch train here:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3858568.
I'll push it to the mailing list once I do a bit more testing.

Do we need to support setting the wake_irq for systems that don't use
DT or ACPI? Ideally I would drop the following block:

if (!dev->of_node && !has_acpi_companion(dev)) {
    device_init_wakeup(dev, true);
    dev_pm_set_wake_irq(dev, client->irq);
}

There are also a few other i2c drivers that need cleanup:
* https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/v5.15/drivers/mfd/max8925-i2c.c;l=218
* https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/v5.15/drivers/input/touchscreen/elants_i2c.c;l=1629
* https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/v5.15/drivers/input/touchscreen/raydium_i2c_ts.c;l=1190

I can send CLs to delete the enable_irq_wake calls from those drivers
if we don't need to support non-DT/non-ACPI boards. Or I can send CLs
to add the boiler plate from above. Do we even need the `disable_irq`
calls in the suspend handlers or can the PM subsystem take care of
that?

Do we also need to handle reading the wake bit from Interrupt/IRQ ACPI
resources? Can those actually wake the system? On AMD platforms the
IO-APIC/PIC can't actually wake the system. It either needs to be an
ACPI GPE or the GPIO controller. If we do need to support it, I can
add some more plumbing.

Thanks!


On Mon, Aug 8, 2022 at 11:10 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Sat, Aug 6, 2022 at 4:20 AM Raul Rangel <rrangel@chromium.org> wrote:
> >
> > I do plan on coming back and updating those patches. I got derailed
> > with other priorities.
>
> I'll leave it to you then.  I'm mostly interested in dropping the
> misguided ACPI_FADT_LOW_POWER_S0 check.
>
> > But as Hans pointed out, we wanted to use
> > `ExclusiveAndWake` to make the decision since not all IRQs can be wake
> > sources while in s0i3.
>
> S0i3 is still S0, so all of the interrupts that work in S0 will still work.
>
> What really matters is whether or not enable_irq_wake() is called for
> the given IRQ, but I'm not sufficiently familiar with the code in
> question to comment on it any further without thorough investigation.
>
> And of course the device needs to be able to generate interrupts in
> the first place and if it is power-manageable by ACPI, I would just
> leave the wakeup handling to the generic ACPI code.
>
> >
> > On Fri, Aug 5, 2022 at 12:54 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > >
> > > Hi,
> > >
> > > On 8/5/22 19:08, Rafael J. Wysocki wrote:
> > > > On Fri, Aug 5, 2022 at 6:59 PM Limonciello, Mario
> > > > <mario.limonciello@amd.com> wrote:
> > > >>
> > > >> On 8/5/2022 11:51, Rafael J. Wysocki wrote:
> > > >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >>>
> > > >>> The ACPI_FADT_LOW_POWER_S0 flag merely means that it is better to
> > > >>> use low-power S0 idle on the given platform than S3 (provided that
> > > >>> the latter is supported) and it doesn't preclude using either of
> > > >>> them (which of them will be used depends on the choices made by user
> > > >>> space).
> > > >>>
> > > >>> Because of that, ACPI_FADT_LOW_POWER_S0 is generally not sufficient
> > > >>> for making decisions in device drivers and so i2c_hid_acpi_probe()
> > > >>> should not use it.
> > > >>>
> > > >>> Moreover, Linux always supports suspend-to-idle, so if a given
> > > >>> device can wake up the system from suspend-to-idle, then it can be
> > > >>> marked as wakeup capable unconditionally, so make that happen in
> > > >>> i2c_hid_acpi_probe().
> > > >>>
> > > >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >>
> > > >> +Raul
> > > >> +Hans
> > > >> +KH
> > > >>
> > > >> Raul had a patch that was actually going to just tear out this code
> > > >> entirely:
> > > >> https://lkml.kernel.org/lkml/20211220163823.1.Ie20ca47a26d3ea68124d8197b67bb1344c67f650@changeid/
> > > >>
> > > >> As part of that patch series discussion another suggestion had
> > > >> transpired
> > > >> (https://patchwork.kernel.org/project/linux-input/patch/20211220163823.2.Id022caf53d01112188308520915798f08a33cd3e@changeid/#24681016):
> > > >>
> > > >> ```
> > > >> if ((acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) &&
> > > >>             !adev->flags.power_manageable) {
> > > >>                  device_set_wakeup_capable(dev, true);
> > > >>                  device_set_wakeup_enable(dev, false);
> > > >>          }
> > > >> ```
> > > >>
> > > >> If this is being changed, maybe consider that suggestion to
> > > >> check `adev->flags.power_manageable`.
> > > >
> > > > Fair enough, I'll send a v2 with this check added.
> > >
> > > Re-reading the original thread:
> > > https://lkml.kernel.org/lkml/20211220163823.1.Ie20ca47a26d3ea68124d8197b67bb1344c67f650@changeid/T/#u
> > >
> > > The conclusion there was that the :
> > >
> > >                  device_set_wakeup_capable(dev, true);
> > >                  device_set_wakeup_enable(dev, false);
> > >
> > > Calls should be made conditional on the IRQ being
> > > marked ExclusiveAndWake instead of the ACPI_FADT_LOW_POWER_S0
> > > check.
> > >
> > > Regards,
> > >
> > > Hans
> > >