Re: [PATCH 2/3] Input: elan_i2c - Use PM subsystem to manage wake irq

On Thu, Dec 23, 2021 at 03:42:24PM +0100, Hans de Goede wrote:

Hi,

On 12/21/21 03:41, Dmitry Torokhov wrote:

quoted

Hi Raul,

On Mon, Dec 20, 2021 at 04:43:45PM -0700, Raul E Rangel wrote:

quoted

@@ -1368,11 +1367,13 @@ static int elan_probe(struct i2c_client *client,
 	}
 
 	/*
-	 * Systems using device tree should set up wakeup via DTS,
+	 * Systems using device tree or ACPI should set up wakeup via DTS/ACPI,
 	 * the rest will configure device as wakeup source by default.
 	 */
-	if (!dev->of_node)
+	if (!dev->of_node && !ACPI_COMPANION(dev)) {

I think this will break our Rambis that use ACPI for enumeration but
actually lack _PRW. As far as I remember their trackpads were capable
of waking up the system.

I think we should remove this chunk completely and instead add necessary
code to drivers/platform/chrome/chrome-laptop.c (I suppose we need to
have additional member in struct acpi_peripheral to indicate whether
device needs to be configured for wakeup and then act upon it in
chromeos_laptop_adjust_client().

FWIW I looked at Rambi some more and I see that it actually defines a
separate device an ACPI to handle wakeups, it is separate from the ACPI
node for the trackpad:

Scope (\_SB)
{
#ifdef BOARD_TRACKPAD_IRQ
        /* Wake device for touchpad */
        Device (TPAD)
        {
                Name (_HID, EisaId ("PNP0C0E"))
                Name (_UID, 1)
                Name (_PRW, Package() { BOARD_TRACKPAD_WAKE_GPIO, 0x3 })

                Name (RBUF, ResourceTemplate()
                {
                        Interrupt (ResourceConsumer, Level, ActiveLow)
                        {
                                BOARD_TRACKPAD_IRQ
                        }
                })

                Method (_CRS)
                {
                        /* Only return interrupt if I2C1 is PCI mode */
                        If (LEqual (\S1EN, 0)) {
                                Return (^RBUF)
                        }

                        /* Return empty resource template otherwise */
                        Return (ResourceTemplate() {})
                }
        }
#endif

I am not quite sure why we did this...

quoted

quoted

 		device_init_wakeup(dev, true);
+		dev_pm_set_wake_irq(dev, client->irq);
+	}

As I already mentioned in my other reply in this thread:

https://lore.kernel.org/linux-input/f594afab-8c1a-8821-a775-e5512e17ce8f@redhat.com/ (local)

AFAICT most x86 ACPI laptops do not use GPEs for wakeup by touchpad and
as such they do not have a _PRW method.

So for wakeup by elan_i2c touchpads to keep working this code is not
just necessary for some ChromeOS devices, but it is necessary on
most ACPI devices.

The problem of not making these calls on devices where a GPE is actually
used for touchpad wakeup (which at least for now is the exception not
the rule) should probably be fixed by no running this "chunk"
when the device has an ACPI_COMPANION (as this patch already checks)
*and* that ACPI_COMPANION has a valid _PRW method.

Simply removing this chunk, or taking this patch as is will very
likely lead to regressions on various x86 laptop models.

Hans, could you share a couple of DSDTs for devices that do not use GPEs
for wakeup?

For OF we already recognize that wakeup source/interrupt might differ
from "main" I2C interrupt, I guess we need to do similar for ACPI cases.
The question is to how determine if a device is supposed to be a wakeup
source if it does not have _PRW.

Thanks.

-- 
Dmitry