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