[RFT][PATCH v1 0/4] ACPI: Install notify or event handlers in ACPI button drivers

[RFT][PATCH v1 0/4] ACPI: Install notify or event handlers in ACPI button drivers

[Rafael J. Wysocki]

Hi Folks,

This series reworks the two ACPI button drivers to install notify or fixed
event handlers (depending on the device type) by themselves, so the core
code need not take the "fixed event handler for a button driver" case in
the general driver notify handler installer.

As a bonus, it separates the notify handler in the "generic" button driver
into a lid notify handler and a proper button notify handler.

It modifies also the core to drop the "fixed event handler for a button driver"
case from there (last patch).

It works for me, but I would appreciate as much testing of this as possible
especially on old hardware.

Obviously, it replaces

https://lore.kernel.org/linux-acpi/4500594.LvFx2qVVIh@kreacher/T/#m064d4130b57cbcb0ece1bb415fae1465ebc20f42

and its followers to some extent, but I think that it's better to deal with the
special case first and then take care of the rest.

Thanks!

[RFT][PATCH v1 1/4] ACPI: button: Eliminate the driver notify callback

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Rework the ACPI button driver to install notify handlers or fixed
event handlers for the devices it binds to by itself, reduce the
indentation level in its notify handler routine and drop its
notify callback.

This will allow acpi_device_install_notify_handler() and
acpi_device_remove_notify_handler() to be simplified going forward
and it will allow the driver to use different notify handlers for the
lid and for the power and sleep buttons.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/button.c |  140 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 96 insertions(+), 44 deletions(-)

Index: linux-pm/drivers/acpi/button.c
===================================================================
--- linux-pm.orig/drivers/acpi/button.c
+++ linux-pm/drivers/acpi/button.c
@@ -135,7 +135,6 @@ static const struct dmi_system_id dmi_li
 
 static int acpi_button_add(struct acpi_device *device);
 static void acpi_button_remove(struct acpi_device *device);
-static void acpi_button_notify(struct acpi_device *device, u32 event);
 
 #ifdef CONFIG_PM_SLEEP
 static int acpi_button_suspend(struct device *dev);
@@ -153,7 +152,6 @@ static struct acpi_driver acpi_button_dr
 	.ops = {
 		.add = acpi_button_add,
 		.remove = acpi_button_remove,
-		.notify = acpi_button_notify,
 	},
 	.drv.pm = &acpi_button_pm,
 };
@@ -409,45 +407,55 @@ static void acpi_lid_initialize_state(st
 	button->lid_state_initialized = true;
 }
 
