Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq

On Tue, Sep 06, 2022 at 11:18:49AM -0600, Raul Rangel wrote:

On Fri, Sep 2, 2022 at 11:07 PM Dmitry Torokhov
[off-list ref] wrote:

quoted

On Wed, Aug 31, 2022 at 08:17:23PM -0600, Raul Rangel wrote:

quoted

On Wed, Aug 31, 2022 at 1:16 PM Dmitry Torokhov
[off-list ref] wrote:

quoted

On Wed, Aug 31, 2022 at 12:12:41PM -0700, Dmitry Torokhov wrote:

quoted

On Wed, Aug 31, 2022 at 08:01:12PM +0200, Rafael J. Wysocki wrote:

quoted

On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel [off-list ref] wrote:

quoted

The Elan I2C touchpad driver is currently manually managing the wake
IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
and instead relies on the PM subsystem. This is done by calling
dev_pm_set_wake_irq.

i2c_device_probe already calls dev_pm_set_wake_irq when using device
tree, so it's only required when using ACPI. The net result is that this
change should be a no-op. i2c_device_remove also already calls
dev_pm_clear_wake_irq, so we don't need to do that in this driver.

I tested this on an ACPI system where the touchpad doesn't have _PRW
defined. I verified I can still wake the system and that the wake source
was the touchpad IRQ GPIO.

Signed-off-by: Raul E Rangel <redacted>

I like this a lot [...]

quoted

quoted

I also like this a lot, but this assumes that firmware has correct
settings for the interrupt... Unfortunately it is not always the case
and I see that at least Chrome OS devices, such as glados line (cave, chell, sentry,
ect) do not mark interrupt as wakeup:

src/mainboard/google/glados/variants/chell/overridetree.cb

                        chip drivers/i2c/generic
                                register "hid" = ""ELAN0000""
                                register "desc" = ""ELAN Touchpad""
                                register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)"
                                register "wake" = "GPE0_DW0_05"
                                device i2c 15 on end

So the above entry specifies the `wake` register. This generates an
ACPI _PRW resource. The patch series will actually fix devices like
this. Today without this patch series we get two wake events for a
device. The ACPI wake GPE specified by the _PRW resource, and the
erroneous GPIO wake event. But you bring up a good point.

quoted

Does this mean that the example that we currently have in coreboot
documentation (Documentation/acpi/devicetree.md) is not correct:

device pci 15.0 on
        chip drivers/i2c/generic
                register "hid" = ""ELAN0000""
                register "desc" = ""ELAN Touchpad""
                register "irq" = "ACPI_IRQ_WAKE_LEVEL_LOW(GPP_A21_IRQ)"
                register "wake" = "GPE0_DW0_21"
                device i2c 15 on end
        end
end # I2C #0

Doesn't in say that we have both GpioIrq and GPE wakeup methods defined
for the same device?

Hrmm, yeah that is wrong and will cause duplicate wake events for the
device. I'll push a CL to clean up the documentation.

Thanks. I think we also need to clean up our ADL boards (and likely
more).

quoted

quoted

I wrote a quick and dirty script (https://0paste.com/391849) to parse
the coreboot device tree entries. Open source firmware is great isn't
it? ;)

$ find src/mainboard/google/ -iname '*.cb' | xargs awk -f touch.awk --
src/mainboard/google/eve/devicetree.cb

...

quoted

src/mainboard/google/sarien/variants/sarien/devicetree.cb
1
chip drivers/i2c/generic
register "hid" = ""ELAN0000""
register "desc" = ""ELAN Touchpad""
register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)"
register "probed" = "1"
device i2c 2c on end
end
Total Touchpad: 202
Total Wake: 195

Out of all the touchpads defined on ChromeOS it looks like only 4
devices are missing a wake declaration. I omitted touchpanels because
ChromeOS doesn't use those as a wake source. chromeos_laptop.c already
defines some devices with i2c board_info and it sets the
`I2C_CLIENT_WAKE` flag. I'm not sure if this is actually working as
expected. `i2c_device_probe` requires a `wakeup` irq to be present in
the device tree if the `I2C_CLIENT_WAKE` flag is set, but I'm assuming

No it does not. If there is no wakeup IRQ defined of_irq_get_byname()
will return an error and we'll take the "else if (client->irq > 0)"
branch and will set up client->irq as the wakeup irq.

quoted

the device tree was missing wake attributes.

Oh thanks for pointing that out. I might refactor patch #4 to just set
the `I2C_CLIENT_WAKE` flag when `acpi_wake_capable` is true.

quoted

quoted

Anyway, patches 6, and 7 are the ones that drop the legacy behavior. I
can figure out how to add the above boards to chromeos_laptop.c and
get the wake attribute plumbed, or I can add something directly to the
elan_i2c_core, etc so others can add overrides for their boards there.
I'll also send out CLs to fix the device tree configs (not that we
would run a FW qual just for this change).

My preference is to limit board-specific hacks in drivers if we can, so
adding missing properties to chromeos_laptop.c would be my preference.

How should we handle non chromeos boards?

My preference would be to shove something like chromeos_laptop into
drivers/platform/x86... Something like
drivers/platform/x86/x86-android-tablets.c

Thanks.

-- 
Dmitry