[PATCH] toshiba_acpi: Add full hotkey support

[PATCH] toshiba_acpi: Add full hotkey support

[Matthew Garrett]

Calling the ENAB method on Toshiba laptops results in notifications 
being sent when laptop hotkeys are pressed. This patch simply calls that 
method and sets up an input device if it's successful.
    
Signed-off-by: Matthew Garrett <mjg@redhat.com>

---

Partially based on polling code written by Daniel Silverstone, but I've 
turned it into a full input device rather than just sending ACPI 
notifications.

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index b3866ad..cd9d697 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -372,6 +372,7 @@ config ACPI_TOSHIBA
 	tristate "Toshiba Laptop Extras"
 	depends on ACPI
 	depends on INPUT
+	select INPUT
 	select INPUT_POLLDEV
 	select NET
 	select RFKILL
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 40e60fc..604f9fa 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -46,6 +46,7 @@
 #include <linux/platform_device.h>
 #include <linux/rfkill.h>
 #include <linux/input-polldev.h>
+#include <linux/input.h>
 
 #include <asm/uaccess.h>
 
@@ -62,9 +63,10 @@ MODULE_LICENSE("GPL");
 
 /* Toshiba ACPI method paths */
 #define METHOD_LCD_BRIGHTNESS	"\\_SB_.PCI0.VGA_.LCD_._BCM"
-#define METHOD_HCI_1		"\\_SB_.VALD.GHCI"
-#define METHOD_HCI_2		"\\_SB_.VALZ.GHCI"
+#define TOSH_INTERFACE_1	"\\_SB_.VALD"
+#define TOSH_INTERFACE_2	"\\_SB_.VALZ"
 #define METHOD_VIDEO_OUT	"\\_SB_.VALX.DSSX"
+#define GHCI_METHOD		".GHCI"
 
 /* Toshiba HCI interface definitions
  *
@@ -116,6 +118,36 @@ static const struct acpi_device_id toshiba_device_ids[] = {
 };
 MODULE_DEVICE_TABLE(acpi, toshiba_device_ids);
 
+struct key_entry {
+	char type;
+	u16 code;
+	u16 keycode;
+};
+
+enum {KE_KEY, KE_END};
+
+static struct key_entry toshiba_acpi_keymap[]  = {
+	{KE_KEY, 0x101, KEY_MUTE},
+	{KE_KEY, 0x13b, KEY_COFFEE},
+	{KE_KEY, 0x13c, KEY_BATTERY},
+	{KE_KEY, 0x13d, KEY_SLEEP},
+	{KE_KEY, 0x13e, KEY_SUSPEND},
+	{KE_KEY, 0x13f, KEY_SWITCHVIDEOMODE},
+	{KE_KEY, 0x140, KEY_BRIGHTNESSDOWN},
+	{KE_KEY, 0x141, KEY_BRIGHTNESSUP},
+	{KE_KEY, 0x142, KEY_WLAN},
+	{KE_KEY, 0x143, KEY_PROG1},
+	{KE_KEY, 0xb05, KEY_PROG2},
+	{KE_KEY, 0xb06, KEY_WWW},
+	{KE_KEY, 0xb07, KEY_MAIL},
+	{KE_KEY, 0xb30, KEY_STOP},
+	{KE_KEY, 0xb31, KEY_PREVIOUSSONG},
+	{KE_KEY, 0xb32, KEY_NEXTSONG},
+	{KE_KEY, 0xb33, KEY_PLAYPAUSE},
+	{KE_KEY, 0xb5a, KEY_MEDIA},
+	{KE_END, 0, 0},
+};
+
 /* utility
  */
 