-static void acpi_button_notify(struct acpi_device *device, u32 event)
+static void acpi_button_notify(acpi_handle handle, u32 event, void *data)
 {
-	struct acpi_button *button = acpi_driver_data(device);
+	struct acpi_device *device = data;
+	struct acpi_button *button;
 	struct input_dev *input;
+	int keycode;
 
-	switch (event) {
-	case ACPI_FIXED_HARDWARE_EVENT:
-		event = ACPI_BUTTON_NOTIFY_STATUS;
-		fallthrough;
-	case ACPI_BUTTON_NOTIFY_STATUS:
-		input = button->input;
-		if (button->type == ACPI_BUTTON_TYPE_LID) {
-			if (button->lid_state_initialized)
-				acpi_lid_update_state(device, true);
-		} else {
-			int keycode;
-
-			acpi_pm_wakeup_event(&device->dev);
-			if (button->suspended)
-				break;
-
-			keycode = test_bit(KEY_SLEEP, input->keybit) ?
-						KEY_SLEEP : KEY_POWER;
-			input_report_key(input, keycode, 1);
-			input_sync(input);
-			input_report_key(input, keycode, 0);
-			input_sync(input);
-
-			acpi_bus_generate_netlink_event(
-					device->pnp.device_class,
-					dev_name(&device->dev),
-					event, ++button->pushed);
-		}
-		break;
-	default:
+	if (event != ACPI_BUTTON_NOTIFY_STATUS) {
 		acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
 				  event);
-		break;
+		return;
+	}
+
+	button = acpi_driver_data(device);
+
+	if (button->type == ACPI_BUTTON_TYPE_LID) {
+		if (button->lid_state_initialized)
+			acpi_lid_update_state(device, true);
+
+		return;
 	}
+
+	acpi_pm_wakeup_event(&device->dev);
+
+	if (button->suspended)
+		return;
+
+	input = button->input;
+	keycode = test_bit(KEY_SLEEP, input->keybit) ? KEY_SLEEP : KEY_POWER;
+
+	input_report_key(input, keycode, 1);
+	input_sync(input);
+	input_report_key(input, keycode, 0);
+	input_sync(input);
+
+	acpi_bus_generate_netlink_event(device->pnp.device_class,
+					dev_name(&device->dev),
+					event, ++button->pushed);
+}
+
+static void acpi_button_notify_run(void *data)
+{
+	acpi_button_notify(NULL, ACPI_BUTTON_NOTIFY_STATUS, data);
+}
+
+static u32 acpi_button_event(void *data)
+{
+	acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_button_notify_run, data);
+	return ACPI_INTERRUPT_HANDLED;
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -492,8 +500,9 @@ static int acpi_button_add(struct acpi_d
 	struct acpi_button *button;
 	struct input_dev *input;
 	const char *hid = acpi_device_hid(device);
+	acpi_status status;
 	char *name, *class;
-	int error;
+	int error = 0;
 
 	if (!strcmp(hid, ACPI_BUTTON_HID_LID) &&
 	     lid_init_state == ACPI_BUTTON_LID_INIT_DISABLED)
@@ -535,12 +544,15 @@ static int acpi_button_add(struct acpi_d
 	} else {
 		pr_info("Unsupported hid [%s]\n", hid);
 		error = -ENODEV;
-		goto err_free_input;
 	}
 
-	error = acpi_button_add_fs(device);
-	if (error)
-		goto err_free_input;
+	if (!error)
+		error = acpi_button_add_fs(device);
+
+	if (error) {
+		input_free_device(input);
+		goto err_free_button;
+	}
 
 	snprintf(button->phys, sizeof(button->phys), "%s/button/input0", hid);
 
@@ -568,6 +580,30 @@ static int acpi_button_add(struct acpi_d
 	error = input_register_device(input);
 	if (error)
 		goto err_remove_fs;
+
+	switch (device->device_type) {
+	case ACPI_BUS_TYPE_POWER_BUTTON:
+		status = acpi_install_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
+							  acpi_button_event,
+							  device);
+		break;
+	case ACPI_BUS_TYPE_SLEEP_BUTTON:
+		status = acpi_install_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
+							  acpi_button_event,
+							  device);
+		break;
+	default:
+		status = acpi_install_notify_handler(device->handle,
+						     ACPI_DEVICE_NOTIFY,
+						     acpi_button_notify,
+						     device);
+		break;
+	}
+	if (ACPI_FAILURE(status)) {
+		error = -ENODEV;
+		goto err_input_unregister;
+	}
+
 	if (button->type == ACPI_BUTTON_TYPE_LID) {
 		/*
 		 * This assumes there's only one lid device, or if there are
@@ -580,11 +616,11 @@ static int acpi_button_add(struct acpi_d
 	pr_info("%s [%s]\n", name, acpi_device_bid(device));
 	return 0;
 
- err_remove_fs:
+err_input_unregister:
+	input_unregister_device(input);
+err_remove_fs:
 	acpi_button_remove_fs(device);
- err_free_input:
-	input_free_device(input);
- err_free_button:
+err_free_button:
 	kfree(button);
 	return error;
 }
@@ -593,6 +629,22 @@ static void acpi_button_remove(struct ac
 {
 	struct acpi_button *button = acpi_driver_data(device);
 
+	switch (device->device_type) {
+	case ACPI_BUS_TYPE_POWER_BUTTON:
+		acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
+						acpi_button_event);
+		break;
+	case ACPI_BUS_TYPE_SLEEP_BUTTON:
+		acpi_remove_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
+						acpi_button_event);
+		break;
+	default:
+		acpi_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
+					   acpi_button_notify);
+		break;
+	}
+	acpi_os_wait_events_complete();
+
 	acpi_button_remove_fs(device);
 	input_unregister_device(button->input);
 	kfree(button);

[RFT][PATCH v1 2/4] ACPI: button: Use different notify handlers for lid and buttons

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since the lid handling in acpi_button_notify() is special, introduce
acpi_lid_notify() specifically for handling lid notifications.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/button.c |   33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

Index: linux-pm/drivers/acpi/button.c
===================================================================
--- linux-pm.orig/drivers/acpi/button.c
+++ linux-pm/drivers/acpi/button.c
@@ -407,12 +407,10 @@ static void acpi_lid_initialize_state(st
 	button->lid_state_initialized = true;
 }
 
-static void acpi_button_notify(acpi_handle handle, u32 event, void *data)
+static void acpi_lid_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct acpi_device *device = data;
 	struct acpi_button *button;
-	struct input_dev *input;
-	int keycode;
 
 	if (event != ACPI_BUTTON_NOTIFY_STATUS) {
 		acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
@@ -421,16 +419,28 @@ static void acpi_button_notify(acpi_hand
 	}
 
 	button = acpi_driver_data(device);
+	if (!button->lid_state_initialized)
+		return;
 
-	if (button->type == ACPI_BUTTON_TYPE_LID) {
-		if (button->lid_state_initialized)
-			acpi_lid_update_state(device, true);
+	acpi_lid_update_state(device, true);
+}
 
+static void acpi_button_notify(acpi_handle handle, u32 event, void *data)
+{
+	struct acpi_device *device = data;
+	struct acpi_button *button;
+	struct input_dev *input;
+	int keycode;
+
+	if (event != ACPI_BUTTON_NOTIFY_STATUS) {
+		acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
+				  event);
 		return;
 	}
 
 	acpi_pm_wakeup_event(&device->dev);
 
+	button = acpi_driver_data(device);
 	if (button->suspended)
 		return;
 
@@ -497,6 +507,7 @@ static int acpi_lid_input_open(struct in
 
 static int acpi_button_add(struct acpi_device *device)
 {
+	acpi_notify_handler handler;
 	struct acpi_button *button;
 	struct input_dev *input;
 	const char *hid = acpi_device_hid(device);
@@ -526,17 +537,20 @@ static int acpi_button_add(struct acpi_d
 	if (!strcmp(hid, ACPI_BUTTON_HID_POWER) ||
 	    !strcmp(hid, ACPI_BUTTON_HID_POWERF)) {
 		button->type = ACPI_BUTTON_TYPE_POWER;
+		handler = acpi_button_notify;
 		strcpy(name, ACPI_BUTTON_DEVICE_NAME_POWER);
 		sprintf(class, "%s/%s",
 			ACPI_BUTTON_CLASS, ACPI_BUTTON_SUBCLASS_POWER);
 	} else if (!strcmp(hid, ACPI_BUTTON_HID_SLEEP) ||
 		   !strcmp(hid, ACPI_BUTTON_HID_SLEEPF)) {
 		button->type = ACPI_BUTTON_TYPE_SLEEP;
+		handler = acpi_button_notify;
 		strcpy(name, ACPI_BUTTON_DEVICE_NAME_SLEEP);
 		sprintf(class, "%s/%s",
 			ACPI_BUTTON_CLASS, ACPI_BUTTON_SUBCLASS_SLEEP);
 	} else if (!strcmp(hid, ACPI_BUTTON_HID_LID)) {
 		button->type = ACPI_BUTTON_TYPE_LID;
+		handler = acpi_lid_notify;
 		strcpy(name, ACPI_BUTTON_DEVICE_NAME_LID);
 		sprintf(class, "%s/%s",
 			ACPI_BUTTON_CLASS, ACPI_BUTTON_SUBCLASS_LID);
@@ -594,8 +608,7 @@ static int acpi_button_add(struct acpi_d
 		break;
 	default:
 		status = acpi_install_notify_handler(device->handle,
-						     ACPI_DEVICE_NOTIFY,
-						     acpi_button_notify,
+						     ACPI_DEVICE_NOTIFY, handler,
 						     device);
 		break;
 	}
@@ -640,7 +653,9 @@ static void acpi_button_remove(struct ac
 		break;
 	default:
 		acpi_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
-					   acpi_button_notify);
+					   button->type == ACPI_BUTTON_TYPE_LID ?
+					   	acpi_lid_notify :
+					   	acpi_button_notify);
 		break;
 	}
 	acpi_os_wait_events_complete();

[RFT][PATCH v1 3/4] ACPI: tiny-power-button: Eliminate the driver notify callback

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Rework the ACPI tiny-power-button driver to install a notify handler or
a fixed event handler for the device it binds to by itself and drop its
notify callback.

This will allow acpi_device_install_notify_handler() and
acpi_device_remove_notify_handler() to be simplified going forward.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/tiny-power-button.c |   49 ++++++++++++++++++++++++++++++++-------
 1 file changed, 41 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/acpi/tiny-power-button.c
===================================================================
--- linux-pm.orig/drivers/acpi/tiny-power-button.c
+++ linux-pm/drivers/acpi/tiny-power-button.c
@@ -19,18 +19,52 @@ static const struct acpi_device_id tiny_
 };
 MODULE_DEVICE_TABLE(acpi, tiny_power_button_device_ids);
 
-static int acpi_noop_add(struct acpi_device *device)
+static void acpi_tiny_power_button_notify(acpi_handle handle, u32 event, void *data)
 {
-	return 0;
+	kill_cad_pid(power_signal, 1);
 }
 
-static void acpi_noop_remove(struct acpi_device *device)
+static void acpi_tiny_power_button_notify_run(void *not_used)
 {
+	acpi_tiny_power_button_notify(NULL, ACPI_FIXED_HARDWARE_EVENT, NULL);
 }
 
-static void acpi_tiny_power_button_notify(struct acpi_device *device, u32 event)
+static u32 acpi_tiny_power_button_event(void *not_used)
 {
-	kill_cad_pid(power_signal, 1);
+	acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_tiny_power_button_notify_run, NULL);
+	return ACPI_INTERRUPT_HANDLED;
+}
+
+static int acpi_tiny_power_button_add(struct acpi_device *device)
+{
+	acpi_status status;
+
+	if (device->device_type == ACPI_BUS_TYPE_POWER_BUTTON) {
+		status = acpi_install_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
+							  acpi_tiny_power_button_event,
+							  NULL);
+	} else {
+		status = acpi_install_notify_handler(device->handle,
+						     ACPI_DEVICE_NOTIFY,
+						     acpi_tiny_power_button_notify,
+						     NULL);
+	}
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	return 0;
+}
+
+static void acpi_tiny_power_button_remove(struct acpi_device *device)
+{
+	if (device->device_type == ACPI_BUS_TYPE_POWER_BUTTON) {
+		acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
+						acpi_tiny_power_button_event);
+	} else {
+		acpi_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
+					   acpi_tiny_power_button_notify);
+	}
+	acpi_os_wait_events_complete();
 }
 
 static struct acpi_driver acpi_tiny_power_button_driver = {
@@ -38,9 +72,8 @@ static struct acpi_driver acpi_tiny_powe
 	.class = "tiny-power-button",
 	.ids = tiny_power_button_device_ids,
 	.ops = {
-		.add = acpi_noop_add,
-		.remove = acpi_noop_remove,
-		.notify = acpi_tiny_power_button_notify,
+		.add = acpi_tiny_power_button_add,
+		.remove = acpi_tiny_power_button_remove,
 	},
 };
 

[RFT][PATCH v1 4/4] ACPI: bus: Simplify installation and removal of notify callback

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Because the only drivers that cared about button fixed events take care
of those events by themselves now, eliminate the code related to them
from acpi_device_install_notify_handler() and
acpi_device_remove_notify_handler().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/bus.c |   53 +++++++++--------------------------------------------
 1 file changed, 9 insertions(+), 44 deletions(-)

Index: linux-pm/drivers/acpi/bus.c
===================================================================
--- linux-pm.orig/drivers/acpi/bus.c
+++ linux-pm/drivers/acpi/bus.c
@@ -530,65 +530,30 @@ static void acpi_notify_device(acpi_hand
 	acpi_drv->ops.notify(device, event);
 }
 
-static void acpi_notify_device_fixed(void *data)
-{
-	struct acpi_device *device = data;
-
-	/* Fixed hardware devices have no handles */
-	acpi_notify_device(NULL, ACPI_FIXED_HARDWARE_EVENT, device);
-}
-
-static u32 acpi_device_fixed_event(void *data)
-{
-	acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_notify_device_fixed, data);
-	return ACPI_INTERRUPT_HANDLED;
-}
-
 static int acpi_device_install_notify_handler(struct acpi_device *device,
 					      struct acpi_driver *acpi_drv)
 {
-	acpi_status status;
-
-	if (device->device_type == ACPI_BUS_TYPE_POWER_BUTTON) {
-		status =
-		    acpi_install_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
-						     acpi_device_fixed_event,
-						     device);
-	} else if (device->device_type == ACPI_BUS_TYPE_SLEEP_BUTTON) {
-		status =
-		    acpi_install_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
-						     acpi_device_fixed_event,
-						     device);
-	} else {
-		u32 type = acpi_drv->flags & ACPI_DRIVER_ALL_NOTIFY_EVENTS ?
+	u32 type = acpi_drv->flags & ACPI_DRIVER_ALL_NOTIFY_EVENTS ?
 				ACPI_ALL_NOTIFY : ACPI_DEVICE_NOTIFY;
+	acpi_status status;
 
-		status = acpi_install_notify_handler(device->handle, type,
-						     acpi_notify_device,
-						     device);
-	}
-
+	status = acpi_install_notify_handler(device->handle, type,
+					     acpi_notify_device, device);
 	if (ACPI_FAILURE(status))
 		return -EINVAL;
