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

["Wilczynski, Michal" <michal.wilczynski@intel.com>]

On 6/2/2023 3:30 PM, Ilpo Järvinen wrote:
> 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.

Sure I can do that, however this moving around make sense in context of this patch as
it's necessary to compile with the new event installation method.

>
>>  }
>>  
>>  /* 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.

Will do that,
Thank you for your time !


>
>> +
>>  	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);
>