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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 6513 bytes --]

On Tue, 17 Mar 2026, Rafael J. Wysocki wrote:

> On Tue, Mar 17, 2026 at 6:48 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Thu, 12 Mar 2026, Rafael J. Wysocki wrote:
> >
> > > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > >
> > > 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@intel.com>
> > > ---
> > >  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?

Yes, to own patch.

-- 
 i.