by Hans de Goede

[permalink] [raw]

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

Hi,

On 12/25/21 14:51, Kai-Heng Feng wrote:
> On Fri, Dec 24, 2021 at 7:11 PM Hans de Goede <[email protected]> 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/[email protected]/
>>>>
>>>> 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