@@ -252,6 +284,8 @@ struct toshiba_acpi_dev {
 	struct platform_device *p_dev;
 	struct rfkill *rfk_dev;
 	struct input_polled_dev *poll_dev;
+	struct input_dev *hotkey_dev;
+	acpi_handle handle;
 
 	const char *bt_name;
 	const char *rfk_name;
@@ -702,6 +736,154 @@ static struct backlight_ops toshiba_backlight_data = {
         .update_status  = set_lcd_status,
 };
 
+static struct key_entry *toshiba_acpi_get_entry_by_scancode(int code)
+{
+	struct key_entry *key;
+
+	for (key = toshiba_acpi_keymap; key->type != KE_END; key++)
+		if (code == key->code)
+			return key;
+
+	return NULL;
+}
+
+static struct key_entry *toshiba_acpi_get_entry_by_keycode(int code)
+{
+	struct key_entry *key;
+
+	for (key = toshiba_acpi_keymap; key->type != KE_END; key++)
+		if (code == key->keycode && key->type == KE_KEY)
+			return key;
+
+	return NULL;
+}
+
+static int toshiba_acpi_getkeycode(struct input_dev *dev, int scancode,
+				   int *keycode)
+{
+	struct key_entry *key = toshiba_acpi_get_entry_by_scancode(scancode);
+
+	if (key && key->type == KE_KEY) {
+		*keycode = key->keycode;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int toshiba_acpi_setkeycode(struct input_dev *dev, int scancode,
+				   int keycode)
+{
+	struct key_entry *key;
+	int old_keycode;
+
+	if (keycode < 0 || keycode > KEY_MAX)
+		return -EINVAL;
+
+	key = toshiba_acpi_get_entry_by_scancode(scancode);
+	if (key && key->type == KE_KEY) {
+		old_keycode = key->keycode;
+		key->keycode = keycode;
+		set_bit(keycode, dev->keybit);
+		if (!toshiba_acpi_get_entry_by_keycode(old_keycode))
+			clear_bit(old_keycode, dev->keybit);
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static void toshiba_acpi_notify(acpi_handle handle, u32 event, void **data)
+{
+	u32 hci_result, value;
+	struct key_entry *key;
+
+	if (event != 0x80)
+		return;
+	do {
+		hci_read1(HCI_SYSTEM_EVENT, &value, &hci_result);
+		if (hci_result == HCI_SUCCESS) {
+			if (value == 0x100)
+				continue;
+			else if (value & 0x80) {
+				key = toshiba_acpi_get_entry_by_scancode
+					(value & ~0x80);
+				if (!key) {
+					printk(MY_INFO "Unknown key %x\n",
+					       value & ~0x80);
+					continue;
+				}
+				input_report_key(toshiba_acpi.hotkey_dev,
+						 key->keycode, 1);
+				input_sync(toshiba_acpi.hotkey_dev);
+				input_report_key(toshiba_acpi.hotkey_dev,
+						 key->keycode, 0);
+				input_sync(toshiba_acpi.hotkey_dev);
+			}
+		} else if (hci_result == HCI_NOT_SUPPORTED) {
+			/* This is a workaround for an unresolved issue on
+			 * some machines where system events sporadically
+			 * become disabled. */
+			hci_write1(HCI_SYSTEM_EVENT, 1, &hci_result);
+			printk(MY_NOTICE "Re-enabled hotkeys\n");
+		}
+	} while (hci_result != HCI_EMPTY);
+}
+
+static int toshiba_acpi_setup_keyboard(char *device)
+{
+	acpi_status status;
+	acpi_handle handle;
+	int result;
+	const struct key_entry *key;
+
+	status = acpi_get_handle(NULL, device, &handle);
+	if (ACPI_FAILURE(status)) {
+		printk(MY_INFO "Unable to get notification device\n");
+		return -ENODEV;
+	}
+
+	toshiba_acpi.handle = handle;
+
+	status = acpi_evaluate_object(handle, "ENAB", NULL, NULL);
+	if (ACPI_FAILURE(status)) {
+		printk(MY_INFO "Unable to enable hotkeys\n");
+		return -ENODEV;
+	}
+
+	status = acpi_install_notify_handler (handle, ACPI_DEVICE_NOTIFY,
+					      toshiba_acpi_notify, NULL);
+	if (ACPI_FAILURE(status)) {
+		printk(MY_INFO "Unable to install hotkey notification\n");
+		return -ENODEV;
+	}
+
+	toshiba_acpi.hotkey_dev = input_allocate_device();
+	if (!toshiba_acpi.hotkey_dev) {
+		printk(MY_INFO "Unable to register input device\n");
+		return -ENOMEM;
+	}
+
+	toshiba_acpi.hotkey_dev->name = "Toshiba input device";
+	toshiba_acpi.hotkey_dev->phys = device;
+	toshiba_acpi.hotkey_dev->id.bustype = BUS_HOST;
+	toshiba_acpi.hotkey_dev->getkeycode = toshiba_acpi_getkeycode;
+	toshiba_acpi.hotkey_dev->setkeycode = toshiba_acpi_setkeycode;
+
+	for (key = toshiba_acpi_keymap; key->type != KE_END; key++) {
+		set_bit(EV_KEY, toshiba_acpi.hotkey_dev->evbit);
+		set_bit(key->keycode, toshiba_acpi.hotkey_dev->keybit);
+	}
+
+	result = input_register_device(toshiba_acpi.hotkey_dev);
+	if (result) {
+		printk(MY_INFO "Unable to register input device\n");
+		return result;
+	}
+
+	return 0;
+}
+
 static void toshiba_acpi_exit(void)
 {
 	if (toshiba_acpi.poll_dev) {
@@ -709,12 +891,18 @@ static void toshiba_acpi_exit(void)
 		input_free_polled_device(toshiba_acpi.poll_dev);
 	}
 
+	if (toshiba_acpi.hotkey_dev)
+		input_unregister_device(toshiba_acpi.hotkey_dev);
+
 	if (toshiba_acpi.rfk_dev)
 		rfkill_unregister(toshiba_acpi.rfk_dev);
 
 	if (toshiba_backlight_device)
 		backlight_device_unregister(toshiba_backlight_device);
 
+	acpi_remove_notify_handler(toshiba_acpi.handle, ACPI_DEVICE_NOTIFY,
+				   toshiba_acpi_notify);
+
 	remove_device();
 
 	if (toshiba_proc_dir)
@@ -738,11 +926,15 @@ static int __init toshiba_acpi_init(void)
 		return -ENODEV;
 
 	/* simple device detection: look for HCI method */
-	if (is_valid_acpi_path(METHOD_HCI_1))
-		method_hci = METHOD_HCI_1;
-	else if (is_valid_acpi_path(METHOD_HCI_2))
-		method_hci = METHOD_HCI_2;
-	else
+	if (is_valid_acpi_path(TOSH_INTERFACE_1 GHCI_METHOD)) {
+		method_hci = TOSH_INTERFACE_1 GHCI_METHOD;
+		if (toshiba_acpi_setup_keyboard(TOSH_INTERFACE_1))
+			printk(MY_INFO "Unable to activate hotkeys\n");
+	} else if (is_valid_acpi_path(TOSH_INTERFACE_2 GHCI_METHOD)) {
+		method_hci = TOSH_INTERFACE_2 GHCI_METHOD;
+		if (toshiba_acpi_setup_keyboard(TOSH_INTERFACE_2))
+			printk(MY_INFO "Unable to activate hotkeys\n");
+	} else
 		return -ENODEV;
 
 	printk(MY_INFO "Toshiba Laptop ACPI Extras version %s\n",

-- 
Matthew Garrett | mjg59@srcf.ucam.org

[PATCH] toshiba_acpi: Add full hotkey support

[Matthew Garrett]

Calling the ENAB method on Toshiba laptops results in notifications 
being sent when laptop hotkeys are pressed. This patch simply calls that 
method and sets up an input device if it's successful.
    
Signed-off-by: Matthew Garrett <mjg@redhat.com>

---

Oops - previous version included a broken Kconfig hunk and I messed up 
Len's address. This one should be good.

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 40e60fc..604f9fa 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -46,6 +46,7 @@
 #include <linux/platform_device.h>
 #include <linux/rfkill.h>
 #include <linux/input-polldev.h>
+#include <linux/input.h>
 
 #include <asm/uaccess.h>
 
@@ -62,9 +63,10 @@ MODULE_LICENSE("GPL");
 
 /* Toshiba ACPI method paths */
 #define METHOD_LCD_BRIGHTNESS	"\\_SB_.PCI0.VGA_.LCD_._BCM"
-#define METHOD_HCI_1		"\\_SB_.VALD.GHCI"
-#define METHOD_HCI_2		"\\_SB_.VALZ.GHCI"
+#define TOSH_INTERFACE_1	"\\_SB_.VALD"
+#define TOSH_INTERFACE_2	"\\_SB_.VALZ"
 #define METHOD_VIDEO_OUT	"\\_SB_.VALX.DSSX"
+#define GHCI_METHOD		".GHCI"
 
 /* Toshiba HCI interface definitions
  *
@@ -116,6 +118,36 @@ static const struct acpi_device_id toshiba_device_ids[] = {
 };
 MODULE_DEVICE_TABLE(acpi, toshiba_device_ids);
 
+struct key_entry {
+	char type;
+	u16 code;
+	u16 keycode;
+};
+
+enum {KE_KEY, KE_END};
+
+static struct key_entry toshiba_acpi_keymap[]  = {
+	{KE_KEY, 0x101, KEY_MUTE},
+	{KE_KEY, 0x13b, KEY_COFFEE},
+	{KE_KEY, 0x13c, KEY_BATTERY},
+	{KE_KEY, 0x13d, KEY_SLEEP},
+	{KE_KEY, 0x13e, KEY_SUSPEND},
+	{KE_KEY, 0x13f, KEY_SWITCHVIDEOMODE},
+	{KE_KEY, 0x140, KEY_BRIGHTNESSDOWN},
+	{KE_KEY, 0x141, KEY_BRIGHTNESSUP},
+	{KE_KEY, 0x142, KEY_WLAN},
+	{KE_KEY, 0x143, KEY_PROG1},
+	{KE_KEY, 0xb05, KEY_PROG2},
+	{KE_KEY, 0xb06, KEY_WWW},
+	{KE_KEY, 0xb07, KEY_MAIL},
+	{KE_KEY, 0xb30, KEY_STOP},
+	{KE_KEY, 0xb31, KEY_PREVIOUSSONG},
+	{KE_KEY, 0xb32, KEY_NEXTSONG},
+	{KE_KEY, 0xb33, KEY_PLAYPAUSE},
+	{KE_KEY, 0xb5a, KEY_MEDIA},
+	{KE_END, 0, 0},
+};
+
 /* utility
  */
 
@@ -252,6 +284,8 @@ struct toshiba_acpi_dev {
 	struct platform_device *p_dev;
 	struct rfkill *rfk_dev;
 	struct input_polled_dev *poll_dev;
+	struct input_dev *hotkey_dev;
+	acpi_handle handle;
 
 	const char *bt_name;
 	const char *rfk_name;
@@ -702,6 +736,154 @@ static struct backlight_ops toshiba_backlight_data = {
         .update_status  = set_lcd_status,
 };
 
+static struct key_entry *toshiba_acpi_get_entry_by_scancode(int code)
+{
+	struct key_entry *key;
+
+	for (key = toshiba_acpi_keymap; key->type != KE_END; key++)
+		if (code == key->code)
+			return key;
+
+	return NULL;
+}
+
+static struct key_entry *toshiba_acpi_get_entry_by_keycode(int code)
+{
+	struct key_entry *key;
+
+	for (key = toshiba_acpi_keymap; key->type != KE_END; key++)
+		if (code == key->keycode && key->type == KE_KEY)
+			return key;
+
+	return NULL;
+}
+
+static int toshiba_acpi_getkeycode(struct input_dev *dev, int scancode,
+				   int *keycode)
+{
+	struct key_entry *key = toshiba_acpi_get_entry_by_scancode(scancode);
+
+	if (key && key->type == KE_KEY) {
+		*keycode = key->keycode;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int toshiba_acpi_setkeycode(struct input_dev *dev, int scancode,
+				   int keycode)
+{
+	struct key_entry *key;
+	int old_keycode;
+
+	if (keycode < 0 || keycode > KEY_MAX)
+		return -EINVAL;
+
+	key = toshiba_acpi_get_entry_by_scancode(scancode);
+	if (key && key->type == KE_KEY) {
+		old_keycode = key->keycode;
+		key->keycode = keycode;
+		set_bit(keycode, dev->keybit);
+		if (!toshiba_acpi_get_entry_by_keycode(old_keycode))
+			clear_bit(old_keycode, dev->keybit);
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static void toshiba_acpi_notify(acpi_handle handle, u32 event, void **data)
+{
+	u32 hci_result, value;
+	struct key_entry *key;
+
+	if (event != 0x80)
+		return;
+	do {
+		hci_read1(HCI_SYSTEM_EVENT, &value, &hci_result);
+		if (hci_result == HCI_SUCCESS) {
+			if (value == 0x100)
+				continue;
+			else if (value & 0x80) {
+				key = toshiba_acpi_get_entry_by_scancode
+					(value & ~0x80);
+				if (!key) {
+					printk(MY_INFO "Unknown key %x\n",
+					       value & ~0x80);
+					continue;
+				}
+				input_report_key(toshiba_acpi.hotkey_dev,
+						 key->keycode, 1);
+				input_sync(toshiba_acpi.hotkey_dev);
+				input_report_key(toshiba_acpi.hotkey_dev,
+						 key->keycode, 0);
+				input_sync(toshiba_acpi.hotkey_dev);
+			}
+		} else if (hci_result == HCI_NOT_SUPPORTED) {
+			/* This is a workaround for an unresolved issue on
+			 * some machines where system events sporadically
+			 * become disabled. */
+			hci_write1(HCI_SYSTEM_EVENT, 1, &hci_result);
+			printk(MY_NOTICE "Re-enabled hotkeys\n");
+		}
+	} while (hci_result != HCI_EMPTY);
+}
+
+static int toshiba_acpi_setup_keyboard(char *device)
+{
+	acpi_status status;
+	acpi_handle handle;
+	int result;
+	const struct key_entry *key;
+
+	status = acpi_get_handle(NULL, device, &handle);
+	if (ACPI_FAILURE(status)) {
+		printk(MY_INFO "Unable to get notification device\n");
+		return -ENODEV;
+	}
+
+	toshiba_acpi.handle = handle;
+
+	status = acpi_evaluate_object(handle, "ENAB", NULL, NULL);
+	if (ACPI_FAILURE(status)) {
+		printk(MY_INFO "Unable to enable hotkeys\n");
+		return -ENODEV;
+	}
+
+	status = acpi_install_notify_handler (handle, ACPI_DEVICE_NOTIFY,
+					      toshiba_acpi_notify, NULL);
+	if (ACPI_FAILURE(status)) {
+		printk(MY_INFO "Unable to install hotkey notification\n");
+		return -ENODEV;
+	}
+
+	toshiba_acpi.hotkey_dev = input_allocate_device();
+	if (!toshiba_acpi.hotkey_dev) {
+		printk(MY_INFO "Unable to register input device\n");
+		return -ENOMEM;
+	}
+
+	toshiba_acpi.hotkey_dev->name = "Toshiba input device";
+	toshiba_acpi.hotkey_dev->phys = device;
+	toshiba_acpi.hotkey_dev->id.bustype = BUS_HOST;
+	toshiba_acpi.hotkey_dev->getkeycode = toshiba_acpi_getkeycode;
+	toshiba_acpi.hotkey_dev->setkeycode = toshiba_acpi_setkeycode;
+
+	for (key = toshiba_acpi_keymap; key->type != KE_END; key++) {
+		set_bit(EV_KEY, toshiba_acpi.hotkey_dev->evbit);
+		set_bit(key->keycode, toshiba_acpi.hotkey_dev->keybit);
+	}
+
+	result = input_register_device(toshiba_acpi.hotkey_dev);
+	if (result) {
+		printk(MY_INFO "Unable to register input device\n");
+		return result;
+	}
+
+	return 0;
+}
+
 static void toshiba_acpi_exit(void)
 {
 	if (toshiba_acpi.poll_dev) {
@@ -709,12 +891,18 @@ static void toshiba_acpi_exit(void)
 		input_free_polled_device(toshiba_acpi.poll_dev);
 	}
 
+	if (toshiba_acpi.hotkey_dev)
+		input_unregister_device(toshiba_acpi.hotkey_dev);
+
 	if (toshiba_acpi.rfk_dev)
 		rfkill_unregister(toshiba_acpi.rfk_dev);
 
 	if (toshiba_backlight_device)
 		backlight_device_unregister(toshiba_backlight_device);
 
+	acpi_remove_notify_handler(toshiba_acpi.handle, ACPI_DEVICE_NOTIFY,
+				   toshiba_acpi_notify);
+
 	remove_device();
 
 	if (toshiba_proc_dir)
@@ -738,11 +926,15 @@ static int __init toshiba_acpi_init(void)
 		return -ENODEV;
 
 	/* simple device detection: look for HCI method */
-	if (is_valid_acpi_path(METHOD_HCI_1))
-		method_hci = METHOD_HCI_1;
-	else if (is_valid_acpi_path(METHOD_HCI_2))
-		method_hci = METHOD_HCI_2;
-	else
+	if (is_valid_acpi_path(TOSH_INTERFACE_1 GHCI_METHOD)) {
+		method_hci = TOSH_INTERFACE_1 GHCI_METHOD;
+		if (toshiba_acpi_setup_keyboard(TOSH_INTERFACE_1))
+			printk(MY_INFO "Unable to activate hotkeys\n");
+	} else if (is_valid_acpi_path(TOSH_INTERFACE_2 GHCI_METHOD)) {
+		method_hci = TOSH_INTERFACE_2 GHCI_METHOD;
+		if (toshiba_acpi_setup_keyboard(TOSH_INTERFACE_2))
+			printk(MY_INFO "Unable to activate hotkeys\n");
+	} else
 		return -ENODEV;
 
 	printk(MY_INFO "Toshiba Laptop ACPI Extras version %s\n",

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Richard Hughes]

On Fri, 2009-03-06 at 00:52 +0000, Matthew Garrett wrote:
> Calling the ENAB method on Toshiba laptops results in notifications 
> being sent when laptop hotkeys are pressed. This patch simply calls that 
> method and sets up an input device if it's successful.

Great news - no polling!

Definitely +1 from me.

Richard.

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Daniel Silverstone]

On Fri, 2009-03-06 at 09:08 +0000, Richard Hughes wrote:
> On Fri, 2009-03-06 at 00:52 +0000, Matthew Garrett wrote:
> > Calling the ENAB method on Toshiba laptops results in notifications 
> > being sent when laptop hotkeys are pressed. This patch simply calls that 
> > method and sets up an input device if it's successful.
> Great news - no polling!

No polling is definitely a good thing.

> Definitely +1 from me.

I'll be a touch less gung-ho than Richard though.

Have you looked at whether or not this method functions on more than the
one laptop? Toshiba are notoriously good at getting their own interfaces
wrong from one laptop to another. In addition, the fn+whatever keymaps
are often different between laptops, especially for things like the WWW
or MAIL buttons. Presumably if the hotkeys fail to activate then the
normal /proc/acpi/toshiba/keys thing will continue?

How will it interact with software stacks like HAL when the lock button
is pressed?

The patch itself looks clean and nice, I'm just concerned about its
behaviour from laptop-to-laptop. Particularly the key-map thing.

D.

-- 
Daniel Silverstone                              http://www.simtec.co.uk/
PGP mail accepted and encouraged.            Key Id: 2BC8 4016 2068 7895

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Matthew Garrett]

On Fri, Mar 06, 2009 at 09:47:23AM +0000, Daniel Silverstone wrote:

> Have you looked at whether or not this method functions on more than the
> one laptop? Toshiba are notoriously good at getting their own interfaces
> wrong from one laptop to another.

It's present on every Toshiba DSDT I have that has a VALD/VALZ method.

> In addition, the fn+whatever keymaps are often different between 
> laptops, especially for things like the WWW or MAIL buttons. 
> Presumably if the hotkeys fail to activate then the normal 
> /proc/acpi/toshiba/keys thing will continue?

Yes, it'll be as functional as it was previously. They can be remapped 
on machines that have a different keymap.
 
> How will it interact with software stacks like HAL when the lock button
> is pressed?

I don't really understand the question?
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Daniel Silverstone]

On Fri, 2009-03-06 at 09:56 +0000, Matthew Garrett wrote:
> > Have you looked at whether or not this method functions on more than the
> > one laptop? Toshiba are notoriously good at getting their own interfaces
> > wrong from one laptop to another.
> It's present on every Toshiba DSDT I have that has a VALD/VALZ method.

Nod, I shall have to have a check through the ones here if I can.

> > In addition, the fn+whatever keymaps are often different between 
> > laptops, especially for things like the WWW or MAIL buttons. 
> > Presumably if the hotkeys fail to activate then the normal 
> > /proc/acpi/toshiba/keys thing will continue?
> Yes, it'll be as functional as it was previously. They can be remapped 
> on machines that have a different keymap.

Since I don't understand the input layer well enough yet, can you
confirm if it is possible to add new mappings in without changing the
source? I.E. if laptop <foo> has another hotkey not yet considered, is
it just an FDI for hal, or is it a source change in the kernel?

> > How will it interact with software stacks like HAL when the lock button
> > is pressed?
> I don't really understand the question?

It was more: will events come out of both the notify method *and*
the /proc/acpi/toshiba/keys stuff, or will they only come out of the
notify method if it can be enabled? (I'm just trying to establish that
things polling /proc/acpi/toshiba/keys won't end up causing duplicated
events).

Assuming I'm just being over-paranoid or not understanding the minutae,
then I give this a +1 because the behaviour is certainly desirable since
it'll allow the tosh laptops to not wake up regularly to check for
hotkeys.

D.

-- 
Daniel Silverstone                              http://www.simtec.co.uk/
PGP mail accepted and encouraged.            Key Id: 2BC8 4016 2068 7895

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Matthew Garrett]

On Fri, Mar 06, 2009 at 10:04:21AM +0000, Daniel Silverstone wrote:
> On Fri, 2009-03-06 at 09:56 +0000, Matthew Garrett wrote:
> > Yes, it'll be as functional as it was previously. They can be remapped 
> > on machines that have a different keymap.
> 
> Since I don't understand the input layer well enough yet, can you
> confirm if it is possible to add new mappings in without changing the
> source? I.E. if laptop <foo> has another hotkey not yet considered, is
> it just an FDI for hal, or is it a source change in the kernel?

Yes, the remapping can be done from userspace.

> > > How will it interact with software stacks like HAL when the lock button
> > > is pressed?
> > I don't really understand the question?
> 
> It was more: will events come out of both the notify method *and*
> the /proc/acpi/toshiba/keys stuff, or will they only come out of the
> notify method if it can be enabled? (I'm just trying to establish that
> things polling /proc/acpi/toshiba/keys won't end up causing duplicated
> events).

Either the kernel or userspace will get the event, but not both. There's 
an argument for disabling the input code if /proc/acpi/toshiba/keys is 
open in order to avoid breaking anything that expects to get the code 
itself.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Daniel Silverstone]

On Fri, 2009-03-06 at 10:09 +0000, Matthew Garrett wrote:
> > > > How will it interact with software stacks like HAL when the lock button
> > > > is pressed?
> > > I don't really understand the question?
> > It was more: will events come out of both the notify method *and*
> > the /proc/acpi/toshiba/keys stuff, or will they only come out of the
> > notify method if it can be enabled? (I'm just trying to establish that
> > things polling /proc/acpi/toshiba/keys won't end up causing duplicated
> > events).
> Either the kernel or userspace will get the event, but not both. There's 
> an argument for disabling the input code if /proc/acpi/toshiba/keys is 
> open in order to avoid breaking anything that expects to get the code 
> itself.

I see. One other thing occurred to me. Is there any way for Hal to know
that the notify stuff is in place and thus doesn't need to
poll /proc/acpi/toshiba/keys? Otherwise it's going to poll it anyway
which fails to remove the periodic wakeups.

D.

-- 
Daniel Silverstone                              http://www.simtec.co.uk/
PGP mail accepted and encouraged.            Key Id: 2BC8 4016 2068 7895

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Matthew Garrett]

On Fri, Mar 06, 2009 at 10:12:56AM +0000, Daniel Silverstone wrote:

> I see. One other thing occurred to me. Is there any way for Hal to know
> that the notify stuff is in place and thus doesn't need to
> poll /proc/acpi/toshiba/keys? Otherwise it's going to poll it anyway
> which fails to remove the periodic wakeups.

No. Distributions should build without --enable-toshiba if they ship a 
kernel with this functionaliy.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Daniel Silverstone]

On Fri, 2009-03-06 at 10:15 +0000, Matthew Garrett wrote:
> > I see. One other thing occurred to me. Is there any way for Hal to know
> > that the notify stuff is in place and thus doesn't need to
> > poll /proc/acpi/toshiba/keys? Otherwise it's going to poll it anyway
> > which fails to remove the periodic wakeups.
> No. Distributions should build without --enable-toshiba if they ship a 
> kernel with this functionaliy.

This only makes sense if all the rest of /proc/acpi/toshiba/* is now
implemented in proper modern kernel facilities. I noticed rfkill
switches (although I'll have to see if another is needed for the HSDPA
modems in modern laptops) and a proper brightness control. I assume
xrandr does what video did, which just leaves the fan control.

D.

-- 
Daniel Silverstone                              http://www.simtec.co.uk/
PGP mail accepted and encouraged.            Key Id: 2BC8 4016 2068 7895

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Andrey Borzenkov]

Matthew Garrett wrote:

> Calling the ENAB method on Toshiba laptops results in notifications
> being sent when laptop hotkeys are pressed. This patch simply calls that
> method and sets up an input device if it's successful.
>     
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> 

Works on my Toshiba Portege 4000. I tried to enable it directly in keyboard  controller using 
information from Toshiba Linux pages but it failed. Thank you!

Tested-by: Andrey Borzenkov <arvidjaar@mail.ru>

Trivial cleanup below. Also the patch should remove /proc/acpi/toshiba/keys. It will consume hot keys 
as soon as they are pressed, so user space watching this file has very little chances to ever see 
them.

---

Subject: [PATCH] Trivial fixes to enable Toshiba keyboard patch
From: Andrey Borzenkov <arvidjaar@mail.ru>

Fix type of third parameter of toshiba_acpi_notify.
Remove extraneous white space in function invocation.

Signed-off-by: Andrey Borzenkov <arvidjaar@mail.ru>

---

 drivers/platform/x86/toshiba_acpi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)


diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 604f9fa..c334007 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -793,7 +793,7 @@ static int toshiba_acpi_setkeycode(struct input_dev *dev, int scancode,
 	return -EINVAL;
 }
 
-static void toshiba_acpi_notify(acpi_handle handle, u32 event, void **data)
+static void toshiba_acpi_notify(acpi_handle handle, u32 event, void *data)
 {
 	u32 hci_result, value;
 	struct key_entry *key;
@@ -851,7 +851,7 @@ static int toshiba_acpi_setup_keyboard(char *device)
 		return -ENODEV;
 	}
 
-	status = acpi_install_notify_handler (handle, ACPI_DEVICE_NOTIFY,
+	status = acpi_install_notify_handler(handle, ACPI_DEVICE_NOTIFY,
 					      toshiba_acpi_notify, NULL);
 	if (ACPI_FAILURE(status)) {
 		printk(MY_INFO "Unable to install hotkey notification\n");

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Matthew Garrett]

On Fri, Mar 06, 2009 at 09:37:50PM +0300, Andrey Borzenkov wrote:

> Trivial cleanup below. Also the patch should remove /proc/acpi/toshiba/keys. It will consume hot keys 
> as soon as they are pressed, so user space watching this file has very little chances to ever see 
> them.

Mm. Actually, thinking about it, the cleanest thing to do would probably 
be to disable the input device when the proc file is opened.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Andrey Borzenkov]

Daniel Silverstone wrote:

> On Fri, 2009-03-06 at 10:15 +0000, Matthew Garrett wrote:
>> > I see. One other thing occurred to me. Is there any way for Hal to know
>> > that the notify stuff is in place and thus doesn't need to
>> > poll /proc/acpi/toshiba/keys? Otherwise it's going to poll it anyway
>> > which fails to remove the periodic wakeups.
>> No. Distributions should build without --enable-toshiba if they ship a
>> kernel with this functionaliy.
> 
> This only makes sense if all the rest of /proc/acpi/toshiba/* is now
> implemented in proper modern kernel facilities.

I guess Matthew refers to HAL build switch. In which case only 
/proc/acpi/toshiba/keys is relevant, other files are not used by this HAL 
add-on.

In any case, as I mentioned, this file has to be removed if patch is 
implemented as it is effectively no-op then.

> I noticed rfkill
> switches (although I'll have to see if another is needed for the HSDPA
> modems in modern laptops) and a proper brightness control. I assume
> xrandr does what video did, which just leaves the fan control.
> 

There are two ways to control fan here - standard ACPI and HCI. ACPI does 
not work. Is there any standard framework for fan control (a la 
power_supply) where HCI version could be plugged in?

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Matthew Garrett]

On Fri, Mar 06, 2009 at 09:49:36PM +0300, Andrey Borzenkov wrote:

> There are two ways to control fan here - standard ACPI and HCI. ACPI does 
> not work. Is there any standard framework for fan control (a la 
> power_supply) where HCI version could be plugged in?

Yes, it could be hooked into the thermal or hwmon layers. Thermal is 
probably best - it'll then automatically be exported as an hwmon device 
as well.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Andrey Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]

On 6 марта 2009 21:44:52 Matthew Garrett wrote:
> On Fri, Mar 06, 2009 at 09:37:50PM +0300, Andrey Borzenkov wrote:
> > Trivial cleanup below. Also the patch should remove
> > /proc/acpi/toshiba/keys. It will consume hot keys as soon as they
> > are pressed, so user space watching this file has very little
> > chances to ever see them.
>
> Mm. Actually, thinking about it, the cleanest thing to do would
> probably be to disable the input device when the proc file is opened.

C'mon, really. Let's make it depend on _DEPRECATED and obsolete in 
2.6.31. Who needs this now?



[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Andrey Borzenkov]

Matthew Garrett wrote:

> +	{KE_KEY, 0x13d, KEY_SLEEP},
> +	{KE_KEY, 0x13e, KEY_SUSPEND},

I have two buttons marked with memory and disk pictures. When I press the 
first one HAL emits "sleep" button event, for the the second one HAL emits 
"hibernate" event. I am using KDE4 and neither works :) According to KDE4 
developer, they implement "suspend" button as suspend to RAM. Just trying to 
clarify which key this should be and whether HAL should be fixed.  (I opened 
bug report for KDE4)

> +	{KE_KEY, 0x13f, KEY_SWITCHVIDEOMODE},

I wonder, who is supposed to act upon it? Is there any generic user space 
agent who implements video output switching?

> +	{KE_KEY, 0x140, KEY_BRIGHTNESSDOWN},
> +	{KE_KEY, 0x141, KEY_BRIGHTNESSUP},
> +	{KE_KEY, 0x142, KEY_WLAN},

Ditto. Theoretically Toshiba even supports turning off radio via HCI, but it 
is again not clear who should actually initiate it.

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Matthew Garrett]

On Sat, Mar 07, 2009 at 10:27:09AM +0300, Andrey Borzenkov wrote:
> Matthew Garrett wrote:
> 
> > +	{KE_KEY, 0x13d, KEY_SLEEP},
> > +	{KE_KEY, 0x13e, KEY_SUSPEND},
> 
> I have two buttons marked with memory and disk pictures. When I press the 
> first one HAL emits "sleep" button event, for the the second one HAL emits 
> "hibernate" event. I am using KDE4 and neither works :) According to KDE4 
> developer, they implement "suspend" button as suspend to RAM. Just trying to 
> clarify which key this should be and whether HAL should be fixed.  (I opened 
> bug report for KDE4)

Yeah, I'm not really a KDE guy, so I'm not sure what's happening there.

> > +	{KE_KEY, 0x13f, KEY_SWITCHVIDEOMODE},
> 
> I wonder, who is supposed to act upon it? Is there any generic user space 
> agent who implements video output switching?

At present I don't believe so, no.

> > +	{KE_KEY, 0x140, KEY_BRIGHTNESSDOWN},
> > +	{KE_KEY, 0x141, KEY_BRIGHTNESSUP},
> > +	{KE_KEY, 0x142, KEY_WLAN},
> 
> Ditto. Theoretically Toshiba even supports turning off radio via HCI, but it 
> is again not clear who should actually initiate it.

Right. In some of these cases there's nothing that currently handles 
them, but userspace should probably get round to it at some point.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Andrey Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 1599 bytes --]

On 7 марта 2009 18:06:40 Matthew Garrett wrote:
> On Sat, Mar 07, 2009 at 10:27:09AM +0300, Andrey Borzenkov wrote:
> > Matthew Garrett wrote:
> > > +	{KE_KEY, 0x13d, KEY_SLEEP},
> > > +	{KE_KEY, 0x13e, KEY_SUSPEND},
> >
> > I have two buttons marked with memory and disk pictures. When I
> > press the first one HAL emits "sleep" button event, for the the
> > second one HAL emits "hibernate" event. I am using KDE4 and neither
> > works :) According to KDE4 developer, they implement "suspend"
> > button as suspend to RAM. Just trying to clarify which key this
> > should be and whether HAL should be fixed.  (I opened bug report
> > for KDE4)
>
> Yeah, I'm not really a KDE guy, so I'm not sure what's happening
> there.
>

Please see reply to another thread titled "Re: suspend / hibernate 
nomenclature".  What happens here is

- addon-acpi-buttons-toshiba emitted "suspend" for Fn-F3 and "hibernate" 
for Fn-F4

- your patch makes HAL emit "sleep" for Fn-F3 and "hibernate" for Fn-F4

So the patch is incompatible change w.r.t. user space. To restore 
previous behaviour we need

- patch toshiba_acpi to return KEY_SUSPEND/KEY_HIBERNATE instead of 
KEY_SLEEP/KEY_SUSPEND. This depends on commit 
6932b918e05b06165ed3457a9f3aa279099a7cbd in linux-next.

- patch HAL to recognize KEY_HIBERNATE and return "suspend" for 
KEY_SUSPEND; right now it is:

        [KEY_SLEEP] = "sleep",
        [KEY_SUSPEND] = "hibernate",

In any case this means that combination of old HAL and new kernel (or 
vice versa) becomes broken. Not sure how to handle it.


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Matthew Garrett]

On Sat, Mar 07, 2009 at 06:38:53PM +0300, Andrey Borzenkov wrote:

> - patch toshiba_acpi to return KEY_SUSPEND/KEY_HIBERNATE instead of 
> KEY_SLEEP/KEY_SUSPEND. This depends on commit 
> 6932b918e05b06165ed3457a9f3aa279099a7cbd in linux-next.
> 
> - patch HAL to recognize KEY_HIBERNATE and return "suspend" for 
> KEY_SUSPEND; right now it is:
> 
>         [KEY_SLEEP] = "sleep",
>         [KEY_SUSPEND] = "hibernate",

Ugh. Why are we changing this? The semantics were pretty clear before. 
The KEY_SUSPEND to hibernate mapping was decided years ago, and it's 
clearly an incompatible change as far as userspace goes.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Richard Hughes]

On Sat, Mar 7, 2009 at 3:44 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Sat, Mar 07, 2009 at 06:38:53PM +0300, Andrey Borzenkov wrote:
>
>> - patch toshiba_acpi to return KEY_SUSPEND/KEY_HIBERNATE instead of
>> KEY_SLEEP/KEY_SUSPEND. This depends on commit
>> 6932b918e05b06165ed3457a9f3aa279099a7cbd in linux-next.
>>
>> - patch HAL to recognize KEY_HIBERNATE and return "suspend" for
>> KEY_SUSPEND; right now it is:
>>
>>         [KEY_SLEEP] = "sleep",
>>         [KEY_SUSPEND] = "hibernate",
>
> Ugh. Why are we changing this? The semantics were pretty clear before.
> The KEY_SUSPEND to hibernate mapping was decided years ago, and it's
> clearly an incompatible change as far as userspace goes.

See my mails to linux-acpi. Hibernate = sleep to disk, suspend = sleep
to ram, and sleep = sleep type not indicated or unknown. This is how
it is in Xorg and the session now.

Mapping KEY_SUSPEND to hibernate is just insane. Can you please change
the toshiba driver to use KEY_HIBERNATE and KEY_SUSPEND as thinkpad
now does? Thanks.

Richard.

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Matthew Garrett]

On Sat, Mar 07, 2009 at 08:19:51PM +0000, Richard Hughes wrote:

> Mapping KEY_SUSPEND to hibernate is just insane. Can you please change
> the toshiba driver to use KEY_HIBERNATE and KEY_SUSPEND as thinkpad
> now does? Thanks.

Mapping KEY_SUSPEND to hibernate is what we've been doing for years. 
It's what hal *still does*. KEY_SLEEP has been the suspend to RAM key 
forever. How are we supposed to perform this transition? We've no idea 
how much of userspace makes the same assumption.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Richard Hughes]

On Sat, Mar 7, 2009 at 8:26 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Sat, Mar 07, 2009 at 08:19:51PM +0000, Richard Hughes wrote:
>
>> Mapping KEY_SUSPEND to hibernate is just insane. Can you please change
>> the toshiba driver to use KEY_HIBERNATE and KEY_SUSPEND as thinkpad
>> now does? Thanks.
>
> Mapping KEY_SUSPEND to hibernate is what we've been doing for years.
> It's what hal *still does*.

Sure, but how much userspace now listens to HAL for these events? Xorg
and evdev has taken over that role for all the session. We can ship a
trivial patch as an fdi file to HAL to remap this if required.

> KEY_SLEEP has been the suspend to RAM key forever.

Except if you're a USB keyboard. Grep through the kernel sources and
see how many drivers get this wrong. We can't map three sleep states
to two buttons in any sane way. For instance, is the sleep acpi button
supposed to trigger a suspend of hibernate? Surely this is user policy
as it is not specified on the the exterior of the machine.

> How are we supposed to perform this transition? We've no idea
> how much of userspace makes the same assumption.

FWIW, I think emitting KEY_ events (not switch events) in HAL is crazy
as now we can just use the fixed Xorg in the session. FWIW, HAL gets
other keys wrong too, for instance KEY_BATTERY is mapped to
display_off, but nobody has noticed as we've been using Xorg since
ages.

Richard.

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Andrey Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 1615 bytes --]

On 8 марта 2009 11:33:48 Richard Hughes wrote:
> On Sat, Mar 7, 2009 at 8:26 PM, Matthew Garrett <mjg59@srcf.ucam.org> 
wrote:
> > On Sat, Mar 07, 2009 at 08:19:51PM +0000, Richard Hughes wrote:
> >> Mapping KEY_SUSPEND to hibernate is just insane. Can you please
> >> change the toshiba driver to use KEY_HIBERNATE and KEY_SUSPEND as
> >> thinkpad now does? Thanks.
> >
> > Mapping KEY_SUSPEND to hibernate is what we've been doing for
> > years. It's what hal *still does*.
>
> Sure, but how much userspace now listens to HAL for these events?

Apparently KDE still does. At least it does not seem to pay any 
attention to KEY_SUSPEND (nor KEY_SLEEP BTW).

And KDE seems to be important enough customer to not wish to break HAL.

> Xorg and evdev has taken over that role for all the session.

Oh, wait. But even Xorg 1.6.0 interprets KEY_SUSPEND as "hibernate".

KeyPress event, serial 31, synthetic NO, window 0x3e00001,
    root 0xbc, subw 0x0, time 87299792, (81,-11), root:(817,290),
    state 0x0, keycode 213 (keysym 0x1008ffa8, XF86Hibernate), 
same_screen YES,
    XLookupString gives 0 bytes:
    XmbLookupString gives 0 bytes:
    XFilterEvent returns: False

So redefining KEY_SUSPEND is going to break Xorg too (as long as anyone 
is using those keysyms).

> We can
> ship a trivial patch as an fdi file to HAL to remap this if required.
>
> > KEY_SLEEP has been the suspend to RAM key forever.

Ehh ... actually at least in Toshiba case it was not :) hald Toshiba 
add-on always emitted exactly "suspend" and "hibernate" D-Bus events. 
Not "sleep".



[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Matthew Garrett]

On Sun, Mar 08, 2009 at 08:33:48AM +0000, Richard Hughes wrote:
> On Sat, Mar 7, 2009 at 8:26 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > Mapping KEY_SUSPEND to hibernate is what we've been doing for years.
> > It's what hal *still does*.
> 
> Sure, but how much userspace now listens to HAL for these events? Xorg
> and evdev has taken over that role for all the session. We can ship a
> trivial patch as an fdi file to HAL to remap this if required.

I've no idea. But I lean towards not gratuitously breaking it for no 
reason other than aesthetics.

> > KEY_SLEEP has been the suspend to RAM key forever.
> 
> Except if you're a USB keyboard. Grep through the kernel sources and
> see how many drivers get this wrong. We can't map three sleep states
> to two buttons in any sane way. For instance, is the sleep acpi button
> supposed to trigger a suspend of hibernate? Surely this is user policy
> as it is not specified on the the exterior of the machine.

We do it the same way we've previously done it (and the same way it 
works outside the Linux world) - the "suspend to RAM" key has 
configurable behaviour. As far as I can tell, the USB driver does the 
right thing here?

> > How are we supposed to perform this transition? We've no idea
> > how much of userspace makes the same assumption.
> 
> FWIW, I think emitting KEY_ events (not switch events) in HAL is crazy
> as now we can just use the fixed Xorg in the session. FWIW, HAL gets
> other keys wrong too, for instance KEY_BATTERY is mapped to
> display_off, but nobody has noticed as we've been using Xorg since
> ages.

I'm happy with obvious bugs being fixed, but this isn't an obvious bug - 
it's purely an aesthetic issue. We don't need to draw a distinction 
between generic sleep and suspend to RAM keys, especially if the cost of 
doing so is having to fix up an undefined quantity of userspace.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] toshiba_acpi: Add full hotkey support

[Len Brown]

On Sun, 8 Mar 2009, Richard Hughes wrote:

> On Sat, Mar 7, 2009 at 8:26 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > On Sat, Mar 07, 2009 at 08:19:51PM +0000, Richard Hughes wrote:
> >
> >> Mapping KEY_SUSPEND to hibernate is just insane. Can you please change
> >> the toshiba driver to use KEY_HIBERNATE and KEY_SUSPEND as thinkpad
> >> now does? Thanks.
> >
> > Mapping KEY_SUSPEND to hibernate is what we've been doing for years.
> > It's what hal *still does*.
> 
> Sure, but how much userspace now listens to HAL for these events? Xorg
> and evdev has taken over that role for all the session. We can ship a
> trivial patch as an fdi file to HAL to remap this if required.
> 
> > KEY_SLEEP has been the suspend to RAM key forever.
> 
> Except if you're a USB keyboard. Grep through the kernel sources and
> see how many drivers get this wrong. We can't map three sleep states
> to two buttons in any sane way. For instance, is the sleep acpi button
> supposed to trigger a suspend of hibernate? Surely this is user policy
> as it is not specified on the the exterior of the machine.

While hot-keys are totally platform dependent and non-standard,
the ACPI spec does describe the power and sleep button.

Unfortunately, it doesn't answer this question for us --
stating that the sleep button enters G1, which can be
any of S1-S4.

Len Brown, Intel Open Source Technology Center