Re: [PATCH] platform/x86: Add Driver to set up lid GPEs on MS Surface device

From: Maximilian Luz
Date: Tue Sep 08 2020 - 16:19:33 EST

On 9/8/20 8:40 PM, Andy Shevchenko wrote:
On Tue, Sep 8, 2020 at 8:20 PM Maximilian Luz <luzmaximilian@xxxxxxxxx> wrote:


+ .gpe_number = 0x17,
+ .gpe_number = 0x4D,
+ .gpe_number = 0x4F,
+ .gpe_number = 0x57,

From where these numbers come from? Can we get them from firmware (ACPI)?

Yes, they are obtained from ACPI/the DSDT. Specifically from the name of
the GPE handler notifying the lid device. See [1] for a repo full of
Surface ACPI dumps (source for this). I'll add a comment pointing this out
in v2.



+static int surface_gpe_probe(struct platform_device *pdev)
+ const struct surface_lid_device *lid;
+ int status;

+ lid = dev_get_platdata(&pdev->dev);
+ if (!lid)
+ return -ENODEV;

Can we use software nodes?

As far as I can tell this would work via fwnode_create_software_node /
fwnode_remove_software_node and device properties? I don't seem to find
much documentation on this (there doesn't seem to be an entry for
software nodes in the official docs?), but I think I should be able to
make this work.

+ status = acpi_mark_gpe_for_wake(NULL, lid->gpe_number);
+ if (status) {
+ dev_err(&pdev->dev, "failed to mark GPE for wake: %d\n", status);
+ return -EINVAL;
+ }

+ status = acpi_enable_gpe(NULL, lid->gpe_number);

Did I miss anything or all calls of enable / disable GPE are using
NULL as a first parameter? What the point in such case?

As far as I can tell, some of the more generic uses have a non-NULL
gpe_device parameter (acpi/device_pm.c, acpi/wakeup.c) and NULL just
means index-0/main device? Not an expert on that though, so probably
just ignore me here and let the ACPI guys answer this.



Can simply



Depends on your preference, really. That would also auto-load the module
on Surface Pro 3 and earlier devices (just won't do anything on those).
So it's a trade-off between unnecessary loading of the module and
maintainability/readability. Let me know what you prefer and I'll switch
to that.

Style and other issues are noted, I'll fix them for v2.