ACPI: button: minor cleanups

PATCH In review Bjorn Helgaas 3 participants 13 messages Apr 8, 2009 – May 11, 2009 2 messages not in the archive

Subsystems (2)

ACPI ACPI component architecture (acpica)

lore

Share & tools

lore

Bjorn Helgaas

author

2009-04-08 15:39 UTC #0 lore raw

cover ACPI: button: minor cleanups

These are minor cleanups - mostly whitespace and style. With the exception of the last, none should make any functional difference.

The last one does make a minor change in /proc "info" files: it removes the "(FF)" or "(CM)" annotation for power and sleep buttons. If anybody thinks that's important, I could rework that patch to keep the annotations while removing the other useless distinctions in the driver.

---

Bjorn Helgaas (6):
ACPI: button: remove control method/fixed hardware distinctions
ACPI: button: remove button->device pointer
ACPI: button: cache hid/name/class pointers
ACPI: button: use Linux style for getting driver_data
ACPI: button: remove unnecessary null pointer checks
ACPI: button: whitespace changes

 drivers/acpi/button.c |  140 +++++++++++++++----------------------------------
 1 files changed, 44 insertions(+), 96 deletions(-)

Signature · 1 line

Bjorn

Bjorn Helgaas

author

2009-04-08 15:39 UTC #1 lore raw

PATCH 1/6 ACPI: button: whitespace changes

This patch changes a bit of whitespace to follow Linux conventions.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/acpi/button.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

drivers/acpi/button.c +4 -6

index d73c94b..52eb06e 100644
@@ -1,5 +1,5 @@
 /*
- *  acpi_button.c - ACPI Button Driver ($Revision: 30 $)
+ *  button.c - ACPI Button Driver
  *
  *  Copyright (C) 2001, 2002 Andy Grover <[email protected]>
  *  Copyright (C) 2001, 2002 Paul Diefenbaugh <[email protected]>

@@ -133,7 +133,6 @@ static int acpi_button_info_seq_show(struct seq_file *seq, void *offset)
 
 	seq_printf(seq, "type:                    %s\n",
 		   acpi_device_name(button->device));
-
 	return 0;
 }

@@ -259,6 +258,7 @@ static int acpi_lid_send_state(struct acpi_button *button)
 					&state);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
+
 	/* input layer checks if event is redundant */
 	input_report_switch(button->input, SW_LID, !state);
 	input_sync(button->input);

@@ -299,13 +299,12 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
 				  "Unsupported event [0x%x]\n", event));
 		break;
 	}
-
-	return;
 }
 
 static int acpi_button_resume(struct acpi_device *device)
 {
 	struct acpi_button *button;
+
 	if (!device)
 		return -EINVAL;
 	button = acpi_driver_data(device);

@@ -424,7 +423,6 @@ static int acpi_button_add(struct acpi_device *device)
 
 	printk(KERN_INFO PREFIX "%s [%s]\n",
 	       acpi_device_name(device), acpi_device_bid(device));
-
 	return 0;
 
  err_remove_fs:

@@ -448,7 +446,6 @@ static int acpi_button_remove(struct acpi_device *device, int type)
 	acpi_button_remove_fs(device);
 	input_unregister_device(button->input);
 	kfree(button);
-
 	return 0;
 }

@@ -459,6 +456,7 @@ static int __init acpi_button_init(void)
 	acpi_button_dir = proc_mkdir(ACPI_BUTTON_CLASS, acpi_root_dir);
 	if (!acpi_button_dir)
 		return -ENODEV;
