| Message ID | 20230601131739.300760-12-michal.wilczynski@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Remove .notify callback in acpi_device_ops | expand |
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, > }, > }; >
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); 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); +} + +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, }, };
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> --- drivers/iio/light/acpi-als.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)