Hi
[...]
+static int surface_lid_enable_wakeup(struct device *dev, bool enable)
+{
+ const struct surface_lid_device *lid = dev_get_drvdata(dev);
+ int action = enable ? ACPI_GPE_ENABLE : ACPI_GPE_DISABLE;
+ acpi_status status;
+
+ status = acpi_set_gpe_wake_mask(NULL, lid->gpe_number, action);
+ if (status) {
I think 'if (ACPI_FAILURE(status))' would be better.
+ dev_err(dev, "failed to set GPE wake mask: %d\n", status);
I'm not sure if it's technically safe to print acpi_status with the %d format
specifier since 'acpi_status' is defined as 'u32' at the moment.
func("%lu", (unsigned long) status)
would be safer. You could also use 'acpi_format_exception()', which is possibly
the most correct approach since it assumes nothing about what 'acpi_status'
actually is.
+ return -EINVAL;
I'm not sure if -EINVAL is the best error to return here.
+ }
+
+ return 0;
+}
[...]
+static int surface_gpe_probe(struct platform_device *pdev)
+{
+ struct surface_lid_device *lid;
+ u32 gpe_number;
+ int status;
+
+ status = device_property_read_u32(&pdev->dev, "gpe", &gpe_number);
+ if (status)
+ return -ENODEV;
'device_property_read_u32()' returns an error code, you could simply return that
instead of hiding it.
+
+ status = acpi_mark_gpe_for_wake(NULL, gpe_number);
+ if (status) {
+ dev_err(&pdev->dev, "failed to mark GPE for wake: %d\n", status);
+ return -EINVAL;
+ }
+
+ status = acpi_enable_gpe(NULL, gpe_number);
+ if (status) {
+ dev_err(&pdev->dev, "failed to enable GPE: %d\n", status);
+ return -EINVAL;
+ }
My previous comments about ACPI and the returned value apply here as well.
Furthermore, 'acpi_mark_gpe_for_wake()' and 'acpi_enable_gpe()' both return
a value of type 'acpi_status', not 'int'.
+
+ lid = devm_kzalloc(&pdev->dev, sizeof(struct surface_lid_device),
+ GFP_KERNEL);
lid = devm_kzalloc(..., sizeof(*lid), ...)
is preferred.
+ if (!lid)
+ return -ENOMEM;
Isn't that problematic that the side effects of the previous two ACPI calls are
not undone when returning here with -ENOMEM? Allocating this struct right after
querying 'gpe_number' could prevent it.
+
+ lid->gpe_number = gpe_number;
+ platform_set_drvdata(pdev, lid);
+
+ status = surface_lid_enable_wakeup(&pdev->dev, false);
+ if (status) {
+ acpi_disable_gpe(NULL, gpe_number);
+ platform_set_drvdata(pdev, NULL);
Why is 'platform_set_drvdata(pdev, NULL)' needed?
+ return status;
+ }
+
+ return 0;
+}
+
+static int surface_gpe_remove(struct platform_device *pdev)
+{
+ struct surface_lid_device *lid = dev_get_drvdata(&pdev->dev);
+
+ /* restore default behavior without this module */
+ surface_lid_enable_wakeup(&pdev->dev, false);
+ acpi_disable_gpe(NULL, lid->gpe_number);
+
+ platform_set_drvdata(pdev, NULL);
I'm wondering why this is needed?
+ return 0;
+}
[...]
+static int __init surface_gpe_init(void)
+{
+ const struct dmi_system_id *match;
+ const struct property_entry *props;
+ struct platform_device *pdev;
+ struct fwnode_handle *fwnode;
+ int status;
+
+ match = dmi_first_match(dmi_lid_device_table);
+ if (!match) {
+ pr_info(KBUILD_MODNAME": no device detected, exiting\n");
If you put
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
before including any headers, you can simply write 'pr_info("no device...")' and it'll
be prefixed by the module name. This is the "usual" way of achieving what you want.
+ return 0;
Shouldn't it return -ENODEV?
+ }
+
+ props = match->driver_data;
+
+ status = platform_driver_register(&surface_gpe_driver);
+ if (status)
+ return status;
+
+ pdev = platform_device_alloc("surface_gpe", PLATFORM_DEVID_NONE);
+ if (!pdev) {
+ platform_driver_unregister(&surface_gpe_driver);
+ return -ENOMEM;
+ }
+
+ fwnode = fwnode_create_software_node(props, NULL);
+ if (IS_ERR(fwnode)) {
+ platform_device_put(pdev);
+ platform_driver_unregister(&surface_gpe_driver);
+ return PTR_ERR(fwnode);
+ }
+
+ pdev->dev.fwnode = fwnode;
+
+ status = platform_device_add(pdev);
+ if (status) {
+ platform_device_put(pdev);
+ platform_driver_unregister(&surface_gpe_driver);
+ return status;
+ }
+
It may be a matter of preference, but I think the 'if (err) goto X' pattern would
be better in this function (at least for the last 3 or so error paths).
+ surface_gpe_device = pdev;
+ return 0;
+}
+module_init(surface_gpe_init);
+
+static void __exit surface_gpe_exit(void)
+{
+ if (!surface_gpe_device)
+ return;
If you returned -ENODEV in init when no DMI match is found,
then this check would be redundant.