| Message ID | 20211220163823.2.Id022caf53d01112188308520915798f08a33cd3e@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix spurious wakes on ACPI platforms | expand |
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(). > device_init_wakeup(dev, true); > + dev_pm_set_wake_irq(dev, client->irq); > + } > > return 0; > } Thanks.
On Mon, Dec 20, 2021 at 7:41 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> 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. Looks like the _PRW was only added for the atmel touchscreen: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/coreboot/src/mainboard/google/rambi/acpi/touchscreen_atmel.asl;l=42 I'm assuming this was before we had the `drivers/i2c/hid` chip driver. > > 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(). I think that's a good idea. Should I add all the mainboards defined here: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/coreboot/src/mainboard/google/rambi/Kconfig;l=48 ? > > > device_init_wakeup(dev, true); > > + dev_pm_set_wake_irq(dev, client->irq); > > + } > > > > return 0; > > } > > Thanks. > > -- > Dmitry
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(). > >> 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. Regards, Hans
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.
Hi, On 12/23/21 22:21, Dmitry Torokhov wrote: > 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. With s2idle (rather then S3) we never really suspend, we just put everything in an as low power state as possible and call halt on the CPU and then hope that the SoC power-management-unit shuts of a whole bunch of power-planes based on all the devices being in a low power state. This means that in practice with s2idle any device can be a wakeup device since regular IRQs work fine as wakeup sources in s2idle. This is what the s2idle support in the i2c-hid code is based on: drivers/hid/i2c-hid/i2c-hid-acpi.c: if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) { device_set_wakeup_capable(dev, true); device_set_wakeup_enable(dev, false); } So I did just test this on a Lenovo ThinkPad X1 carbon gen 8, which uses i2c_hid_acpi as driver for its touchpad and if I echo enabled to the wakeup attr there, then wakeup by touchpad does work. One interesting thing there is that the touchpad ACPI node does not have _PS0 and _PS3. Which means that the touchpad working as wakeup device makes sense, since it can not be turned off at all. So I guess we could extend the above check in the i2c-hid-acpi code to read: 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); } Because if there is a _PS3, which presumably is the case for the troublesome touchscreen Raul is trying to fix, then we will call that on suspend; and after that it is likely that the device will not work as a wakeup source. And I just checked the DSDT of a couple of devices where I'm reasonable sure that the touchpad uses I2C-HID and none of them define _PS0/_PS3 methods on the touchpad ACPI node. So I think that the above suggestion should fix things for the i2c-hid case. I've added Kai-Heng, the author of the original change introducing the device_set_wakeup_capable() call, to the Cc. Kai-Heng what do you think about this ? Raul, can you check if this resolves your issue? FWIW here is an acpidump of the X1C8: https://fedorapeople.org/~jwrdegoede/acpidump-lenovo-x1c8 Regards, Hans p.s. An other interesting datapoint is that despite not declaring a _PRW method the DSDTs which I've checked do all 3 contain an _S0W method, returning 3 or 4. Which suggests that maybe the ACPI code should look at _S0W even when no GPE is being used?
On Fri, Dec 24, 2021 at 7:11 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 12/23/21 22:21, Dmitry Torokhov wrote: > > 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. > > With s2idle (rather then S3) we never really suspend, we just put > everything in an as low power state as possible and call halt on the > CPU and then hope that the SoC power-management-unit shuts of a whole > bunch of power-planes based on all the devices being in a low power > state. > > This means that in practice with s2idle any device can be a wakeup > device since regular IRQs work fine as wakeup sources in s2idle. > > This is what the s2idle support in the i2c-hid code is based on: > drivers/hid/i2c-hid/i2c-hid-acpi.c: > > if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) { > device_set_wakeup_capable(dev, true); > device_set_wakeup_enable(dev, false); > } > > So I did just test this on a Lenovo ThinkPad X1 carbon gen 8, which > uses i2c_hid_acpi as driver for its touchpad and if I echo > enabled to the wakeup attr there, then wakeup by touchpad does work. > > One interesting thing there is that the touchpad ACPI node does not > have _PS0 and _PS3. Which means that the touchpad working as wakeup > device makes sense, since it can not be turned off at all. > > So I guess we could extend the above check in the i2c-hid-acpi > code to read: > > 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); > } > > Because if there is a _PS3, which presumably is the case for > the troublesome touchscreen Raul is trying to fix, then we > will call that on suspend; and after that it is likely that > the device will not work as a wakeup source. > > And I just checked the DSDT of a couple of devices where I'm > reasonable sure that the touchpad uses I2C-HID and none of > them define _PS0/_PS3 methods on the touchpad ACPI node. > > So I think that the above suggestion should fix things > for the i2c-hid case. > > I've added Kai-Heng, the author of the original change > introducing the device_set_wakeup_capable() call, to the Cc. > Kai-Heng what do you think about this ? > > Raul, can you check if this resolves your issue? > > FWIW here is an acpidump of the X1C8: > https://fedorapeople.org/~jwrdegoede/acpidump-lenovo-x1c8 > > Regards, > > Hans > > > p.s. > > An other interesting datapoint is that despite not declaring > a _PRW method the DSDTs which I've checked do all 3 contain > an _S0W method, returning 3 or 4. Which suggests that maybe the > ACPI code should look at _S0W even when no GPE is being used? > Maybe "ExclusiveAndWake" in _CRS is enough? ACPI spec says "whether it is capable of waking the system from a low-power idle or system sleep state" without mentioning the need for _PRW. Kai-Heng
Hi, On 12/25/21 14:51, Kai-Heng Feng wrote: > On Fri, Dec 24, 2021 at 7:11 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 12/23/21 22:21, Dmitry Torokhov wrote: >>> 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. >> >> With s2idle (rather then S3) we never really suspend, we just put >> everything in an as low power state as possible and call halt on the >> CPU and then hope that the SoC power-management-unit shuts of a whole >> bunch of power-planes based on all the devices being in a low power >> state. >> >> This means that in practice with s2idle any device can be a wakeup >> device since regular IRQs work fine as wakeup sources in s2idle. >> >> This is what the s2idle support in the i2c-hid code is based on: >> drivers/hid/i2c-hid/i2c-hid-acpi.c: >> >> if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) { >> device_set_wakeup_capable(dev, true); >> device_set_wakeup_enable(dev, false); >> } >> >> So I did just test this on a Lenovo ThinkPad X1 carbon gen 8, which >> uses i2c_hid_acpi as driver for its touchpad and if I echo >> enabled to the wakeup attr there, then wakeup by touchpad does work. >> >> One interesting thing there is that the touchpad ACPI node does not >> have _PS0 and _PS3. Which means that the touchpad working as wakeup >> device makes sense, since it can not be turned off at all. >> >> So I guess we could extend the above check in the i2c-hid-acpi >> code to read: >> >> 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); >> } >> >> Because if there is a _PS3, which presumably is the case for >> the troublesome touchscreen Raul is trying to fix, then we >> will call that on suspend; and after that it is likely that >> the device will not work as a wakeup source. >> >> And I just checked the DSDT of a couple of devices where I'm >> reasonable sure that the touchpad uses I2C-HID and none of >> them define _PS0/_PS3 methods on the touchpad ACPI node. >> >> So I think that the above suggestion should fix things >> for the i2c-hid case. >> >> I've added Kai-Heng, the author of the original change >> introducing the device_set_wakeup_capable() call, to the Cc. >> Kai-Heng what do you think about this ? >> >> Raul, can you check if this resolves your issue? >> >> FWIW here is an acpidump of the X1C8: >> https://fedorapeople.org/~jwrdegoede/acpidump-lenovo-x1c8 >> >> Regards, >> >> Hans >> >> >> p.s. >> >> An other interesting datapoint is that despite not declaring >> a _PRW method the DSDTs which I've checked do all 3 contain >> an _S0W method, returning 3 or 4. Which suggests that maybe the >> ACPI code should look at _S0W even when no GPE is being used? >> Sorry for being slow to respond. > Maybe "ExclusiveAndWake" in _CRS is enough? ACPI spec says "whether it > is capable of waking the system from a low-power idle or system sleep > state" without mentioning the need for _PRW. Ah yes checking for that is probable even better. We probably need to add some ACPI helper for that though. Regards, hans
diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c index 47af62c12267..58f056ee0747 100644 --- a/drivers/input/mouse/elan_i2c_core.c +++ b/drivers/input/mouse/elan_i2c_core.c @@ -33,6 +33,7 @@ #include <linux/jiffies.h> #include <linux/completion.h> #include <linux/of.h> +#include <linux/pm_wakeirq.h> #include <linux/property.h> #include <linux/regulator/consumer.h> #include <asm/unaligned.h> @@ -86,8 +87,6 @@ struct elan_tp_data { u16 fw_page_size; u32 fw_signature_address; - bool irq_wake; - u8 min_baseline; u8 max_baseline; bool baseline_ready; @@ -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)) { device_init_wakeup(dev, true); + dev_pm_set_wake_irq(dev, client->irq); + } return 0; } @@ -1394,13 +1395,10 @@ static int __maybe_unused elan_suspend(struct device *dev) disable_irq(client->irq); - if (device_may_wakeup(dev)) { + if (device_may_wakeup(dev)) ret = elan_sleep(data); - /* Enable wake from IRQ */ - data->irq_wake = (enable_irq_wake(client->irq) == 0); - } else { + else ret = elan_disable_power(data); - } mutex_unlock(&data->sysfs_mutex); return ret; @@ -1412,11 +1410,6 @@ static int __maybe_unused elan_resume(struct device *dev) struct elan_tp_data *data = i2c_get_clientdata(client); int error; - if (device_may_wakeup(dev) && data->irq_wake) { - disable_irq_wake(client->irq); - data->irq_wake = false; - } - error = elan_enable_power(data); if (error) { dev_err(dev, "power up when resuming failed: %d\n", error);
The Elan I2C touchpad driver is currently manually managing the wake IRQ. When a device is managed by device tree or ACPI it is expected that those subsystems manage defining the wake pin and manage enabling it. This change removes the explicit enable_irq_wake/disable_irq_wake and relies on the PM subsystem. See device_wakeup_arm_wake_irqs. It also registers the IRQ using dev_pm_set_wake_irq only in the case where the device isn't DT or ACPI managed. This should preserve the existing behavior. I'm not sure how we actually get into this state though. I tested this with an ACPI device that has a `_PRW` method that uses a GPE was the wake event. I no longer see the GPIO controller being called to enable the wake pin. This results in the GPE correctly being marked as the wake source. Signed-off-by: Raul E Rangel <rrangel@chromium.org> --- drivers/input/mouse/elan_i2c_core.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)