Reporting the lid status using INPUT

PATCH In review Richard Hughes 4 participants 10 messages Jun 15, 2007 – Jun 27, 2007 ACPI

lore

Share & tools

lore

Richard Hughes

author

2007-06-15 12:30 UTC #0 lore raw

Guys,

I'm getting a few bugreports where:

* user shuts lid, initiates suspend
{lid is reported down}
* user opens lid, resume completes

But I'm not getting any lid INPUT up event. This works if you don't initiate suspend when you shut the lid (blank screen for instance) -- so I know my hardware is okay.

I see:

input_report_switch(input, SW_LID, !state);

in response to an event, but I'm thinking in a resume hook we should probably do acpi_evaluate_integer(handle, "_LID", NULL, &state) and then send an event, just so userspace is aware of what the state of the panel is.

Does this sound sane?

Richard.

Richard Hughes

author

2007-06-15 13:14 UTC #1 lore raw

PATCH 1/1 Reporting the lid status using INPUT

Quoted 4 lines · Richard Hughes

in response to an event, but I'm thinking in a resume hook we should probably do acpi_evaluate_integer(handle, "_LID", NULL, &state) and then send an event, just so userspace is aware of what the state of the panel is.

Attached patch fixed the issue for me. Comments?

Richard.

drivers/acpi/button.c +23 -0

index cb4110b..3631b8f 100644
@@ -68,6 +68,7 @@ MODULE_LICENSE("GPL");
 
 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_info_open_fs(struct inode *inode, struct file *file);
 static int acpi_button_state_open_fs(struct inode *inode, struct file *file);

@@ -77,6 +78,7 @@ static struct acpi_driver acpi_button_driver = {
 	.ids = "button_power,button_sleep,PNP0C0D,PNP0C0C,PNP0C0E",
 	.ops = {
 		.add = acpi_button_add,
+		.resume = acpi_button_resume,
 		.remove = acpi_button_remove,
 	},
 };

@@ -487,6 +489,27 @@ static int acpi_button_remove(struct acpi_device *device, int type)
 	return 0;
 }
 
+/* this is needed to learn about changes made in suspended state */
+static int acpi_button_resume(struct acpi_device *device)
+{
+	struct acpi_button *button;
+	struct acpi_handle *handle;
+	struct input_dev *input;
+	unsigned long state;
+
+	button = device->driver_data;
+	handle = button->device->handle;
+	input = button->input;
+
+	/* on resume we send the state; it might be the same, but userspace
+	 * should handle duplicated events */
+	if (!ACPI_FAILURE(acpi_evaluate_integer(handle, "_LID",
+						NULL, &state)))
+		input_report_switch(input, SW_LID, !state);
+
+	return 0;
+}
+
 static int __init acpi_button_init(void)
 {
 	int result;

Dmitry Torokhov

2007-06-15 14:29 UTC #2 lore raw

Quoted 8 lines · Richard Hughes

Quoted 4 lines · Richard Hughes
in response to an event, but I'm thinking in a resume hook we should probably do acpi_evaluate_integer(handle, "_LID", NULL, &state) and then send an event, just so userspace is aware of what the state of the panel is.
Attached patch fixed the issue for me. Comments?

The patch makes perfect sense. The only issue I have is this:

drivers/acpi/button.c static int acpi_button_remove(struct acpi_device *device, int type) · line 505

Quoted 2 lines

drivers/acpi/button.c +2 -0

+	/* on resume we send the state; it might be the same, but userspace
+	 * should handle duplicated events */

If switch state matches to what input layer thinks it is the event will not even reach userspace.

Signature · 1 line

Dmitry

Richard Hughes

author

2007-06-15 14:54 UTC #3 lore raw

Quoted 10 lines · Dmitry Torokhov

Quoted 8 lines · Richard Hughes
Quoted 4 lines · Richard Hughes
in response to an event, but I'm thinking in a resume hook we should probably do acpi_evaluate_integer(handle, "_LID", NULL, &state) and then send an event, just so userspace is aware of what the state of the panel is.
Attached patch fixed the issue for me. Comments?

Quoted 7 lines

The patch makes perfect sense. The only issue I have is this:
Review
Quoted 2 lines
(quoted diff) +2 -0
+	/* on resume we send the state; it might be the same, but userspace
+	 * should handle duplicated events */
If switch state matches to what input layer thinks it is the event will not even reach userspace.

Okay, new patch attached, thanks for the speedy review.

Signed-off-by Richard Hughes <[email protected]>

On resume we need to refresh the lid status as we will not get an event if the lid opening was what triggered the suspend.
This manifests itself in users never getting a "lid open" event when a suspend happens because of lid close on hardware that supports wake on lid open. This makes userspace gets very confused indeed. Patch attached forces a check of the lid status in the resume handler.

Signed-off-by Richard Hughes <[email protected]>

---

drivers/acpi/button.c +25 -0

index cb4110b..fd3473b 100644
@@ -68,6 +68,7 @@ MODULE_LICENSE("GPL");
 
 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_info_open_fs(struct inode *inode, struct file *file);
 static int acpi_button_state_open_fs(struct inode *inode, struct file *file);

@@ -77,6 +78,7 @@ static struct acpi_driver acpi_button_driver = {
 	.ids = "button_power,button_sleep,PNP0C0D,PNP0C0C,PNP0C0E",
 	.ops = {
 		.add = acpi_button_add,
+		.resume = acpi_button_resume,
 		.remove = acpi_button_remove,
 	},
 };

@@ -487,6 +489,29 @@ static int acpi_button_remove(struct acpi_device *device, int type)
 	return 0;
 }
 
