Re: [PATCH v4 21/35] platform/x86/fujitsu-laptop: Move handler installing logic to driver

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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@intel.com>
> ---
>  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.