On Thu, 1 Jun 2023, Michal Wilczynski 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>
> ---
> drivers/platform/x86/asus-wireless.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wireless.c b/drivers/platform/x86/asus-wireless.c
> index abf01e00b799..6544e3419ae4 100644
> --- a/drivers/platform/x86/asus-wireless.c
> +++ b/drivers/platform/x86/asus-wireless.c
> @@ -108,19 +108,22 @@ static void led_state_set(struct led_classdev *led, enum led_brightness value)
> queue_work(data->wq, &data->led_work);
> }
>
> -static void asus_wireless_notify(struct acpi_device *adev, u32 event)
> +static void asus_wireless_notify(acpi_handle handle, u32 event, void *data)
> {
> - struct asus_wireless_data *data = acpi_driver_data(adev);
> + struct asus_wireless_data *w_data;
> + struct acpi_device *adev = data;
> +
> + w_data = acpi_driver_data(adev);
>
> dev_dbg(&adev->dev, "event=%#x\n", event);
> if (event != 0x88) {
> dev_notice(&adev->dev, "Unknown ASHS event: %#x\n", event);
> return;
> }
> - input_report_key(data->idev, KEY_RFKILL, 1);
> - input_sync(data->idev);
> - input_report_key(data->idev, KEY_RFKILL, 0);
> - input_sync(data->idev);
> + input_report_key(w_data->idev, KEY_RFKILL, 1);
> + input_sync(w_data->idev);
> + input_report_key(w_data->idev, KEY_RFKILL, 0);
> + input_sync(w_data->idev);
> }
>
> static int asus_wireless_add(struct acpi_device *adev)
> @@ -169,16 +172,20 @@ static int asus_wireless_add(struct acpi_device *adev)
> data->led.max_brightness = 1;
> data->led.default_trigger = "rfkill-none";
> err = devm_led_classdev_register(&adev->dev, &data->led);
> - if (err)
> + if (err) {
> destroy_workqueue(data->wq);
> + return err;
> + }
>
> - return err;
> + return acpi_device_install_event_handler(adev, ACPI_DEVICE_NOTIFY, asus_wireless_notify);
So if acpi_device_install_event_handler() returns error, should you
rollback something here like the error branch above? If that's the case,
use goto to rollback as you'll have two places calling destroy_workqueue()
already.
--
i.
> }
>
> static void asus_wireless_remove(struct acpi_device *adev)
> {
> struct asus_wireless_data *data = acpi_driver_data(adev);
>
> + acpi_device_remove_event_handler(adev, ACPI_DEVICE_NOTIFY, asus_wireless_notify);
> +
> if (data->wq) {
> devm_led_classdev_unregister(&adev->dev, &data->led);
> destroy_workqueue(data->wq);
> @@ -192,7 +199,6 @@ static struct acpi_driver asus_wireless_driver = {
> .ops = {
> .add = asus_wireless_add,
> .remove = asus_wireless_remove,
> - .notify = asus_wireless_notify,
> },
> };
> module_acpi_driver(asus_wireless_driver);
>
