LinuxLists.cc - [PATCH] toshiba_acpi: Add full hotkey support

[permalink] [raw]

Subject: [PATCH] toshiba_acpi: Add full hotkey support

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 <[email protected]>

---

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 | [email protected]

2009-03-06 09:10:14

by Richard Hughes

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

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.

2009-03-06 09:47:42

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

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

2009-03-06 09:56:44

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

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 | [email protected]

2009-03-06 10:04:37

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

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

2009-03-06 10:10:19

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

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 | [email protected]

2009-03-06 10:13:18

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

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

2009-03-06 10:15:49

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

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 | [email protected]

2009-03-06 10:22:05

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

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

2009-03-06 18:38:38

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

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 <[email protected]>
>

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 <[email protected]>

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 <[email protected]>

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

Signed-off-by: Andrey Borzenkov <[email protected]>

---

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");

2009-03-06 18:45:13

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

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 | [email protected]

2009-03-06 18:49:55

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

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?

2009-03-06 18:54:04

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

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 | [email protected]

2009-03-06 18:57:43

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

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?

Attachments:

(No filename) (582.00 B)
signature.asc (197.00 B)
This is a digitally signed message part. Download all attachments

2009-03-07 07:27:29

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

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.

2009-03-07 15:06:53

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

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 | [email protected]

2009-03-07 15:39:38

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

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.

Attachments:

(No filename) (1.52 kB)
signature.asc (197.00 B)
This is a digitally signed message part. Download all attachments

2009-03-07 15:44:39

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

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 | [email protected]

2009-03-07 20:20:08

by Richard Hughes

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

On Sat, Mar 7, 2009 at 3:44 PM, Matthew Garrett <[email protected]> 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.

2009-03-07 20:27:03

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

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 | [email protected]

2009-03-08 08:34:07

by Richard Hughes

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

On Sat, Mar 7, 2009 at 8:26 PM, Matthew Garrett <[email protected]> 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.

2009-03-08 14:30:13

[permalink] [raw]

Subject: Re: [PATCH] toshiba_acpi: Add full hotkey support

On 8 марта 2009 11:33:48 Richard Hughes wrote:
> On Sat, Mar 7, 2009 at 8:26 PM, Matthew Garrett <[email protected]>
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".

Attachments:

(No filename) (1.54 kB)
signature.asc (197.00 B)
This is a digitally signed message part. Download all attachments

2009-03-08 14:36:53