On Thu, 1 Jun 2023 15:17:14 +0200
Michal Wilczynski <michal.wilczynski@xxxxxxxxx> wrote:
> Currently logic for installing notifications from ACPI devices is
> implemented using notify callback in struct acpi_driver. Preparations
> are being made to replace acpi_driver with more generic struct
> platform_driver, which doesn't contain notify callback. Furthermore
> as of now handlers are being called indirectly through
> acpi_notify_device(), which decreases performance.
>
> Call acpi_device_install_event_handler() at the end of .add() callback.
> Call acpi_device_remove_event_handler() at the beginning of .remove()
> callback. Change arguments passed to the notify callback to match with
> what's required by acpi_device_install_event_handler().
>
> Signed-off-by: Michal Wilczynski <michal.wilczynski@xxxxxxxxx>
Hi Michal,
Comments inline.
> ---
> drivers/iio/light/acpi-als.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
> index 2d91caf24dd0..5e200c6d91bc 100644
> --- a/drivers/iio/light/acpi-als.c
> +++ b/drivers/iio/light/acpi-als.c
> @@ -100,10 +100,14 @@ static int acpi_als_read_value(struct acpi_als *als, char *prop, s32 *val)
> return 0;
> }
>
> -static void acpi_als_notify(struct acpi_device *device, u32 event)
> +static void acpi_als_notify(acpi_handle handle, u32 event, void *data)
> {
> - struct iio_dev *indio_dev = acpi_driver_data(device);
> - struct acpi_als *als = iio_priv(indio_dev);
> + struct acpi_device *device = data;
> + struct iio_dev *indio_dev;
> + struct acpi_als *als;
> +
> + indio_dev = acpi_driver_data(device);
> + als = iio_priv(indio_dev);
Not particularly important, but I'd have kept to existing style
struct acpi_device *device = data;
struct iio_dev *indio_dev = acpi_driver_data(device);
struct acpi_als *als = iio_priv(indio_dev);
Less churn that way.
>
> if (iio_buffer_enabled(indio_dev) && iio_trigger_using_own(indio_dev)) {
> switch (event) {
> @@ -225,7 +229,16 @@ static int acpi_als_add(struct acpi_device *device)
> if (ret)
> return ret;
>
> - return devm_iio_device_register(dev, indio_dev);
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret)
> + return ret;
> +
> + return acpi_device_install_event_handler(device, ACPI_DEVICE_NOTIFY, acpi_als_notify);
Prefer to keep to a fully devm managed flow for removal
So use a devm_add_action_or_reset() to unwind this rather than adding a remove()
callback.
Obviously ordering is the same currently but that may change if this driver
is modified in future and it's a lot easier to get that right if fully devm
(or fully not).
Jonathan
> +}
> +
> +static void acpi_als_remove(struct acpi_device *device)
> +{
> + acpi_device_remove_event_handler(device, ACPI_DEVICE_NOTIFY, acpi_als_notify);
> }
>
> static const struct acpi_device_id acpi_als_device_ids[] = {
> @@ -241,7 +254,7 @@ static struct acpi_driver acpi_als_driver = {
> .ids = acpi_als_device_ids,
> .ops = {
> .add = acpi_als_add,
> - .notify = acpi_als_notify,
> + .remove = acpi_als_remove,
> },
> };
>
