Re: [PATCH v4 11/35] iio/acpi-als: Move handler installing logic to driver

[Jonathan Cameron <jic23@kernel.org>]

On Thu,  1 Jun 2023 15:17:14 +0200
Michal Wilczynski <michal.wilczynski@intel.com> 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@intel.com>
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,
>  	},
>  };
>