diff mbox series

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

Message ID 20230601131739.300760-22-michal.wilczynski@intel.com
State New
Headers show
Series [v4,01/35] acpi: Adjust functions installing bus event handlers | expand

Commit Message

Michal Wilczynski June 1, 2023, 1:17 p.m. UTC
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(-)

Comments

Ilpo Järvinen June 2, 2023, 1:30 p.m. UTC | #1
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);
Michal Wilczynski June 2, 2023, 1:49 p.m. UTC | #2
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);
>
Ilpo Järvinen June 2, 2023, 1:55 p.m. UTC | #3
On Fri, 2 Jun 2023, Wilczynski, Michal wrote:
> 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.

You move the function in one patch without changing anything else. And 
then in another change that is this notify change you make the necessary 
adjustments to the moved function.

It makes both patches easier to review because one is a trivial copy and 
notify related changes are no longer mixed up with the copy.
diff mbox series

Patch

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);
 }
 
 /* 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;
+
 	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);
@@ -889,13 +909,16 @@  static void acpi_fujitsu_laptop_release(struct acpi_device *device)
 	}
 }
 
-static void acpi_fujitsu_laptop_notify(struct acpi_device *device, u32 event)
+static void acpi_fujitsu_laptop_notify(acpi_handle handle, u32 event, void *data)
 {
-	struct fujitsu_laptop *priv = acpi_driver_data(device);
+	struct acpi_device *device = data;
+	struct fujitsu_laptop *priv;
 	unsigned long flags;
 	int scancode, i = 0;
 	unsigned int irb;
 
+	priv = acpi_driver_data(device);
+
 	if (event != ACPI_FUJITSU_NOTIFY_CODE) {
 		acpi_handle_info(device->handle, "Unsupported event [0x%x]\n",
 				 event);
@@ -947,7 +970,7 @@  static struct acpi_driver acpi_fujitsu_bl_driver = {
 	.ids = fujitsu_bl_device_ids,
 	.ops = {
 		.add = acpi_fujitsu_bl_add,
-		.notify = acpi_fujitsu_bl_notify,
+		.remove = acpi_fujitsu_bl_remove,
 		},
 };
 
@@ -963,7 +986,6 @@  static struct acpi_driver acpi_fujitsu_laptop_driver = {
 	.ops = {
 		.add = acpi_fujitsu_laptop_add,
 		.remove = acpi_fujitsu_laptop_remove,
-		.notify = acpi_fujitsu_laptop_notify,
 		},
 };