+
 	result = acpi_bus_register_driver(&acpi_button_driver);
 	if (result < 0) {
 		remove_proc_entry(ACPI_BUTTON_CLASS, acpi_root_dir);

Bjorn Helgaas

author

2009-04-08 15:39 UTC #2 lore raw

PATCH 2/6 ACPI: button: remove unnecessary null pointer checks

In reply to Bjorn Helgaas

Better to oops and learn about a bug than to silently cover it up.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/acpi/button.c |   22 +---------------------
 1 files changed, 1 insertions(+), 21 deletions(-)

drivers/acpi/button.c +1 ~~-21~~

index 52eb06e..c463236 100644
@@ -128,9 +128,6 @@ static int acpi_button_info_seq_show(struct seq_file *seq, void *offset)
 {
 	struct acpi_button *button = seq->private;
 
-	if (!button || !button->device)
-		return 0;
-
 	seq_printf(seq, "type:                    %s\n",
 		   acpi_device_name(button->device));
 	return 0;

@@ -147,9 +144,6 @@ static int acpi_button_state_seq_show(struct seq_file *seq, void *offset)
 	acpi_status status;
 	unsigned long long state;
 
-	if (!button || !button->device)
-		return 0;
-
 	status = acpi_evaluate_integer(button->device->handle, "_LID", NULL, &state);
 	seq_printf(seq, "state:      %s\n",
 		   ACPI_FAILURE(status) ? "unsupported" :

@@ -171,9 +165,6 @@ static int acpi_button_add_fs(struct acpi_device *device)
 	struct proc_dir_entry *entry = NULL;
 	struct acpi_button *button;
 
-	if (!device || !acpi_driver_data(device))
-		return -EINVAL;
-
 	button = acpi_driver_data(device);
 
 	switch (button->type) {

@@ -270,9 +261,6 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
 	struct acpi_button *button = acpi_driver_data(device);
 	struct input_dev *input;
 
-	if (!button || !button->device)
-		return;
-
 	switch (event) {
 	case ACPI_FIXED_HARDWARE_EVENT:
 		event = ACPI_BUTTON_NOTIFY_STATUS;

@@ -305,10 +293,8 @@ static int acpi_button_resume(struct acpi_device *device)
 {
 	struct acpi_button *button;
 
-	if (!device)
-		return -EINVAL;
 	button = acpi_driver_data(device);
-	if (button && button->type == ACPI_BUTTON_TYPE_LID)
+	if (button->type == ACPI_BUTTON_TYPE_LID)
 		return acpi_lid_send_state(button);
 	return 0;
 }

@@ -319,9 +305,6 @@ static int acpi_button_add(struct acpi_device *device)
 	struct acpi_button *button;
 	struct input_dev *input;
 
-	if (!device)
-		return -EINVAL;
-
 	button = kzalloc(sizeof(struct acpi_button), GFP_KERNEL);
 	if (!button)
 		return -ENOMEM;

@@ -438,9 +421,6 @@ static int acpi_button_remove(struct acpi_device *device, int type)
 {
 	struct acpi_button *button;
 
-	if (!device || !acpi_driver_data(device))
-		return -EINVAL;
-
 	button = acpi_driver_data(device);
 
 	acpi_button_remove_fs(device);

Bjorn Helgaas

author

2009-04-08 15:39 UTC #3 lore raw

PATCH 3/6 ACPI: button: use Linux style for getting driver_data

In reply to Bjorn Helgaas

It's typical and slightly more compact to look up the driver_data structure by initializing the automatic variable at its definition.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/acpi/button.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

drivers/acpi/button.c +4 -9

index c463236..8ef8f44 100644
@@ -162,10 +162,8 @@ static struct proc_dir_entry *acpi_lid_dir;
 
 static int acpi_button_add_fs(struct acpi_device *device)
 {
+	struct acpi_button *button = acpi_driver_data(device);
 	struct proc_dir_entry *entry = NULL;
-	struct acpi_button *button;
-
-	button = acpi_driver_data(device);
 
 	switch (button->type) {
 	case ACPI_BUTTON_TYPE_POWER:

@@ -291,9 +289,8 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
 
 static int acpi_button_resume(struct acpi_device *device)
 {
-	struct acpi_button *button;
+	struct acpi_button *button = acpi_driver_data(device);
 
-	button = acpi_driver_data(device);
 	if (button->type == ACPI_BUTTON_TYPE_LID)
 		return acpi_lid_send_state(button);
 	return 0;

@@ -301,9 +298,9 @@ static int acpi_button_resume(struct acpi_device *device)
 
 static int acpi_button_add(struct acpi_device *device)
 {
-	int error;
 	struct acpi_button *button;
 	struct input_dev *input;
+	int error;
 
 	button = kzalloc(sizeof(struct acpi_button), GFP_KERNEL);
 	if (!button)

@@ -419,9 +416,7 @@ static int acpi_button_add(struct acpi_device *device)
 
 static int acpi_button_remove(struct acpi_device *device, int type)
 {
-	struct acpi_button *button;
-
-	button = acpi_driver_data(device);
+	struct acpi_button *button = acpi_driver_data(device);
 
 	acpi_button_remove_fs(device);
 	input_unregister_device(button->input);

Bjorn Helgaas

author

2009-04-08 15:39 UTC #4 lore raw

PATCH 4/6 ACPI: button: cache hid/name/class pointers

In reply to Bjorn Helgaas

This patch adds temporaries to cache the acpi_device_hid(), acpi_device_name(), and acpi_device_class() pointers so we don't have to clutter the code with so many uses of those interfaces.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/acpi/button.c |   48 ++++++++++++++++++++++++------------------------
 1 files changed, 24 insertions(+), 24 deletions(-)

drivers/acpi/button.c +24 ~~-24~~

index 8ef8f44..9f6d2e6 100644
@@ -300,6 +300,7 @@ static int acpi_button_add(struct acpi_device *device)
 {
 	struct acpi_button *button;
 	struct input_dev *input;
+	char *hid, *name, *class;
 	int error;
 
 	button = kzalloc(sizeof(struct acpi_button), GFP_KERNEL);

@@ -315,40 +316,41 @@ static int acpi_button_add(struct acpi_device *device)
 		goto err_free_button;
 	}
 
+	hid = acpi_device_hid(device);
+	name = acpi_device_name(device);
+	class = acpi_device_class(device);
+
 	/*
 	 * Determine the button type (via hid), as fixed-feature buttons
 	 * need to be handled a bit differently than generic-space.
 	 */
-	if (!strcmp(acpi_device_hid(device), ACPI_BUTTON_HID_POWER)) {
+	if (!strcmp(hid, ACPI_BUTTON_HID_POWER)) {
 		button->type = ACPI_BUTTON_TYPE_POWER;
-		strcpy(acpi_device_name(device), ACPI_BUTTON_DEVICE_NAME_POWER);
-		sprintf(acpi_device_class(device), "%s/%s",
+		strcpy(name, ACPI_BUTTON_DEVICE_NAME_POWER);
+		sprintf(class, "%s/%s",
 			ACPI_BUTTON_CLASS, ACPI_BUTTON_SUBCLASS_POWER);
-	} else if (!strcmp(acpi_device_hid(device), ACPI_BUTTON_HID_POWERF)) {
+	} else if (!strcmp(hid, ACPI_BUTTON_HID_POWERF)) {
 		button->type = ACPI_BUTTON_TYPE_POWERF;
-		strcpy(acpi_device_name(device),
-		       ACPI_BUTTON_DEVICE_NAME_POWERF);
-		sprintf(acpi_device_class(device), "%s/%s",
+		strcpy(name, ACPI_BUTTON_DEVICE_NAME_POWERF);
+		sprintf(class, "%s/%s",
 			ACPI_BUTTON_CLASS, ACPI_BUTTON_SUBCLASS_POWER);
-	} else if (!strcmp(acpi_device_hid(device), ACPI_BUTTON_HID_SLEEP)) {
+	} else if (!strcmp(hid, ACPI_BUTTON_HID_SLEEP)) {
 		button->type = ACPI_BUTTON_TYPE_SLEEP;
-		strcpy(acpi_device_name(device), ACPI_BUTTON_DEVICE_NAME_SLEEP);
-		sprintf(acpi_device_class(device), "%s/%s",
+		strcpy(name, ACPI_BUTTON_DEVICE_NAME_SLEEP);
+		sprintf(class, "%s/%s",
 			ACPI_BUTTON_CLASS, ACPI_BUTTON_SUBCLASS_SLEEP);
-	} else if (!strcmp(acpi_device_hid(device), ACPI_BUTTON_HID_SLEEPF)) {
+	} else if (!strcmp(hid, ACPI_BUTTON_HID_SLEEPF)) {
 		button->type = ACPI_BUTTON_TYPE_SLEEPF;
-		strcpy(acpi_device_name(device),
-		       ACPI_BUTTON_DEVICE_NAME_SLEEPF);
-		sprintf(acpi_device_class(device), "%s/%s",
+		strcpy(name, ACPI_BUTTON_DEVICE_NAME_SLEEPF);
+		sprintf(class, "%s/%s",
 			ACPI_BUTTON_CLASS, ACPI_BUTTON_SUBCLASS_SLEEP);
-	} else if (!strcmp(acpi_device_hid(device), ACPI_BUTTON_HID_LID)) {
+	} else if (!strcmp(hid, ACPI_BUTTON_HID_LID)) {
 		button->type = ACPI_BUTTON_TYPE_LID;
-		strcpy(acpi_device_name(device), ACPI_BUTTON_DEVICE_NAME_LID);
-		sprintf(acpi_device_class(device), "%s/%s",
+		strcpy(name, ACPI_BUTTON_DEVICE_NAME_LID);
+		sprintf(class, "%s/%s",
 			ACPI_BUTTON_CLASS, ACPI_BUTTON_SUBCLASS_LID);
 	} else {
-		printk(KERN_ERR PREFIX "Unsupported hid [%s]\n",
-			    acpi_device_hid(device));
+		printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid);
 		error = -ENODEV;
 		goto err_free_input;
 	}

@@ -357,10 +359,9 @@ static int acpi_button_add(struct acpi_device *device)
 	if (error)
 		goto err_free_input;
 
-	snprintf(button->phys, sizeof(button->phys),
-		 "%s/button/input0", acpi_device_hid(device));
+	snprintf(button->phys, sizeof(button->phys), "%s/button/input0", hid);
 
-	input->name = acpi_device_name(device);
+	input->name = name;
 	input->phys = button->phys;
 	input->id.bustype = BUS_HOST;
 	input->id.product = button->type;

@@ -401,8 +402,7 @@ static int acpi_button_add(struct acpi_device *device)
 		device->wakeup.state.enabled = 1;
 	}
 
-	printk(KERN_INFO PREFIX "%s [%s]\n",
-	       acpi_device_name(device), acpi_device_bid(device));
+	printk(KERN_INFO PREFIX "%s [%s]\n", name, acpi_device_bid(device));
 	return 0;
 
  err_remove_fs:

Bjorn Helgaas

author

2009-04-08 15:40 UTC #5 lore raw

PATCH 5/6 ACPI: button: remove button->device pointer

In reply to Bjorn Helgaas

We no longer need a pointer from struct acpi_button back to the struct acpi_device. Everywhere we used that pointer, we either already have, or can easily get, the acpi_device pointer without using the copy from acpi_button. So this patch removes the structure element.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/acpi/button.c |   31 +++++++++++++------------------
 1 files changed, 13 insertions(+), 18 deletions(-)

drivers/acpi/button.c +13 ~~-18~~

index 9f6d2e6..c844162 100644
@@ -95,7 +95,6 @@ static struct acpi_driver acpi_button_driver = {
 };
 
 struct acpi_button {
-	struct acpi_device *device;	/* Fixed button kludge */
 	unsigned int type;
 	struct input_dev *input;
 	char phys[32];			/* for input device */

@@ -126,10 +125,10 @@ static struct proc_dir_entry *acpi_button_dir;
 
 static int acpi_button_info_seq_show(struct seq_file *seq, void *offset)
 {
-	struct acpi_button *button = seq->private;
+	struct acpi_device *device = seq->private;
 
 	seq_printf(seq, "type:                    %s\n",
-		   acpi_device_name(button->device));
+		   acpi_device_name(device));
 	return 0;
 }

@@ -140,11 +139,11 @@ static int acpi_button_info_open_fs(struct inode *inode, struct file *file)
 
 static int acpi_button_state_seq_show(struct seq_file *seq, void *offset)
 {
-	struct acpi_button *button = seq->private;
+	struct acpi_device *device = seq->private;
 	acpi_status status;
 	unsigned long long state;
 
-	status = acpi_evaluate_integer(button->device->handle, "_LID", NULL, &state);
+	status = acpi_evaluate_integer(device->handle, "_LID", NULL, &state);
 	seq_printf(seq, "state:      %s\n",
 		   ACPI_FAILURE(status) ? "unsupported" :
 			(state ? "open" : "closed"));

@@ -198,8 +197,7 @@ static int acpi_button_add_fs(struct acpi_device *device)
 	/* 'info' [R] */
 	entry = proc_create_data(ACPI_BUTTON_FILE_INFO,
 				 S_IRUGO, acpi_device_dir(device),
-				 &acpi_button_info_fops,
-				 acpi_driver_data(device));
+				 &acpi_button_info_fops, device);
 	if (!entry)
 		return -ENODEV;

@@ -207,8 +205,7 @@ static int acpi_button_add_fs(struct acpi_device *device)
 	if (button->type == ACPI_BUTTON_TYPE_LID) {
 		entry = proc_create_data(ACPI_BUTTON_FILE_STATE,
 					 S_IRUGO, acpi_device_dir(device),
-					 &acpi_button_state_fops,
-					 acpi_driver_data(device));
+					 &acpi_button_state_fops, device);
 		if (!entry)
 			return -ENODEV;
 	}

@@ -238,13 +235,13 @@ static int acpi_button_remove_fs(struct acpi_device *device)
 /* --------------------------------------------------------------------------
                                 Driver Interface
    -------------------------------------------------------------------------- */
-static int acpi_lid_send_state(struct acpi_button *button)
+static int acpi_lid_send_state(struct acpi_device *device)
 {
+	struct acpi_button *button = acpi_driver_data(device);
 	unsigned long long state;
 	acpi_status status;
 
-	status = acpi_evaluate_integer(button->device->handle, "_LID", NULL,
-					&state);
+	status = acpi_evaluate_integer(device->handle, "_LID", NULL, &state);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;

@@ -266,7 +263,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
 	case ACPI_BUTTON_NOTIFY_STATUS:
 		input = button->input;
 		if (button->type == ACPI_BUTTON_TYPE_LID) {
-			acpi_lid_send_state(button);
+			acpi_lid_send_state(device);
 		} else {
 			int keycode = test_bit(KEY_SLEEP, input->keybit) ?
 						KEY_SLEEP : KEY_POWER;

@@ -277,8 +274,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
 			input_sync(input);
 		}
 
-		acpi_bus_generate_proc_event(button->device, event,
-					++button->pushed);
+		acpi_bus_generate_proc_event(device, event, ++button->pushed);
 		break;
 	default:
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,

@@ -292,7 +288,7 @@ static int acpi_button_resume(struct acpi_device *device)
 	struct acpi_button *button = acpi_driver_data(device);
 
 	if (button->type == ACPI_BUTTON_TYPE_LID)
-		return acpi_lid_send_state(button);
+		return acpi_lid_send_state(device);
 	return 0;
 }

@@ -307,7 +303,6 @@ static int acpi_button_add(struct acpi_device *device)
 	if (!button)
 		return -ENOMEM;
 
-	button->device = device;
 	device->driver_data = button;
 
 	button->input = input = input_allocate_device();

@@ -390,7 +385,7 @@ static int acpi_button_add(struct acpi_device *device)
 	if (error)
 		goto err_remove_fs;
 	if (button->type == ACPI_BUTTON_TYPE_LID)
-		acpi_lid_send_state(button);
+		acpi_lid_send_state(device);
 
 	if (device->wakeup.flags.valid) {
 		/* Button's GPE is run-wake GPE */

Bjorn Helgaas

author

2009-04-08 15:40 UTC #6 lore raw

PATCH 6/6 ACPI: button: remove control method/fixed hardware distinctions

In reply to Bjorn Helgaas

This patch removes the driver distinction between control method (CM) and fixed hardware (FF) buttons. We previously needed that so we could install either a fixed event handler or a notify handler, but the Linux/ACPI code now handles that for us, so we don't need to worry about it.

Note that this removes the FF/CM annotation from the "info" files in /proc. For example,

/proc/acpi/button/PWRF/info:
-type: Power Button (FF)
+type: Power Button

I don't think there's anything meaningful user-space can do by knowing whether a button is a control method or a fixed hardware button, so nobody should be looking at the FF/CM.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/acpi/button.c |   32 ++++++--------------------------
 1 files changed, 6 insertions(+), 26 deletions(-)

drivers/acpi/button.c +6 ~~-26~~

index c844162..9195deb 100644
@@ -41,17 +41,13 @@
 
 #define ACPI_BUTTON_SUBCLASS_POWER	"power"
 #define ACPI_BUTTON_HID_POWER		"PNP0C0C"
-#define ACPI_BUTTON_DEVICE_NAME_POWER	"Power Button (CM)"
-#define ACPI_BUTTON_DEVICE_NAME_POWERF	"Power Button (FF)"
+#define ACPI_BUTTON_DEVICE_NAME_POWER	"Power Button"
 #define ACPI_BUTTON_TYPE_POWER		0x01
-#define ACPI_BUTTON_TYPE_POWERF		0x02
 
 #define ACPI_BUTTON_SUBCLASS_SLEEP	"sleep"
 #define ACPI_BUTTON_HID_SLEEP		"PNP0C0E"
-#define ACPI_BUTTON_DEVICE_NAME_SLEEP	"Sleep Button (CM)"
-#define ACPI_BUTTON_DEVICE_NAME_SLEEPF	"Sleep Button (FF)"
+#define ACPI_BUTTON_DEVICE_NAME_SLEEP	"Sleep Button"
 #define ACPI_BUTTON_TYPE_SLEEP		0x03
-#define ACPI_BUTTON_TYPE_SLEEPF		0x04
 
 #define ACPI_BUTTON_SUBCLASS_LID	"lid"
 #define ACPI_BUTTON_HID_LID		"PNP0C0D"

@@ -166,14 +162,12 @@ static int acpi_button_add_fs(struct acpi_device *device)
 
 	switch (button->type) {
 	case ACPI_BUTTON_TYPE_POWER:
-	case ACPI_BUTTON_TYPE_POWERF:
 		if (!acpi_power_dir)
 			acpi_power_dir = proc_mkdir(ACPI_BUTTON_SUBCLASS_POWER,
 						    acpi_button_dir);
 		entry = acpi_power_dir;
 		break;
 	case ACPI_BUTTON_TYPE_SLEEP:
-	case ACPI_BUTTON_TYPE_SLEEPF:
 		if (!acpi_sleep_dir)
 			acpi_sleep_dir = proc_mkdir(ACPI_BUTTON_SUBCLASS_SLEEP,
 						    acpi_button_dir);

@@ -315,30 +309,18 @@ static int acpi_button_add(struct acpi_device *device)
 	name = acpi_device_name(device);
 	class = acpi_device_class(device);
 
-	/*
-	 * Determine the button type (via hid), as fixed-feature buttons
-	 * need to be handled a bit differently than generic-space.
-	 */
-	if (!strcmp(hid, ACPI_BUTTON_HID_POWER)) {
+	if (!strcmp(hid, ACPI_BUTTON_HID_POWER) ||
+	    !strcmp(hid, ACPI_BUTTON_HID_POWERF)) {
 		button->type = ACPI_BUTTON_TYPE_POWER;
 		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_POWERF)) {
-		button->type = ACPI_BUTTON_TYPE_POWERF;
-		strcpy(name, ACPI_BUTTON_DEVICE_NAME_POWERF);
-		sprintf(class, "%s/%s",
-			ACPI_BUTTON_CLASS, ACPI_BUTTON_SUBCLASS_POWER);
-	} else if (!strcmp(hid, ACPI_BUTTON_HID_SLEEP)) {
+	} else if (!strcmp(hid, ACPI_BUTTON_HID_SLEEP) ||
+		   !strcmp(hid, ACPI_BUTTON_HID_SLEEPF)) {
 		button->type = ACPI_BUTTON_TYPE_SLEEP;
 		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_SLEEPF)) {
-		button->type = ACPI_BUTTON_TYPE_SLEEPF;
-		strcpy(name, ACPI_BUTTON_DEVICE_NAME_SLEEPF);
-		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;
 		strcpy(name, ACPI_BUTTON_DEVICE_NAME_LID);

@@ -364,13 +346,11 @@ static int acpi_button_add(struct acpi_device *device)
 
 	switch (button->type) {
 	case ACPI_BUTTON_TYPE_POWER:
-	case ACPI_BUTTON_TYPE_POWERF:
 		input->evbit[0] = BIT_MASK(EV_KEY);
 		set_bit(KEY_POWER, input->keybit);
 		break;
 
 	case ACPI_BUTTON_TYPE_SLEEP:
-	case ACPI_BUTTON_TYPE_SLEEPF:
 		input->evbit[0] = BIT_MASK(EV_KEY);
 		set_bit(KEY_SLEEP, input->keybit);
 		break;

Len Brown

reviewer

2009-04-18 04:19 UTC #7 lore raw

In reply to Bjorn Helgaas

i added these to the acpi tree on the 11th.

thanks,
Len Brown, Intel Open Source Technology Center

A message that is not in the archive (sent off-list or not yet received): [email protected]

Bjorn Helgaas

author

2009-05-06 17:20 UTC #10 lore raw

Quoted 8 lines · Thomas Renninger

Below is the outcome of the patch which fixes a fixed feature button s2ram issue.
This does not work anymore since Bjoern's patches. Comparing the with the HID of the acpi device looks ugly.
Shall I revive the fixed feature vs GPE button types? ACPI_BUTTON_TYPE_POWERF and ACPI_BUTTON_TYPE_SLEEPF?

I don't understand exactly how I broke this.

Your patch makes us remove the fixed event handler in .suspend() and install it again in .resume(). In 2.6.29, we didn't touch the handler during suspend/resume, and my patches didn't change that aspect of things.

Whatever the problem is, it feels like something that should be handled in the Linux/ACPI code, not in the driver.

Did you identify the patch that broke things? My guess would be 373cfc360ec773b, but I took a look at it (and 46ec8598fde74b, on which it depends), and I don't see the problem yet.

Bjorn

Quoted 87 lines

Can you think of anything nicer?

Thanks,

Thomas

ACPI: button: Fix fixed feature buttons for s2disk for HW modifing event registers

This happened on a recent HP laptop. After s2disk the fixed feature power button event handler is not functional because the BIOS has modified the fixed feature register(s) when resuming/rebooting after s2disk.
Setting up some bits there again by unregistering and re-installing the fixed feature event handler fixes the problem and the button is functional again after s2disk.

Comment from Rafael:
This may be a consequence of the fact that we initialize ACPI before reading the hibernation image from disk. The spec seems to assume that won't be done. Hence, I think we should handle this.

Signed-off-by Thomas Renninger <[email protected]>

drivers/acpi/button.c +36 -1

index 9195deb..80ee2bc 100644
@@ -74,6 +74,7 @@ MODULE_DEVICE_TABLE(acpi, button_device_ids);
 static int acpi_button_add(struct acpi_device *device);
 static int acpi_button_remove(struct acpi_device *device, int type);
 static int acpi_button_resume(struct acpi_device *device);
+static int acpi_button_suspend(struct acpi_device *device, pm_message_t state);
 static void acpi_button_notify(struct acpi_device *device, u32 event);
 static int acpi_button_info_open_fs(struct inode *inode, struct file *file);
 static int acpi_button_state_open_fs(struct inode *inode, struct file *file);

@@ -85,6 +86,7 @@ static struct acpi_driver acpi_button_driver = {
 	.ops = {
 		.add = acpi_button_add,
 		.resume = acpi_button_resume,
+		.suspend = acpi_button_suspend,
 		.remove = acpi_button_remove,
 		.notify = acpi_button_notify,
 	},

@@ -281,8 +283,41 @@ static int acpi_button_resume(struct acpi_device *device)
 {
 	struct acpi_button *button = acpi_driver_data(device);
 
-	if (button->type == ACPI_BUTTON_TYPE_LID)
+	if (!button)
+		return -EINVAL;
+	switch (button->type) {
+	case ACPI_BUTTON_TYPE_LID:
 		return acpi_lid_send_state(device);
+	case ACPI_BUTTON_TYPE_SLEEPF:
+		return acpi_install_fixed_event_handler(
+					ACPI_EVENT_SLEEP_BUTTON,
+					acpi_button_notify_fixed, button);
+
+	case ACPI_BUTTON_TYPE_POWERF:
+		return acpi_install_fixed_event_handler(
+					ACPI_EVENT_POWER_BUTTON,
+					acpi_button_notify_fixed, button);
+	}
+	return 0;
+}
+
+static int acpi_button_suspend(struct acpi_device *device, pm_message_t state)
+{
+	struct acpi_button *button;
+	if (!device)
+		return -EINVAL;
+	button = acpi_driver_data(device);
+	if (!button)
+		return -EINVAL;
+	switch (button->type) {
+	case ACPI_BUTTON_TYPE_SLEEPF:
+		return acpi_remove_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
+					       acpi_button_notify_fixed);
+
+	case ACPI_BUTTON_TYPE_POWERF:
+		return acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
+					       acpi_button_notify_fixed);
+	}
 	return 0;
 }

Bjorn Helgaas

author

2009-05-08 17:21 UTC #11 lore raw

Quoted 17 lines · Bjorn Helgaas

Quoted 8 lines · Thomas Renninger
Below is the outcome of the patch which fixes a fixed feature button s2ram issue.
This does not work anymore since Bjoern's patches. Comparing the with the HID of the acpi device looks ugly.
Shall I revive the fixed feature vs GPE button types? ACPI_BUTTON_TYPE_POWERF and ACPI_BUTTON_TYPE_SLEEPF?
I don't understand exactly how I broke this.
Your patch makes us remove the fixed event handler in .suspend() and install it again in .resume(). In 2.6.29, we didn't touch the handler during suspend/resume, and my patches didn't change that aspect of things.

Quoted 6 lines

Whatever the problem is, it feels like something that should be handled in the Linux/ACPI code, not in the driver.
Did you identify the patch that broke things? My guess would be 373cfc360ec773b, but I took a look at it (and 46ec8598fde74b, on which it depends), and I don't see the problem yet.

What's happening here? If I broke something, I want to figure it out ASAP because it sounds like a regression from 2.6.29. Is there a bugzilla or more detailed bug report anywhere?

Bjorn

Quoted 90 lines

Quoted 87 lines

Can you think of anything nicer?

Thanks,

Thomas

ACPI: button: Fix fixed feature buttons for s2disk for HW modifing event registers

Signed-off-by Thomas Renninger <[email protected]>

drivers/acpi/button.c +36 -1

index 9195deb..80ee2bc 100644
@@ -74,6 +74,7 @@ MODULE_DEVICE_TABLE(acpi, button_device_ids);
 static int acpi_button_add(struct acpi_device *device);
 static int acpi_button_remove(struct acpi_device *device, int type);
 static int acpi_button_resume(struct acpi_device *device);
+static int acpi_button_suspend(struct acpi_device *device, pm_message_t state);
 static void acpi_button_notify(struct acpi_device *device, u32 event);
 static int acpi_button_info_open_fs(struct inode *inode, struct file *file);
 static int acpi_button_state_open_fs(struct inode *inode, struct file *file);

@@ -85,6 +86,7 @@ static struct acpi_driver acpi_button_driver = {
 	.ops = {
 		.add = acpi_button_add,
 		.resume = acpi_button_resume,
+		.suspend = acpi_button_suspend,
 		.remove = acpi_button_remove,
 		.notify = acpi_button_notify,
 	},

@@ -281,8 +283,41 @@ static int acpi_button_resume(struct acpi_device *device)
 {
 	struct acpi_button *button = acpi_driver_data(device);
 
-	if (button->type == ACPI_BUTTON_TYPE_LID)
+	if (!button)
+		return -EINVAL;
+	switch (button->type) {
+	case ACPI_BUTTON_TYPE_LID:
 		return acpi_lid_send_state(device);
+	case ACPI_BUTTON_TYPE_SLEEPF:
+		return acpi_install_fixed_event_handler(
+					ACPI_EVENT_SLEEP_BUTTON,
+					acpi_button_notify_fixed, button);
+
+	case ACPI_BUTTON_TYPE_POWERF:
+		return acpi_install_fixed_event_handler(
+					ACPI_EVENT_POWER_BUTTON,
+					acpi_button_notify_fixed, button);
+	}
+	return 0;
+}
+
+static int acpi_button_suspend(struct acpi_device *device, pm_message_t state)
+{
+	struct acpi_button *button;
+	if (!device)
+		return -EINVAL;
+	button = acpi_driver_data(device);
+	if (!button)
+		return -EINVAL;
+	switch (button->type) {
+	case ACPI_BUTTON_TYPE_SLEEPF:
+		return acpi_remove_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
+					       acpi_button_notify_fixed);
+
+	case ACPI_BUTTON_TYPE_POWERF:
+		return acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
+					       acpi_button_notify_fixed);
+	}
 	return 0;
 }

Bjorn Helgaas

author

2009-05-08 20:53 UTC #12 lore raw

In reply to Bjorn Helgaas

Quoted 11 lines · Bjorn Helgaas

Quoted 8 lines · Thomas Renninger
Below is the outcome of the patch which fixes a fixed feature button s2ram issue.
This does not work anymore since Bjoern's patches. Comparing the with the HID of the acpi device looks ugly.
Shall I revive the fixed feature vs GPE button types? ACPI_BUTTON_TYPE_POWERF and ACPI_BUTTON_TYPE_SLEEPF?
I don't understand exactly how I broke this.

What tree are you testing? In Linus's upstream tree, 373cfc360e adds the .notify method, and at the same time, it removes acpi_button_notify_fixed().

Your patch below, which fixes the problem, shows that you already have the .notify method, and yet you apparently *also* have the acpi_button_notify_fixed() function. That makes me suspect that you're using a tree with a merge error.

Bjorn

Quoted 55 lines

Quoted 52 lines

(quoted diff) +35 -1

@@ -85,6 +86,7 @@ static struct acpi_driver acpi_button_driver = {
 	.ops = {
 		.add = acpi_button_add,
 		.resume = acpi_button_resume,
+		.suspend = acpi_button_suspend,
 		.remove = acpi_button_remove,
 		.notify = acpi_button_notify,
 	},

@@ -281,8 +283,41 @@ static int acpi_button_resume(struct acpi_device *device)
 {
 	struct acpi_button *button = acpi_driver_data(device);
 
-	if (button->type == ACPI_BUTTON_TYPE_LID)
+	if (!button)
+		return -EINVAL;
+	switch (button->type) {
+	case ACPI_BUTTON_TYPE_LID:
 		return acpi_lid_send_state(device);
+	case ACPI_BUTTON_TYPE_SLEEPF:
+		return acpi_install_fixed_event_handler(
+					ACPI_EVENT_SLEEP_BUTTON,
+					acpi_button_notify_fixed, button);
+
+	case ACPI_BUTTON_TYPE_POWERF:
+		return acpi_install_fixed_event_handler(
+					ACPI_EVENT_POWER_BUTTON,
+					acpi_button_notify_fixed, button);
+	}
+	return 0;
+}
+
+static int acpi_button_suspend(struct acpi_device *device, pm_message_t state)
+{
+	struct acpi_button *button;
+	if (!device)
+		return -EINVAL;
+	button = acpi_driver_data(device);
+	if (!button)
+		return -EINVAL;
+	switch (button->type) {
+	case ACPI_BUTTON_TYPE_SLEEPF:
+		return acpi_remove_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
+					       acpi_button_notify_fixed);
+
+	case ACPI_BUTTON_TYPE_POWERF:
+		return acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
+					       acpi_button_notify_fixed);
+	}
 	return 0;
 }

Thomas Renninger

2009-05-11 05:55 UTC #13 lore raw

Quoted 5 lines · Bjorn Helgaas

Quoted 4 lines · Bjorn Helgaas
Quoted 3 lines · Thomas Renninger
Below is the outcome of the patch which fixes a fixed feature button s2ram issue.

Quoted 7 lines

Quoted 7 lines
Quoted 5 lines
This does not work anymore since Bjoern's patches. Comparing the with the HID of the acpi device looks ugly.
Shall I revive the fixed feature vs GPE button types? ACPI_BUTTON_TYPE_POWERF and ACPI_BUTTON_TYPE_SLEEPF?
I don't understand exactly how I broke this.

Ahh, sorry.
You did not break anything.

The patch is a fix (not for a regression), but is now (with your change) somewhat harder/uncleaner to integrate.

The fix removes fixed featured button's fixed event handler on suspend and installs it again on resume.

With your change, the button driver does not know anymore about fixed feature buttons. And notify handlers shouldn't be touched with your change in the button driver, but I think this is cleaner.

So this is nothing urgent. I'll send a patch which works with your version, but I am not sure what the cleanest way is now to add this.

Sorry for not being clear,

Thomas

Quoted 61 lines

What tree are you testing? In Linus's upstream tree, 373cfc360e adds the .notify method, and at the same time, it removes acpi_button_notify_fixed().

Bjorn

Quoted 50 lines

(quoted diff) +34 -1

@@ -85,6 +86,7 @@ static struct acpi_driver acpi_button_driver = {
 	.ops = {
 		.add = acpi_button_add,
 		.resume = acpi_button_resume,
+		.suspend = acpi_button_suspend,
 		.remove = acpi_button_remove,
 		.notify = acpi_button_notify,
 	},

@@ -281,8 +283,41 @@ static int acpi_button_resume(struct acpi_device
 *device) {
 	struct acpi_button *button = acpi_driver_data(device);
 
-	if (button->type == ACPI_BUTTON_TYPE_LID)
+	if (!button)
+		return -EINVAL;
+	switch (button->type) {
+	case ACPI_BUTTON_TYPE_LID:
 		return acpi_lid_send_state(device);
+	case ACPI_BUTTON_TYPE_SLEEPF:
+		return acpi_install_fixed_event_handler(
+					ACPI_EVENT_SLEEP_BUTTON,
+					acpi_button_notify_fixed, button);
+
+	case ACPI_BUTTON_TYPE_POWERF:
+		return acpi_install_fixed_event_handler(
+					ACPI_EVENT_POWER_BUTTON,
+					acpi_button_notify_fixed, button);
+	}
+	return 0;
+}
+
+static int acpi_button_suspend(struct acpi_device *device,
 pm_message_t state) +{
+	struct acpi_button *button;
+	if (!device)
+		return -EINVAL;
+	button = acpi_driver_data(device);
+	if (!button)
+		return -EINVAL;
+	switch (button->type) {
+	case ACPI_BUTTON_TYPE_SLEEPF:
+		return acpi_remove_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
+					       acpi_button_notify_fixed);
+
+	case ACPI_BUTTON_TYPE_POWERF:
+		return acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
+					       acpi_button_notify_fixed);
+	}
 	return 0;
 }

Bjorn Helgaas

author

2009-05-11 22:13 UTC #14 lore raw

Quoted 12 lines · Thomas Renninger

Quoted 11 lines · Bjorn Helgaas
Quoted 10 lines · Bjorn Helgaas
Quoted 8 lines · Thomas Renninger
Below is the outcome of the patch which fixes a fixed feature button s2ram issue.
This does not work anymore since Bjoern's patches. Comparing the with the HID of the acpi device looks ugly.
Shall I revive the fixed feature vs GPE button types? ACPI_BUTTON_TYPE_POWERF and ACPI_BUTTON_TYPE_SLEEPF?

Quoted 6 lines

Quoted 1 line
Quoted 1 line
I don't understand exactly how I broke this.
Ahh, sorry.
You did not break anything.
The patch is a fix (not for a regression), but is now (with your change) somewhat harder/uncleaner to integrate.

Oh, I see. I was getting worried, so thanks for clarifying that!

The problem is that on some laptops, the button driver doesn't receive fixed hardware power button events after resuming from hibernation. Here's my guess at what's happening:

disable button event in PM1_EN (acpi_ev_fixed_event_initialize()) load button driver
install fixed hardware event handler
clear button event status in PM1_STS (acpi_install_fixed_event_handler()) enable button event in PM1_EN (acpi_install_fixed_event_handler())

disable button event in PM1_EN (acpi_ev_fixed_event_initialize()) read hibernation image from disk
branch into hibernation image

Now the button driver is present because it was in the hibernation image, but we didn't call acpi_install_fixed_event_handler() in the second boot, so the fixed hardware event has never been enabled in PM1_EN.

If my guess were correct, fixed hardware power buttons shouldn't work on *any* systems after hibernation. But maybe there's more that I don't understand.

Maybe we need something along the lines of the patch below. I notice that there are several other places where we disable/enable GPEs in the suspend/resume path, and I suppose we should consider fixed hardware events at those places, too.

Bjorn

drivers/acpi/acpica/evxfevnt.c +23 -0

index d0a0807..b590ce5 100644
@@ -812,6 +812,29 @@ acpi_ev_get_gpe_device(struct acpi_gpe_xrupt_info *gpe_xrupt_info,
 	return (AE_OK);
 }
 
+acpi_status acpi_enable_fixed_events(void)
+{
+	acpi_status status;
+	u32 i;
+
+	ACPI_FUNCTION_TRACE(acpi_disable_all_gpes);
+
+	status = acpi_ut_acquire_mutex(ACPI_MTX_EVENTS);
+	if (ACPI_FAILURE(status)) {
+		return_ACPI_STATUS(status);
+	}
+
+	for (i = 0; i < ACPI_NUM_FIXED_EVENTS; i++) {
+		if (acpi_gbl_fixed_event_handlers[i].handler) {
+			acpi_enable_event(i, 0);
+		}
+	}
+
+	(void)acpi_ut_release_mutex(ACPI_MTX_EVENTS);
+
+	return_ACPI_STATUS(AE_OK);
+}
+
 /******************************************************************************
  *
  * FUNCTION:    acpi_disable_all_gpes

drivers/acpi/sleep.c +1 -0

index 01574a0..85aa8be 100644
@@ -501,6 +501,7 @@ static void acpi_hibernation_leave(void)
 
 static void acpi_pm_enable_gpes(void)
 {
+	acpi_enable_fixed_events();
 	acpi_enable_all_runtime_gpes();
 }