On Mon, Sep 25, 2023 at 6:31 PM Michal Wilczynski <michal.wilczynski@xxxxxxxxx> wrote: > > acpi_dev_install_notify_handler() and acpi_dev_remove_notify_handler() > are wrappers around ACPICA installers. They are meant to save some > duplicated code from drivers. However as we're moving towards drivers > operating on platform_device they become a bit inconvenient to use as > inside the driver code we mostly want to use driver data of platform > device instead of ACPI device. That's fair enough, but -> > Make notify handlers installer wrappers more generic, while still > saving some code that would be duplicated otherwise. > > Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx> > Signed-off-by: Michal Wilczynski <michal.wilczynski@xxxxxxxxx> > --- > > Notes: > So one solution could be to just replace acpi_device with > platform_device as an argument in those functions. However I don't > believe this is a correct solution, as it is very often the case that > drivers declare their own private structures which gets allocated during > the .probe() callback, and become the heart of the driver. When drivers > do that it makes much more sense to just pass the private structure > to the notify handler instead of forcing user to dance with the > platform_device or acpi_device. > > drivers/acpi/ac.c | 6 +++--- > drivers/acpi/acpi_video.c | 6 +++--- > drivers/acpi/battery.c | 6 +++--- > drivers/acpi/bus.c | 14 ++++++-------- > drivers/acpi/hed.c | 6 +++--- > drivers/acpi/nfit/core.c | 6 +++--- > drivers/acpi/thermal.c | 6 +++--- > include/acpi/acpi_bus.h | 9 ++++----- > 8 files changed, 28 insertions(+), 31 deletions(-) > > diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c > index 225dc6818751..0b245f9f7ec8 100644 > --- a/drivers/acpi/ac.c > +++ b/drivers/acpi/ac.c > @@ -256,8 +256,8 @@ static int acpi_ac_add(struct acpi_device *device) > ac->battery_nb.notifier_call = acpi_ac_battery_notify; > register_acpi_notifier(&ac->battery_nb); > > - result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, > - acpi_ac_notify); > + result = acpi_dev_install_notify_handler(device->handle, ACPI_ALL_NOTIFY, > + acpi_ac_notify, device); > if (result) > goto err_unregister; > > @@ -306,7 +306,7 @@ static void acpi_ac_remove(struct acpi_device *device) > > ac = acpi_driver_data(device); > > - acpi_dev_remove_notify_handler(device, ACPI_ALL_NOTIFY, > + acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, > acpi_ac_notify); > power_supply_unregister(ac->charger); > unregister_acpi_notifier(&ac->battery_nb); > diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c > index 948e31f7ce6e..025c17890127 100644 > --- a/drivers/acpi/acpi_video.c > +++ b/drivers/acpi/acpi_video.c > @@ -2059,8 +2059,8 @@ static int acpi_video_bus_add(struct acpi_device *device) > > acpi_video_bus_add_notify_handler(video); > > - error = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY, > - acpi_video_bus_notify); > + error = acpi_dev_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, > + acpi_video_bus_notify, device); > if (error) > goto err_remove; > > @@ -2092,7 +2092,7 @@ static void acpi_video_bus_remove(struct acpi_device *device) > > video = acpi_driver_data(device); > > - acpi_dev_remove_notify_handler(device, ACPI_DEVICE_NOTIFY, > + acpi_dev_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, > acpi_video_bus_notify); > > mutex_lock(&video_list_lock); > diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c > index 969bf81e8d54..45dae32a8646 100644 > --- a/drivers/acpi/battery.c > +++ b/drivers/acpi/battery.c > @@ -1213,8 +1213,8 @@ static int acpi_battery_add(struct acpi_device *device) > > device_init_wakeup(&device->dev, 1); > > - result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, > - acpi_battery_notify); > + result = acpi_dev_install_notify_handler(device->handle, ACPI_ALL_NOTIFY, > + acpi_battery_notify, device); > if (result) > goto fail_pm; > > @@ -1241,7 +1241,7 @@ static void acpi_battery_remove(struct acpi_device *device) > > battery = acpi_driver_data(device); > > - acpi_dev_remove_notify_handler(device, ACPI_ALL_NOTIFY, > + acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, > acpi_battery_notify); > > device_init_wakeup(&device->dev, 0); > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c > index f41dda2d3493..479fe888d629 100644 > --- a/drivers/acpi/bus.c > +++ b/drivers/acpi/bus.c > @@ -554,14 +554,13 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device, > acpi_os_wait_events_complete(); > } > > -int acpi_dev_install_notify_handler(struct acpi_device *adev, > - u32 handler_type, > - acpi_notify_handler handler) > +int acpi_dev_install_notify_handler(acpi_handle handle, u32 handler_type, > + acpi_notify_handler handler, void *context) > { > acpi_status status; > > - status = acpi_install_notify_handler(adev->handle, handler_type, > - handler, adev); > + status = acpi_install_notify_handler(handle, handler_type, > + handler, context); The wrapper now takes exactly the same arguments as the wrapped function, so what exactly is the point of having it? The return value type? > if (ACPI_FAILURE(status)) > return -ENODEV; > > @@ -569,11 +568,10 @@ int acpi_dev_install_notify_handler(struct acpi_device *adev, > } > EXPORT_SYMBOL_GPL(acpi_dev_install_notify_handler); > > -void acpi_dev_remove_notify_handler(struct acpi_device *adev, > - u32 handler_type, > +void acpi_dev_remove_notify_handler(acpi_handle handle, u32 handler_type, > acpi_notify_handler handler) > { > - acpi_remove_notify_handler(adev->handle, handler_type, handler); > + acpi_remove_notify_handler(handle, handler_type, handler); > acpi_os_wait_events_complete(); Here at least there is the extra workqueues synchronization point. That said, why exactly is it better to use acpi_handle instead of a struct acpi_device pointer? Realistically, in a platform driver you'll need the latter to obtain the former anyway.