+/* this is needed to learn about changes made in suspended state */
+static int acpi_button_resume(struct acpi_device *device)
+{
+	struct acpi_button *button;
+	struct acpi_handle *handle;
+	struct input_dev *input;
+	unsigned long state;
+
+	button = device->driver_data;
+	handle = button->device->handle;
+	input = button->input;
+
+	/*
+	 * On resume we send the state; if it matches to what input layer
+	 * thinks then the event will not even reach userspace.
+	 */
+	if (!ACPI_FAILURE(acpi_evaluate_integer(handle, "_LID",
+						NULL, &state)))
+		input_report_switch(input, SW_LID, !state);
+
+	return 0;
+}
+
 static int __init acpi_button_init(void)
 {
 	int result;

Richard Hughes

author

2007-06-16 17:13 UTC #4 lore raw

Quoted 13 lines · Richard Hughes

Quoted 12 lines · Dmitry Torokhov
Quoted 8 lines · Richard Hughes
Quoted 4 lines · Richard Hughes
in response to an event, but I'm thinking in a resume hook we should probably do acpi_evaluate_integer(handle, "_LID", NULL, &state) and then send an event, just so userspace is aware of what the state of the panel is.
Attached patch fixed the issue for me. Comments?
The patch makes perfect sense. The only issue I have is this:

Quoted 7 lines

Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+	/* on resume we send the state; it might be the same, but userspace
+	 * should handle duplicated events */
If switch state matches to what input layer thinks it is the event will not even reach userspace.
Okay, new patch attached, thanks for the speedy review.

This fix is also confirmed by somebody else, see
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=243030

It would be great if this could go into .22, although I appreciate it's quite late in the day.

Richard.

Dmitry Torokhov

2007-06-16 17:51 UTC #5 lore raw

Quoted 20 lines · Richard Hughes

Quoted 19 lines · Richard Hughes
Quoted 17 lines · Dmitry Torokhov
Quoted 8 lines · Richard Hughes
Quoted 4 lines · Richard Hughes
in response to an event, but I'm thinking in a resume hook we should probably do acpi_evaluate_integer(handle, "_LID", NULL, &state) and then send an event, just so userspace is aware of what the state of the panel is.
Attached patch fixed the issue for me. Comments?
The patch makes perfect sense. The only issue I have is this:
Review
Quoted 2 lines
(quoted diff) +2 -0
+	/* on resume we send the state; it might be the same, but userspace
+	 * should handle duplicated events */
If switch state matches to what input layer thinks it is the event will not even reach userspace.

Quoted 8 lines

Quoted 1 line
Okay, new patch attached, thanks for the speedy review.
This fix is also confirmed by somebody else, see
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=243030
It would be great if this could go into .22, although I appreciate it's quite late in the day.

This is of course Len's call but in my book this is a bugfix and as such is appropriate for -rc4/rc5.

Signature · 1 line

Dmitry

GMail

author

2007-06-24 22:06 UTC #6 lore raw

Quoted 26 lines · Dmitry Torokhov

Quoted 25 lines · Richard Hughes
Quoted 13 lines · Richard Hughes
Quoted 12 lines · Dmitry Torokhov
Quoted 8 lines · Richard Hughes
Quoted 4 lines · Richard Hughes
in response to an event, but I'm thinking in a resume hook we should probably do acpi_evaluate_integer(handle, "_LID", NULL, &state) and then send an event, just so userspace is aware of what the state of the panel is.
Attached patch fixed the issue for me. Comments?
The patch makes perfect sense. The only issue I have is this:
Quoted 7 lines
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+       /* on resume we send the state; it might be the same, but userspace
+        * should handle duplicated events */
If switch state matches to what input layer thinks it is the event will not even reach userspace.
Okay, new patch attached, thanks for the speedy review.
This fix is also confirmed by somebody else, see
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=243030

Quoted 6 lines

Quoted 3 lines
It would be great if this could go into .22, although I appreciate it's quite late in the day.
This is of course Len's call but in my book this is a bugfix and as such is appropriate for -rc4/rc5.

Guys? Any ack-nak?

Richard.

Bartók Albert

2007-06-25 20:49 UTC #7 lore raw

Quoted 26 lines · Dmitry Torokhov

Quoted 25 lines · Richard Hughes
Quoted 13 lines · Richard Hughes
Quoted 12 lines · Dmitry Torokhov
Quoted 8 lines · Richard Hughes
Quoted 4 lines · Richard Hughes
in response to an event, but I'm thinking in a resume hook we should probably do acpi_evaluate_integer(handle, "_LID", NULL, &state) and then send an event, just so userspace is aware of what the state of the panel is.
Attached patch fixed the issue for me. Comments?
The patch makes perfect sense. The only issue I have is this:
Quoted 7 lines
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+       /* on resume we send the state; it might be the same, but userspace
+        * should handle duplicated events */
If switch state matches to what input layer thinks it is the event will not even reach userspace.
Okay, new patch attached, thanks for the speedy review.
This fix is also confirmed by somebody else, see
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=243030

Quoted 8 lines

Quoted 3 lines
It would be great if this could go into .22, although I appreciate it's quite late in the day.
This is of course Len's call but in my book this is a bugfix and as such is appropriate for -rc4/rc5.
Guys? Any ack-nak?

works perfectly for me so far.

Albert

15% KEDVEZMÉNY minden PLASZTIKAI MŰTÉTRE az Aesthetica orvosi központban! Klikk ide!http://www.webdesign.hu/aesthetica/flash_microsite/?id=9;p_code=2079

Richard Hughes

author

2007-06-25 21:25 UTC #8 lore raw

Quoted 31 lines · Bartók Albert

Quoted 30 lines · Dmitry Torokhov
Quoted 28 lines · Richard Hughes
Quoted 13 lines · Richard Hughes
Quoted 12 lines · Dmitry Torokhov
Quoted 8 lines · Richard Hughes
Quoted 4 lines · Richard Hughes
in response to an event, but I'm thinking in a resume hook we should probably do acpi_evaluate_integer(handle, "_LID", NULL, &state) and then send an event, just so userspace is aware of what the state of the panel is.
Attached patch fixed the issue for me. Comments?
The patch makes perfect sense. The only issue I have is this:
Quoted 7 lines
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+       /* on resume we send the state; it might be the same, but userspace
+        * should handle duplicated events */
If switch state matches to what input layer thinks it is the event will not even reach userspace.
Okay, new patch attached, thanks for the speedy review.
This fix is also confirmed by somebody else, see
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=243030
It would be great if this could go into .22, although I appreciate it's quite late in the day.

Quoted 6 lines

Quoted 4 lines
This is of course Len's call but in my book this is a bugfix and as such is appropriate for -rc4/rc5.
Guys? Any ack-nak?
works perfectly for me so far.

Thanks. Len, can you add this to your tree please. Thanks.

Richard.

-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Richard Hughes

author

2007-06-27 19:43 UTC #9 lore raw

PATCH 1/1 Reporting the lid status using INPUT

In reply to Bartók Albert

Len,

* user shuts lid, initiates suspend
{lid is reported down}
* user opens lid, resume completes

But the lid INPUT up event is not sent. This works if you don't initiate suspend when you shut the lid (blank screen for instance) -- so I know my hardware is okay.

I see:
input_report_switch(input, SW_LID, !state);

in response to an event, but in a resume hook we should probably do acpi_evaluate_integer(handle, "_LID", NULL, &state) and then send an event, just so userspace is aware of what the state of the panel is.

If switch state matches to what input layer thinks it is the event will not be sent.

Signed-off-by Richard Hughes <[email protected]>

---

drivers/acpi/button.c +25 -0

index cb4110b..fd3473b 100644
@@ -68,6 +68,7 @@ MODULE_LICENSE("GPL");
 
 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_info_open_fs(struct inode *inode, struct file *file);
 static int acpi_button_state_open_fs(struct inode *inode, struct file *file);

@@ -77,6 +78,7 @@ static struct acpi_driver acpi_button_driver = {
 	.ids = "button_power,button_sleep,PNP0C0D,PNP0C0C,PNP0C0E",
 	.ops = {
 		.add = acpi_button_add,
+		.resume = acpi_button_resume,
 		.remove = acpi_button_remove,
 	},
 };

@@ -487,6 +489,29 @@ static int acpi_button_remove(struct acpi_device *device, int type)
 	return 0;
 }
 
+/* this is needed to learn about changes made in suspended state */
+static int acpi_button_resume(struct acpi_device *device)
+{
+	struct acpi_button *button;
+	struct acpi_handle *handle;
+	struct input_dev *input;
+	unsigned long state;
+
+	button = device->driver_data;
+	handle = button->device->handle;
+	input = button->input;
+
+	/*
+	 * On resume we send the state; if it matches to what input layer
+	 * thinks then the event will not even reach userspace.
+	 */
+	if (!ACPI_FAILURE(acpi_evaluate_integer(handle, "_LID",
+						NULL, &state)))
+		input_report_switch(input, SW_LID, !state);
+
+	return 0;
+}
+
 static int __init acpi_button_init(void)
 {
 	int result;