[PATCH 0/6] ACPI: button: minor cleanups

[PATCH 0/6] ACPI: button: minor cleanups

[Bjorn Helgaas]

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(-)

-- 
Bjorn

[PATCH 1/6] ACPI: button: whitespace changes

[Bjorn Helgaas]

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

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/button.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index d73c94b..52eb06e 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -1,5 +1,5 @@
 /*
- *  acpi_button.c - ACPI Button Driver ($Revision: 30 $)
+ *  button.c - ACPI Button Driver
  *
  *  Copyright (C) 2001, 2002 Andy Grover <andrew.grover@intel.com>
  *  Copyright (C) 2001, 2002 Paul Diefenbaugh <paul.s.diefenbaugh@intel.com>
@@ -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);

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

[Bjorn Helgaas]

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

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/button.c |   22 +---------------------
 1 files changed, 1 insertions(+), 21 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 52eb06e..c463236 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -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);

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

[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 <bjorn.helgaas@hp.com>
---
 drivers/acpi/button.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index c463236..8ef8f44 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -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);

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

[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 <bjorn.helgaas@hp.com>
---
 drivers/acpi/button.c |   48 ++++++++++++++++++++++++------------------------
 1 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 8ef8f44..9f6d2e6 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -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:

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

[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 <bjorn.helgaas@hp.com>
---
 drivers/acpi/button.c |   31 +++++++++++++------------------
 1 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 9f6d2e6..c844162 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -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 */

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

[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 <bjorn.helgaas@hp.com>
---
 drivers/acpi/button.c |   32 ++++++--------------------------
 1 files changed, 6 insertions(+), 26 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index c844162..9195deb 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -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;

Re: [PATCH 0/6] ACPI: button: minor cleanups

[Len Brown]

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

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 0/6] ACPI: button: minor cleanups

[Bjorn Helgaas]

On Wednesday 06 May 2009 09:13:07 am Thomas Renninger wrote:
> 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

> 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 <trenn@suse.de>
> 
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 9195deb..80ee2bc 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -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;
>  }
>  
> 

Re: [PATCH 0/6] ACPI: button: minor cleanups

[Bjorn Helgaas]

On Wednesday 06 May 2009 11:20:52 am Bjorn Helgaas wrote:
> On Wednesday 06 May 2009 09:13:07 am Thomas Renninger wrote:
> > 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.

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

> > 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 <trenn@suse.de>
> > 
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index 9195deb..80ee2bc 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -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;
> >  }
> >  
> > 
> 
> 
> 

Re: [PATCH 0/6] ACPI: button: minor cleanups

[Bjorn Helgaas]

On Wednesday 06 May 2009 11:20:52 am Bjorn Helgaas wrote:
> On Wednesday 06 May 2009 09:13:07 am Thomas Renninger wrote:
> > 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

> > @@ -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;
> >  }
> >  
> > 
> 
> 
> 

Re: [PATCH 0/6] ACPI: button: minor cleanups

[Thomas Renninger]

On Friday 08 May 2009 10:53:01 pm Bjorn Helgaas wrote:
> On Wednesday 06 May 2009 11:20:52 am Bjorn Helgaas wrote:
> > On Wednesday 06 May 2009 09:13:07 am Thomas Renninger wrote:
> > > 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.
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

> 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
>
> > > @@ -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;
> > >  }

Re: [PATCH 0/6] ACPI: button: minor cleanups

[Bjorn Helgaas]

On Monday 11 May 2009 12:56:37 am Thomas Renninger wrote:
> On Friday 08 May 2009 10:53:01 pm Bjorn Helgaas wrote:
> > On Wednesday 06 May 2009 11:20:52 am Bjorn Helgaas wrote:
> > > On Wednesday 06 May 2009 09:13:07 am Thomas Renninger wrote:
> > > > 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.
> 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:

    <first boot>

	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())

	<write hibernation image to disk and power off>

    <second boot>

	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


diff --git a/drivers/acpi/acpica/evxfevnt.c b/drivers/acpi/acpica/evxfevnt.c
index d0a0807..b590ce5 100644
--- a/drivers/acpi/acpica/evxfevnt.c
+++ b/drivers/acpi/acpica/evxfevnt.c
@@ -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
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 01574a0..85aa8be 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -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();
 }