Re: [PATCH v1 1/9] ACPI: bus: Make notify wrappers more generic
From: Rafael J. Wysocki
Date: Wed Oct 04 2023 - 15:11:02 EST
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.