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/fujitsu-laptop.c | 86 +++++++++++++++++----------
> 1 file changed, 54 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 085e044e888e..001333edba9f 100644
> --- a/drivers/platform/x86/fujitsu-laptop.c
> +++ b/drivers/platform/x86/fujitsu-laptop.c
> @@ -136,6 +136,8 @@ struct fujitsu_laptop {
>
> static struct acpi_device *fext;
>
> +static void acpi_fujitsu_laptop_notify(acpi_handle handle, u32 event, void *data);
> +
> /* Fujitsu ACPI interface function */
>
> static int call_fext_func(struct acpi_device *device,
> @@ -382,6 +384,37 @@ static int fujitsu_backlight_register(struct acpi_device *device)
> return 0;
> }
>
> +static void acpi_fujitsu_bl_notify(acpi_handle handle, u32 event, void *data)
> +{
> + struct acpi_device *device = data;
> + struct fujitsu_bl *priv;
> + int oldb, newb;
> +
> + priv = acpi_driver_data(device);
> +
> + if (event != ACPI_FUJITSU_NOTIFY_CODE) {
> + acpi_handle_info(device->handle, "unsupported event [0x%x]\n",
> + event);
> + sparse_keymap_report_event(priv->input, -1, 1, true);
> + return;
> + }
> +
> + oldb = priv->brightness_level;
> + get_lcd_level(device);
> + newb = priv->brightness_level;
> +
> + acpi_handle_debug(device->handle,
> + "brightness button event [%i -> %i]\n", oldb, newb);
> +
> + if (oldb == newb)
> + return;
> +
> + if (!disable_brightness_adjust)
> + set_lcd_level(device, newb);
> +
> + sparse_keymap_report_event(priv->input, oldb < newb, 1, true);
> +}
> +
> static int acpi_fujitsu_bl_add(struct acpi_device *device)
> {
> struct fujitsu_bl *priv;
> @@ -410,37 +443,17 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device)
> if (ret)
> return ret;
>
> - return fujitsu_backlight_register(device);
> -}
> + ret = fujitsu_backlight_register(device);
> + if (ret)
> + return ret;
>
> -/* Brightness notify */
> + return acpi_device_install_event_handler(device, ACPI_DEVICE_NOTIFY,
> + acpi_fujitsu_bl_notify);
> +}
>
> -static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event)
> +static void acpi_fujitsu_bl_remove(struct acpi_device *device)
> {
> - struct fujitsu_bl *priv = acpi_driver_data(device);
> - int oldb, newb;
> -
> - if (event != ACPI_FUJITSU_NOTIFY_CODE) {
> - acpi_handle_info(device->handle, "unsupported event [0x%x]\n",
> - event);
> - sparse_keymap_report_event(priv->input, -1, 1, true);
> - return;
> - }
> -
> - oldb = priv->brightness_level;
> - get_lcd_level(device);
> - newb = priv->brightness_level;
> -
> - acpi_handle_debug(device->handle,
> - "brightness button event [%i -> %i]\n", oldb, newb);
> -
> - if (oldb == newb)
> - return;
> -
> - if (!disable_brightness_adjust)
> - set_lcd_level(device, newb);
> -
> - sparse_keymap_report_event(priv->input, oldb < newb, 1, true);
> + acpi_device_remove_event_handler(device, ACPI_DEVICE_NOTIFY, acpi_fujitsu_bl_notify);
Please move the function in a separate (no function changes intended)
patch to keep this patch on point.
> }
>
> /* ACPI device for hotkey handling */
> @@ -839,6 +852,11 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
> if (ret)
> goto err_free_fifo;
>
> + ret = acpi_device_install_event_handler(device, ACPI_DEVICE_NOTIFY,
> + acpi_fujitsu_laptop_notify);
> + if (ret)
> + goto err_free_fifo;
Here too, fujitsu_laptop_platform_remove() is necessary, goto + put it
into the rollback path.
Please go through your patches with these rollback corrections in mind,
I'll skip looking through the rest of platform/x86 patches until you've
done that.
> +
> return 0;
>
> err_free_fifo:
> @@ -851,6 +869,8 @@ static void acpi_fujitsu_laptop_remove(struct acpi_device *device)
> {
> struct fujitsu_laptop *priv = acpi_driver_data(device);
>
> + acpi_device_remove_event_handler(device, ACPI_DEVICE_NOTIFY, acpi_fujitsu_laptop_notify);
> +
> fujitsu_laptop_platform_remove(device);
>
> kfifo_free(&priv->fifo);
--
i.
