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

[Rafael J. Wysocki]

On Tue, Mar 17, 2026 at 6:48 PM Ilpo Järvinen
<ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
>
> 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?

I just preserved the existing pattern.  I can remove it, but that
would be a separate patch, wouldn't it?

> >       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.
>