+
 	return 0;
 }
 
 static void acpi_device_remove_notify_handler(struct acpi_device *device,
 					      struct acpi_driver *acpi_drv)
 {
-	if (device->device_type == ACPI_BUS_TYPE_POWER_BUTTON) {
-		acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
-						acpi_device_fixed_event);
-	} else if (device->device_type == ACPI_BUS_TYPE_SLEEP_BUTTON) {
-		acpi_remove_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
-						acpi_device_fixed_event);
-	} else {
-		u32 type = acpi_drv->flags & ACPI_DRIVER_ALL_NOTIFY_EVENTS ?
+	u32 type = acpi_drv->flags & ACPI_DRIVER_ALL_NOTIFY_EVENTS ?
 				ACPI_ALL_NOTIFY : ACPI_DEVICE_NOTIFY;
 
-		acpi_remove_notify_handler(device->handle, type,
-					   acpi_notify_device);
-	}
+	acpi_remove_notify_handler(device->handle, type,
+				   acpi_notify_device);
+
 	acpi_os_wait_events_complete();
 }
 

Re: [RFT][PATCH v1 1/4] ACPI: button: Eliminate the driver notify callback

[Wilczynski, Michal]

On 6/4/2023 5:19 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Rework the ACPI button driver to install notify handlers or fixed
> event handlers for the devices it binds to by itself, reduce the
> indentation level in its notify handler routine and drop its
> notify callback.
>
> This will allow acpi_device_install_notify_handler() and
> acpi_device_remove_notify_handler() to be simplified going forward
> and it will allow the driver to use different notify handlers for the
> lid and for the power and sleep buttons.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/button.c |  140 ++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 96 insertions(+), 44 deletions(-)
>
> Index: linux-pm/drivers/acpi/button.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/button.c
> +++ linux-pm/drivers/acpi/button.c
> @@ -135,7 +135,6 @@ static const struct dmi_system_id dmi_li
>  
>  static int acpi_button_add(struct acpi_device *device);
>  static void acpi_button_remove(struct acpi_device *device);
> -static void acpi_button_notify(struct acpi_device *device, u32 event);
>  
>  #ifdef CONFIG_PM_SLEEP
>  static int acpi_button_suspend(struct device *dev);
> @@ -153,7 +152,6 @@ static struct acpi_driver acpi_button_dr
>  	.ops = {
>  		.add = acpi_button_add,
>  		.remove = acpi_button_remove,
> -		.notify = acpi_button_notify,
>  	},
>  	.drv.pm = &acpi_button_pm,
>  };
> @@ -409,45 +407,55 @@ static void acpi_lid_initialize_state(st
>  	button->lid_state_initialized = true;
>  }
>  
> -static void acpi_button_notify(struct acpi_device *device, u32 event)
> +static void acpi_button_notify(acpi_handle handle, u32 event, void *data)
>  {
> -	struct acpi_button *button = acpi_driver_data(device);
> +	struct acpi_device *device = data;
> +	struct acpi_button *button;
>  	struct input_dev *input;
> +	int keycode;
>  
> -	switch (event) {
> -	case ACPI_FIXED_HARDWARE_EVENT:
> -		event = ACPI_BUTTON_NOTIFY_STATUS;
> -		fallthrough;
> -	case ACPI_BUTTON_NOTIFY_STATUS:
> -		input = button->input;
> -		if (button->type == ACPI_BUTTON_TYPE_LID) {
> -			if (button->lid_state_initialized)
> -				acpi_lid_update_state(device, true);
> -		} else {
> -			int keycode;
> -
> -			acpi_pm_wakeup_event(&device->dev);
> -			if (button->suspended)
> -				break;
> -
> -			keycode = test_bit(KEY_SLEEP, input->keybit) ?
> -						KEY_SLEEP : KEY_POWER;
> -			input_report_key(input, keycode, 1);
> -			input_sync(input);
> -			input_report_key(input, keycode, 0);
> -			input_sync(input);
> -
> -			acpi_bus_generate_netlink_event(
> -					device->pnp.device_class,
> -					dev_name(&device->dev),
> -					event, ++button->pushed);
> -		}
> -		break;
> -	default:
> +	if (event != ACPI_BUTTON_NOTIFY_STATUS) {
>  		acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
>  				  event);
> -		break;
> +		return;
> +	}
> +
> +	button = acpi_driver_data(device);
> +
> +	if (button->type == ACPI_BUTTON_TYPE_LID) {
> +		if (button->lid_state_initialized)
> +			acpi_lid_update_state(device, true);
> +
> +		return;
>  	}
> +
> +	acpi_pm_wakeup_event(&device->dev);
> +
> +	if (button->suspended)
> +		return;
> +
> +	input = button->input;
> +	keycode = test_bit(KEY_SLEEP, input->keybit) ? KEY_SLEEP : KEY_POWER;
> +
> +	input_report_key(input, keycode, 1);
> +	input_sync(input);
> +	input_report_key(input, keycode, 0);
> +	input_sync(input);
> +
> +	acpi_bus_generate_netlink_event(device->pnp.device_class,
> +					dev_name(&device->dev),
> +					event, ++button->pushed);
> +}
> +
> +static void acpi_button_notify_run(void *data)
> +{
> +	acpi_button_notify(NULL, ACPI_BUTTON_NOTIFY_STATUS, data);
> +}
> +
> +static u32 acpi_button_event(void *data)
> +{
> +	acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_button_notify_run, data);
> +	return ACPI_INTERRUPT_HANDLED;
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
> @@ -492,8 +500,9 @@ static int acpi_button_add(struct acpi_d
>  	struct acpi_button *button;
>  	struct input_dev *input;
>  	const char *hid = acpi_device_hid(device);
> +	acpi_status status;
>  	char *name, *class;
> -	int error;
> +	int error = 0;
>  
>  	if (!strcmp(hid, ACPI_BUTTON_HID_LID) &&
>  	     lid_init_state == ACPI_BUTTON_LID_INIT_DISABLED)
> @@ -535,12 +544,15 @@ static int acpi_button_add(struct acpi_d
>  	} else {
>  		pr_info("Unsupported hid [%s]\n", hid);
>  		error = -ENODEV;
...
> -		goto err_free_input;
>  	}
>  
> -	error = acpi_button_add_fs(device);
> -	if (error)
> -		goto err_free_input;
> +	if (!error)
> +		error = acpi_button_add_fs(device);
> +
> +	if (error) {
> +		input_free_device(input);
> +		goto err_free_button;
> +	}

This logic is correct, just a bit weird to read and it's saving just one call to input_free_device().

>  
>  	snprintf(button->phys, sizeof(button->phys), "%s/button/input0", hid);
>  
> @@ -568,6 +580,30 @@ static int acpi_button_add(struct acpi_d
>  	error = input_register_device(input);
>  	if (error)
>  		goto err_remove_fs;
> +
> +	switch (device->device_type) {
> +	case ACPI_BUS_TYPE_POWER_BUTTON:
> +		status = acpi_install_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
> +							  acpi_button_event,
> +							  device);
> +		break;
> +	case ACPI_BUS_TYPE_SLEEP_BUTTON:
> +		status = acpi_install_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
> +							  acpi_button_event,
> +							  device);
> +		break;
> +	default:
> +		status = acpi_install_notify_handler(device->handle,
> +						     ACPI_DEVICE_NOTIFY,
> +						     acpi_button_notify,
> +						     device);
> +		break;
> +	}
> +	if (ACPI_FAILURE(status)) {
> +		error = -ENODEV;
> +		goto err_input_unregister;
> +	}
> +
>  	if (button->type == ACPI_BUTTON_TYPE_LID) {
>  		/*
>  		 * This assumes there's only one lid device, or if there are
> @@ -580,11 +616,11 @@ static int acpi_button_add(struct acpi_d
>  	pr_info("%s [%s]\n", name, acpi_device_bid(device));
>  	return 0;
>  
> - err_remove_fs:
> +err_input_unregister:
> +	input_unregister_device(input);
> +err_remove_fs:
>  	acpi_button_remove_fs(device);
> - err_free_input:
> -	input_free_device(input);
> - err_free_button:
> +err_free_button:
>  	kfree(button);
>  	return error;
>  }
> @@ -593,6 +629,22 @@ static void acpi_button_remove(struct ac
>  {
>  	struct acpi_button *button = acpi_driver_data(device);
>  
> +	switch (device->device_type) {
> +	case ACPI_BUS_TYPE_POWER_BUTTON:
> +		acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
> +						acpi_button_event);
> +		break;
> +	case ACPI_BUS_TYPE_SLEEP_BUTTON:
> +		acpi_remove_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
> +						acpi_button_event);
> +		break;
> +	default:
> +		acpi_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
> +					   acpi_button_notify);
> +		break;
> +	}
> +	acpi_os_wait_events_complete();
> +
>  	acpi_button_remove_fs(device);
>  	input_unregister_device(button->input);
>  	kfree(button);
>
>

Reviewed-by: Michal Wilczynski <michal.wilczynski@intel.com>