lkml.org 
[lkml]   [2021]   [Dec]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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:
    > > 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

    \
     
     \ /
      Last update: 2021-12-23 22:22    [W:2.262 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site