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.
> [...]
Regards,
Barnabás Pőcze
