Re: [PATCH v1 2/3] platform/x86: panasonic-laptop: Register ACPI notify handler directly

[Rafael J. Wysocki]

On Tue, Mar 17, 2026 at 6:35 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>
> >
> > To facilitate subsequent conversion of the driver to a platform one,
> > make it install an ACPI notify handler directly instead of using
> > a .notify() callback in struct acpi_driver.
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > ---
> >  drivers/platform/x86/panasonic-laptop.c | 20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
> > index 64195ff0f40e..4bd83b987761 100644
> > --- a/drivers/platform/x86/panasonic-laptop.c
> > +++ b/drivers/platform/x86/panasonic-laptop.c
> > @@ -185,7 +185,7 @@ enum SINF_BITS { SINF_NUM_BATTERIES = 0,
> >
> >  static int acpi_pcc_hotkey_add(struct acpi_device *device);
> >  static void acpi_pcc_hotkey_remove(struct acpi_device *device);
> > -static void acpi_pcc_hotkey_notify(struct acpi_device *device, u32 event);
> > +static void acpi_pcc_hotkey_notify(acpi_handle handle, u32 event, void *data);
> >
> >  static const struct acpi_device_id pcc_device_ids[] = {
> >       { "MAT0012", 0},
> > @@ -208,7 +208,6 @@ static struct acpi_driver acpi_pcc_driver = {
> >       .ops =          {
> >                               .add =          acpi_pcc_hotkey_add,
> >                               .remove =       acpi_pcc_hotkey_remove,
> > -                             .notify =       acpi_pcc_hotkey_notify,
> >                       },
> >       .drv.pm =       &acpi_pcc_hotkey_pm,
> >  };
> > @@ -869,9 +868,9 @@ static void acpi_pcc_generate_keyinput(struct pcc_acpi *pcc)
> >               pr_err("Unknown hotkey event: 0x%04llx\n", result);
> >  }
> >
> > -static void acpi_pcc_hotkey_notify(struct acpi_device *device, u32 event)
> > +static void acpi_pcc_hotkey_notify(acpi_handle handle, u32 event, void *data)
> >  {
> > -     struct pcc_acpi *pcc = acpi_driver_data(device);
> > +     struct pcc_acpi *pcc = data;
> >
> >       switch (event) {
> >       case HKEY_NOTIFY:
> > @@ -1073,13 +1072,18 @@ static int acpi_pcc_hotkey_add(struct acpi_device *device)
> >       if (result)
> >               goto out_backlight;
> >
> > +     result = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY,
> > +                                              acpi_pcc_hotkey_notify, pcc);
> > +     if (result)
> > +             goto out_sysfs;
> > +
> >       /* optical drive initialization */
> >       if (ACPI_SUCCESS(check_optd_present())) {
> >               pcc->platform = platform_device_register_simple("panasonic",
> >                       PLATFORM_DEVID_NONE, NULL, 0);
> >               if (IS_ERR(pcc->platform)) {
> >                       result = PTR_ERR(pcc->platform);
> > -                     goto out_sysfs;
> > +                     goto out_notify_handler;
> >               }
> >               result = device_create_file(&pcc->platform->dev,
> >                       &dev_attr_cdpower);
> > @@ -1095,6 +1099,9 @@ static int acpi_pcc_hotkey_add(struct acpi_device *device)
> >
> >  out_platform:
> >       platform_device_unregister(pcc->platform);
> > +out_notify_handler:
> > +     acpi_dev_remove_notify_handler(device, ACPI_DEVICE_NOTIFY,
> > +                                    acpi_pcc_hotkey_notify);
> >  out_sysfs:
> >       sysfs_remove_group(&device->dev.kobj, &pcc_attr_group);
> >  out_backlight:
> > @@ -1123,6 +1130,9 @@ static void acpi_pcc_hotkey_remove(struct acpi_device *device)
> >
> >       sysfs_remove_group(&device->dev.kobj, &pcc_attr_group);
> >
> > +     acpi_dev_remove_notify_handler(device, ACPI_DEVICE_NOTIFY,
> > +                                    acpi_pcc_hotkey_notify);
> > +
>
> This diverges from the setup order. Is that intentional? (I can see the
> current order is not exactly reverse of the setup order but it still
> looks it's possible to place the added remove call better than here.)
>
> This driver also likely has pre-existing rollback problems related to
> pcc_register_optd_notifier/pcc_unregister_optd_notifier().

So it would be good to fix them while at it.

OK, I'll have a look.

> >       backlight_device_unregister(pcc->backlight);
> >
> >       input_unregister_device(pcc->input_dev);
> >
>
> --
>  i.
>