Re: [PATCH v1 2/2] platform/x86: system76: Convert ACPI driver to a platform one

[Ilpo Järvinen]

On Thu, 12 Mar 2026, Rafael J. Wysocki wrote:

> From: "Rafael J. Wysocki" <rafael.j.wysocki@xxxxxxxxx>
> 
> In all cases in which a struct acpi_driver is used for binding a driver
> to an ACPI device object, a corresponding platform device is created by
> the ACPI core and that device is regarded as a proper representation of
> underlying hardware.  Accordingly, a struct platform_driver should be
> used by driver code to bind to that device.  There are multiple reasons
> why drivers should not bind directly to ACPI device objects [1].
> 
> Overall, it is better to bind drivers to platform devices than to their
> ACPI companions, so convert the System76 ACPI driver to a platform one.
> 
> After this change, the subordinate hwmon, input and LED class devices
> will be registered under the platform device used for driver binding
> instead of its ACPI companion.
> 
> While this is not expected to alter functionality, it changes sysfs
> layout and so it will be visible to user space.
> 
> Link: https://lore.kernel.org/all/2396510.ElGaqSPkdT@rafael.j.wysocki/ [1]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> ---
>  drivers/platform/x86/system76_acpi.c | 52 +++++++++++++++-------------
>  1 file changed, 27 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
> index 35e294a55fa3..b3a4254156ae 100644
> --- a/drivers/platform/x86/system76_acpi.c
> +++ b/drivers/platform/x86/system76_acpi.c
> @@ -18,6 +18,7 @@
>  #include <linux/leds.h>
>  #include <linux/module.h>
>  #include <linux/pci_ids.h>
> +#include <linux/platform_device.h>
>  #include <linux/power_supply.h>
>  #include <linux/sysfs.h>
>  #include <linux/types.h>
> @@ -670,16 +671,19 @@ static void system76_notify(acpi_handle handle, u32 event, void *context)
>  	}
>  }
>  
> -// Add a System76 ACPI device
> -static int system76_add(struct acpi_device *acpi_dev)
> +// Probe a System76 platform device
> +static int system76_probe(struct platform_device *pdev)
>  {
> +	struct acpi_device *acpi_dev = ACPI_COMPANION(&pdev->dev);
>  	struct system76_data *data;
>  	int err;
>  
> -	data = devm_kzalloc(&acpi_dev->dev, sizeof(*data), GFP_KERNEL);
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
> -	acpi_dev->driver_data = data;
> +
> +	platform_set_drvdata(pdev, data);
> +
>  	data->acpi_dev = acpi_dev;
>  
>  	// Some models do not run open EC firmware. Check for an ACPI method
> @@ -695,7 +699,7 @@ static int system76_add(struct acpi_device *acpi_dev)
>  	data->ap_led.brightness_set_blocking = ap_led_set;
>  	data->ap_led.max_brightness = 1;
>  	data->ap_led.default_trigger = "rfkill-none";
> -	err = devm_led_classdev_register(&acpi_dev->dev, &data->ap_led);
> +	err = devm_led_classdev_register(&pdev->dev, &data->ap_led);
>  	if (err)
>  		return err;
>  
> @@ -739,19 +743,19 @@ static int system76_add(struct acpi_device *acpi_dev)
>  	}
>  
>  	if (data->kbled_type != KBLED_NONE) {
> -		err = devm_led_classdev_register(&acpi_dev->dev, &data->kb_led);
> +		err = devm_led_classdev_register(&pdev->dev, &data->kb_led);
>  		if (err)
>  			return err;
>  	}
>  
> -	data->input = devm_input_allocate_device(&acpi_dev->dev);
> +	data->input = devm_input_allocate_device(&pdev->dev);
>  	if (!data->input)
>  		return -ENOMEM;
>  
>  	data->input->name = "System76 ACPI Hotkeys";
>  	data->input->phys = "system76_acpi/input0";
>  	data->input->id.bustype = BUS_HOST;
> -	data->input->dev.parent = &acpi_dev->dev;
> +	data->input->dev.parent = &pdev->dev;
>  	input_set_capability(data->input, EV_KEY, KEY_SCREENLOCK);
>  
>  	err = input_register_device(data->input);
> @@ -767,7 +771,7 @@ static int system76_add(struct acpi_device *acpi_dev)
>  		if (err)
>  			goto error;
>  
> -		data->therm = devm_hwmon_device_register_with_info(&acpi_dev->dev,
> +		data->therm = devm_hwmon_device_register_with_info(&pdev->dev,
>  			"system76_acpi", data, &thermal_chip_info, NULL);
>  		err = PTR_ERR_OR_ZERO(data->therm);
>  		if (err)
> @@ -795,14 +799,13 @@ static int system76_add(struct acpi_device *acpi_dev)
>  	return err;
>  }
>  
> -// Remove a System76 ACPI device
> -static void system76_remove(struct acpi_device *acpi_dev)
> +// Remove a System76 platform device
> +static void system76_remove(struct platform_device *pdev)
>  {
> -	struct system76_data *data;
> -
> -	acpi_dev_remove_notify_handler(acpi_dev, ACPI_DEVICE_NOTIFY, system76_notify);
> +	struct system76_data *data = platform_get_drvdata(pdev);
>  
> -	data = acpi_driver_data(acpi_dev);
> +	acpi_dev_remove_notify_handler(ACPI_COMPANION(&pdev->dev),
> +				       ACPI_DEVICE_NOTIFY, system76_notify);
>  
>  	if (data->has_open_ec) {
>  		system76_battery_exit();
> @@ -810,22 +813,21 @@ static void system76_remove(struct acpi_device *acpi_dev)
>  		kfree(data->ntmp);
>  	}
>  
> -	devm_led_classdev_unregister(&acpi_dev->dev, &data->ap_led);
> -	devm_led_classdev_unregister(&acpi_dev->dev, &data->kb_led);
> +	devm_led_classdev_unregister(&pdev->dev, &data->ap_led);
> +	devm_led_classdev_unregister(&pdev->dev, &data->kb_led);

With devm_* being used, why are these needed?

>  	system76_get(data, "FINI");
>  }
>  
> -static struct acpi_driver system76_driver = {
> -	.name = "System76 ACPI Driver",
> -	.class = "hotkey",
> -	.ids = device_ids,
> -	.ops = {
> -		.add = system76_add,
> -		.remove = system76_remove,
> +static struct platform_driver system76_driver = {
> +	.probe = system76_probe,
> +	.remove = system76_remove,
> +	.driver = {
> +		.name = "System76 ACPI Driver",
> +		.acpi_match_table = device_ids,
>  	},
>  };
> -module_acpi_driver(system76_driver);
> +module_platform_driver(system76_driver);
>  
>  MODULE_DESCRIPTION("System76 ACPI Driver");
>  MODULE_AUTHOR("Jeremy Soller <jeremy@xxxxxxxxxxxx>");
> 

-- 
 i.