Date:   Thu, 23 Dec 2021 13:21:20 -0800
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     Raul E Rangel <rrangel@chromium.org>, linux-kernel@vger.kernel.org,
        mario.limonciello@amd.com, linux-input@vger.kernel.org,
        dianders@chromium.org, "jingle.wu" <jingle.wu@emc.com.tw>
Subject: Re: [PATCH 2/3] Input: elan_i2c - Use PM subsystem to manage wake irq
Message-ID: <YcToUCQ8gzzSWbrm@google.com>
References: <20211220234346.2798027-1-rrangel@chromium.org>
 <20211220163823.2.Id022caf53d01112188308520915798f08a33cd3e@changeid>
 <YcE+xrSnS7qw0G1/@google.com>
 <9b004b3d-deed-1b63-2344-a445a9e53b61@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9b004b3d-deed-1b63-2344-a445a9e53b61@redhat.com>
Precedence: bulk

On Thu, Dec 23, 2021 at 03:42:24PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 12/21/21 03:41, Dmitry Torokhov wrote:
> > Hi Raul,
> > 
> > On Mon, Dec 20, 2021 at 04:43:45PM -0700, Raul E Rangel wrote:
> >> @@ -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...

> > 
> >>  		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/
> 
> 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