Untangling the sleep hotkey mess

Untangling the sleep hotkey mess

[Matthew Garrett]

Currently, there are three ways that a sleep hotkey may generate an 
event:

1) Exposed in DSDT as an ACPI object, generates an ACPI notification 
event (the "normal" way)

2) Uses hardware-specific ACPI driver, generates an ACPI notification 
event. Not exposed in the DSDT in any standard way (ibm-acpi, 
toshiba-acpi, panasonic-acpi)

3) Generates a scancode, which is picked up by the kernel and turned 
into a keycode (HP laptops work like this)

This is all quite horribly confusing, and makes life miserable for 
userspace. I would like to suggest the following standardisation:

a) Hal should assume that all hardware has a sleep key, since there's no 
way to actually tell.

b) Events generated in cases (1) and (2) should, for now, be caught by 
acpid (or something similar) and then fed back into the input layer via 
uinput. This should be scancode 142, which will end up as X keycode 223.

c) Most keyboards in case (3) will already send scancode 142. For 
laptops, those that shouldn't should be remapped at boot time by 
checking the system DMI information and consulting a lookup table.

Rationale:

Having one type of event rather than three makes it easier for userspace 
coders. Choosing to do it through the input layer lets people take 
advantage of pre-existing code for binding userspace events to keyboard 
events, and is significantly easier to do than getting keyboard events 
back to the ACPI layer. Keycode 142 is chosen because it's what 
Microsoft uses, and so most manufacturers who have taken this approach 
have copied them.

Future:

1) OSDL have just set up a mailing list 
(desktop_portables-qjLDD68F18O7TbgM5vRIOg@public.gmane.org) for discussion of userspace and 
laptop interaction. Can I encourage people who are interested in 
hammering out cross-distribution solutions to problems like this to 
subscribe?

2) /usr/include/linux/input.h defines two keycodes for suspend-type 
behaviour: KEY_SLEEP (scancode 142) and KEY_SUSPEND (scancode 205). I'd 
like to propose using the second of these for keys that trigger suspend 
to disk. There's much less standardisation here, though, so I'd be 
interested in hearing what other people think.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Untangling the sleep hotkey mess

[Henrik Brix Andersen]

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

On Sat, Jan 07, 2006 at 05:24:46PM +0000, Matthew Garrett wrote:
> a) Hal should assume that all hardware has a sleep key, since there's no 
> way to actually tell.

Why bring Hal into this equation?

> b) Events generated in cases (1) and (2) should, for now, be caught by 
> acpid (or something similar) and then fed back into the input layer via 
> uinput. This should be scancode 142, which will end up as X keycode 223.

And how would this work for users who doesn't have Hal installed?

Having ACPI hotkey events delivered as normal keyboard input makes it
difficult to run system scripts as root:root.

Regards,
Brix
-- 
Henrik Brix Andersen <brix-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
Gentoo Metadistribution | Mobile computing herd

[-- Attachment #2: Type: application/pgp-signature, Size: 211 bytes --]

Re: Untangling the sleep hotkey mess

[Matthew Garrett]

On Sat, Jan 07, 2006 at 08:24:44PM +0100, Henrik Brix Andersen wrote:
> On Sat, Jan 07, 2006 at 05:24:46PM +0000, Matthew Garrett wrote:
> > a) Hal should assume that all hardware has a sleep key, since there's no 
> > way to actually tell.
> 
> Why bring Hal into this equation?

Because, over time, applications are likely to start worrying about what 
hal thinks in this respect. gnome-power-manager already listens for hal 
events related to the sleep button.

> > b) Events generated in cases (1) and (2) should, for now, be caught by 
> > acpid (or something similar) and then fed back into the input layer via 
> > uinput. This should be scancode 142, which will end up as X keycode 223.
> 
> And how would this work for users who doesn't have Hal installed?

I'm sorry, I don't understand the question. This is entirely independent 
of hal.

> Having ACPI hotkey events delivered as normal keyboard input makes it
> difficult to run system scripts as root:root.

But that does nothing to help systems which only expose sleep keys via 
the keyboard controller. It's not practical to get them to generate ACPI 
events - however, it /is/ practical to get ACPI events to generate input 
events. At that point there's nothing to stop a hotkey daemon running as 
root.
-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Untangling the sleep hotkey mess

[Henrik Brix Andersen]

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

On Sat, Jan 07, 2006 at 08:52:32PM +0000, Matthew Garrett wrote:
> On Sat, Jan 07, 2006 at 08:24:44PM +0100, Henrik Brix Andersen wrote:
> > Having ACPI hotkey events delivered as normal keyboard input makes it
> > difficult to run system scripts as root:root.
> 
> But that does nothing to help systems which only expose sleep keys via 
> the keyboard controller. It's not practical to get them to generate ACPI 
> events - however, it /is/ practical to get ACPI events to generate input 
> events. At that point there's nothing to stop a hotkey daemon running as 
> root.

So what you really propose is to abstract the handling of ACPI events
by yet another layer, a hotkey daemon?

./Brix
-- 
Henrik Brix Andersen <brix-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
Gentoo Metadistribution | Mobile computing herd

[-- Attachment #2: Type: application/pgp-signature, Size: 211 bytes --]

Re: Untangling the sleep hotkey mess

[Matthew Garrett]

On Sat, Jan 07, 2006 at 10:01:03PM +0100, Henrik Brix Andersen wrote:

> So what you really propose is to abstract the handling of ACPI events
> by yet another layer, a hotkey daemon?

No, what I propose is that sleep keys end up generating the same 
userspace event no matter how they're connected. The easiest way to 
achieve that (that is, the one which requires the least amount of new 
code to be written) would be to do it through the input layer. If you 
then want to respond to events by running something as root, this would 
require a small daemon that monitors input events.

The current situation (where sleep keys may generate three different 
types of events) is getting somewhat untenable.
-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [gpm] Untangling the sleep hotkey mess

[Richard Hughes]

On Sat, 2006-01-07 at 17:24 +0000, Matthew Garrett wrote:
> Currently, there are three ways that a sleep hotkey may generate an 
> event:
> 
> 1) Exposed in DSDT as an ACPI object, generates an ACPI notification 
> event (the "normal" way)
> 
> 2) Uses hardware-specific ACPI driver, generates an ACPI notification 
> event. Not exposed in the DSDT in any standard way (ibm-acpi, 
> toshiba-acpi, panasonic-acpi)
> 
> 3) Generates a scancode, which is picked up by the kernel and turned 
> into a keycode (HP laptops work like this)
> 
> This is all quite horribly confusing, and makes life miserable for 
> userspace.

Wholeheartedly agree.

>  I would like to suggest the following standardisation:
> 
> a) Hal should assume that all hardware has a sleep key, since there's no 
> way to actually tell.
> 
> b) Events generated in cases (1) and (2) should, for now, be caught by 
> acpid (or something similar) and then fed back into the input layer via 
> uinput. This should be scancode 142, which will end up as X keycode 223.

Yes, I proposed something similar here
[http://www.nabble.com/ACPI-to-uinput-bridge-t409384.html] -- using HAL
and uinput to send the keys to user-programs.

> c) Most keyboards in case (3) will already send scancode 142. For 
> laptops, those that shouldn't should be remapped at boot time by 
> checking the system DMI information and consulting a lookup table.

Yes, perhaps using the existing HAL infrastructure with FDI lookup
tables? We already have a HAL addon for watching ACPI events, and doing
things like refreshing the battery values, and emitting conditions for
the common buttons (lid, sleep, power) -- we could just extend this code
in the HAL addon.

The HAL addon is well tested, and plays nice with acpid if required.

> Rationale:
> 
> Having one type of event rather than three makes it easier for userspace 
> coders. Choosing to do it through the input layer lets people take 
> advantage of pre-existing code for binding userspace events to keyboard 
> events, and is significantly easier to do than getting keyboard events 
> back to the ACPI layer. Keycode 142 is chosen because it's what 
> Microsoft uses, and so most manufacturers who have taken this approach 
> have copied them.

Can we not go further and define Dock/UnDock,
BrightnessUp/BrightnessDown, Hibernate, etc?

Richard.

Re: [gpm] Untangling the sleep hotkey mess

[Matthew Garrett]

On Sun, Jan 08, 2006 at 12:58:44PM +0000, Richard Hughes wrote:

> Can we not go further and define Dock/UnDock,
> BrightnessUp/BrightnessDown, Hibernate, etc?

BrightnessUp/BrightnessDown have keycodes defined in 
/usr/src/linux/input.h, so that shouldn't be a problem. I suggested a 
keycode for hibernate (KEY_SUSPEND, with suspend to RAM on KEY_SLEEP). 
I'm less convinced about Dock/Undock - that's a problem that's so far 
from being solved I've no idea what sort of things userspace wants to 
know :)

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [gpm] Untangling the sleep hotkey mess

[Richard Hughes]

On Sun, 2006-01-08 at 13:47 +0000, Matthew Garrett wrote:
> On Sun, Jan 08, 2006 at 12:58:44PM +0000, Richard Hughes wrote:
> 
> > Can we not go further and define Dock/UnDock,
> > BrightnessUp/BrightnessDown, Hibernate, etc?
> 
> BrightnessUp/BrightnessDown have keycodes defined in 
> /usr/src/linux/input.h, so that shouldn't be a problem. I suggested a 
> keycode for hibernate (KEY_SUSPEND, with suspend to RAM on KEY_SLEEP). 

Okay, that's good for me.

> I'm less convinced about Dock/Undock - that's a problem that's so far 
> from being solved I've no idea what sort of things userspace wants to 
> know :)

Sure, just an example of "other stuff". Lock is maybe a better example
as gnome-screensaver (or equiv) can respond to this.

The main problem now is implementation.

1. We use the current hodge-podge of methods now (like sony-pi, toshiba,
asus, raw etc) and define backends for all these modules.

2. We use the raw acpi device like Ubuntu do (exposes /dev/acpi) -- Is
this a long term solution? And this requires a kernel patch that I think
the linux-acpi people are unhappy with, and distros such as Fedora
wouldn't like (due to not being in vanilla kernel).

3. Use the new acpi kernel hotkeys stuff -- not that I really understand
the interface, nor how to use it correctly. Plus I'm not sure how much
of the vendor kernel modules can use this new interface, or if it's
suitable. It all seemed a bit fragile last time I looked.

Richard.

-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [gpm] Untangling the sleep hotkey mess

[Yu Luming]

>  3. Use the new acpi kernel hotkeys stuff -- not that I really understand
>  the interface, nor how to use it correctly. Plus I'm not sure how much
>  of the vendor kernel modules can use this new interface, or if it's
>  suitable. It all seemed a bit fragile last time I looked.

Please take a look at 
http://bugzilla.kernel.org/show_bug.cgi?id=5749

There are some examples to use hotkey.c for specific laptops with dedicated
hotkey acpi device objects , and dedicated AML methods.
This patch: http://bugzilla.kernel.org/show_bug.cgi?id=5749#c3
can be used to replace sony_acpi.c

This patch: http://bugzilla.kernel.org/show_bug.cgi?id=5749#c4
can be used to support brightness control for panasonic laptop.

This patch: http://bugzilla.kernel.org/show_bug.cgi?id=5749#c5
can be used to support brighness contrl for ASUS laptop.

Please notes, I will update these patch against len's testing tree, and 
ask for inclusion.

-- 
Thanks,
Luming
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [gpm] Untangling the sleep hotkey mess

[Richard Hughes]

On Mon, 2006-01-09 at 09:10 +0800, Yu Luming wrote:
> >  3. Use the new acpi kernel hotkeys stuff -- not that I really understand
> >  the interface, nor how to use it correctly. Plus I'm not sure how much
> >  of the vendor kernel modules can use this new interface, or if it's
> >  suitable. It all seemed a bit fragile last time I looked.
> 
> Please take a look at 
> http://bugzilla.kernel.org/show_bug.cgi?id=5749
> 
> There are some examples to use hotkey.c for specific laptops with dedicated
> hotkey acpi device objects , and dedicated AML methods.
> This patch: http://bugzilla.kernel.org/show_bug.cgi?id=5749#c3
> can be used to replace sony_acpi.c
> 
> This patch: http://bugzilla.kernel.org/show_bug.cgi?id=5749#c4
> can be used to support brightness control for panasonic laptop.
> 
> This patch: http://bugzilla.kernel.org/show_bug.cgi?id=5749#c5
> can be used to support brighness contrl for ASUS laptop.
> 
> Please notes, I will update these patch against len's testing tree, and 
> ask for inclusion.

Why do we want to use the new hotkey stuff when we can just process all
the events in userspace using acpi events? The hotkey stuff seems
overcomplicated and fragile in my opinion.

Sending and receiving :::data:in:some::odd:format seems a little unusual
to me when we can just listen for the event in one place (like HAL) --
but then maybe I don't understand all the details. It's not like we will
have different programs registering for different acpi button events,
right?

Richard.

Re: [gpm] Untangling the sleep hotkey mess

[Richard Hughes]

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

On Sun, 2006-01-08 at 14:13 +0000, Richard Hughes wrote:
> On Sun, 2006-01-08 at 13:47 +0000, Matthew Garrett wrote:
> > On Sun, Jan 08, 2006 at 12:58:44PM +0000, Richard Hughes wrote:
> > 
> > > Can we not go further and define Dock/UnDock,
> > > BrightnessUp/BrightnessDown, Hibernate, etc?
> > 
> > BrightnessUp/BrightnessDown have keycodes defined in 
> > /usr/src/linux/input.h, so that shouldn't be a problem. I suggested a 
> > keycode for hibernate (KEY_SUSPEND, with suspend to RAM on KEY_SLEEP). 
> 
> Okay, that's good for me.
> 
> > I'm less convinced about Dock/Undock - that's a problem that's so far 
> > from being solved I've no idea what sort of things userspace wants to 
> > know :)
> 
> Sure, just an example of "other stuff". Lock is maybe a better example
> as gnome-screensaver (or equiv) can respond to this.
> 
> The main problem now is implementation.

Further to the conversation on IRC, I've attached two ACPI patches to
provoke discussion.

The first creates the /org/freedesktop/Hal/devices/acpi_uinput like I
did for the toshiba specific HAL addon.
The second uses the acpi events for creating uinput events that can be
captured using gnome-keybindings, and also creates HAL ButtonPressed
events so that DBUS aware applications (like g-v-m and g-p-m) can
monitor the events.

To use this on toshiba, you have to use the kernel patch:
http://www.kernel.org/git/?p=linux/kernel/git/bcollins/ubuntu-2.6.git;a=commitdiff;h=84bd08e019f34628e6e5c276115db5baa46ea824;hp=cfed69e108d0ff7e3cc1e9330f662a1e758eb04d
which may be upstreamed soon. Plus you'll need 
"options toshiba_acpi hotkeys_over_acpi=1" in /etc/modprobe.conf to
enable the new stuff.

Also, to create the input events, you have to "modprobe uinput" and then
restart haldaemon -- so you could say this patch is rough and ready. :-)

Adding other id's (for example for IBM), the keys will just work with no
kernel patches required. Toshiba is special as the interface is unique
(and broken), so that's why we need the patch. Thanks to Matthew for the
pointer to the patch.

This 30 minute hack works for me, but has had little real-world testing.

Comments please.

Richard.

[-- Attachment #2: hal-add-vbutton.patch --]
[-- Type: text/x-patch, Size: 2342 bytes --]

Index: hald/linux2/acpi.c
===================================================================
RCS file: /cvs/hal/hal/hald/linux2/acpi.c,v
retrieving revision 1.43
diff -u -r1.43 acpi.c
--- hald/linux2/acpi.c	2 Dec 2005 17:11:00 -0000	1.43
+++ hald/linux2/acpi.c	2 Dec 2005 18:09:30 -0000
@@ -47,6 +47,7 @@
 	ACPI_TYPE_IBM_DISPLAY,
 	ACPI_TYPE_PANASONIC_DISPLAY,
 	ACPI_TYPE_SONY_DISPLAY,
+	ACPI_TYPE_VBUTTON,
 	ACPI_TYPE_BUTTON
 };
 
@@ -375,6 +376,27 @@
 	return TRUE;
 }
 
+static gboolean
+vbutton_refresh (HalDevice *d, ACPIDevHandler *handler)
+{
+	const char *path;
+	path = hal_device_property_get_string (d, "linux.acpi_path");
+	if (path == NULL)
+		return FALSE;
+
+	/* only set up device new if really needed */
+	if (!hal_device_has_capability (d, "button")){
+		device_property_atomic_update_begin ();
+		hal_device_property_set_string (d, "info.product", "Virtual Button");
+		hal_device_property_set_string (d, "info.category", "button");
+		hal_device_property_set_string (d, "button.type", "virtual");
+		hal_device_property_set_bool (d, "button.has_state", FALSE);
+		hal_device_add_capability (d, "button");
+		device_property_atomic_update_end ();
+	}
+	return TRUE;
+}
+
 /** Removes all the possible battery.* keys.
  *
  *  @param	d		Valid battery HalDevice
@@ -880,6 +902,10 @@
 	snprintf (path, sizeof (path), "%s/acpi/button/sleep", get_hal_proc_path ());
 	acpi_synthesize (path, ACPI_TYPE_BUTTON);
 
+	/* create the extra vbutton button */
+	snprintf (path, sizeof (path), "/dev/uinput");
+	acpi_synthesize_item (path, ACPI_TYPE_VBUTTON);
+
 	/*
 	 * Collect video adaptors (from vendor added modules)
 	 * I *know* we should use the /proc/acpi/video/LCD method, but this
@@ -1022,12 +1048,21 @@
 	.remove      = acpi_generic_remove
 };
 
+static ACPIDevHandler acpidev_handler_vbutton = {
+	.acpi_type   = ACPI_TYPE_VBUTTON,
+	.add         = acpi_generic_add,
+	.compute_udi = acpi_generic_compute_udi,
+	.refresh     = vbutton_refresh,
+	.remove      = acpi_generic_remove
+};
+
 static ACPIDevHandler *acpi_handlers[] = {
 	&acpidev_handler_battery,
 	&acpidev_handler_processor,
 	&acpidev_handler_fan,
 	&acpidev_handler_button,
 	&acpidev_handler_ac_adapter,
+	&acpidev_handler_vbutton,
 	&acpidev_handler_laptop_panel_toshiba,
 	&acpidev_handler_laptop_panel_ibm,
 	&acpidev_handler_laptop_panel_panasonic,

[-- Attachment #3: hal-uinput-new_part.patch --]
[-- Type: text/x-patch, Size: 5751 bytes --]

Index: hald/linux2/addons/addon-acpi.c
===================================================================
RCS file: /cvs/hal/hal/hald/linux2/addons/addon-acpi.c,v
retrieving revision 1.13
diff -u -r1.13 addon-acpi.c
--- hald/linux2/addons/addon-acpi.c	9 Nov 2005 21:13:30 -0000	1.13
+++ hald/linux2/addons/addon-acpi.c	9 Jan 2006 00:38:27 -0000
@@ -5,6 +5,7 @@
  *
  * Copyright (C) 2005 David Zeuthen, <david-o55+BOBDEFg@public.gmane.org>
  * Copyright (C) 2005 Ryan Lortie <desrt-0xnayjDhYQY@public.gmane.org>
+ * Copyright (C) 2006 Richard Hughes <richard-DRAXeJJ+PL1BDgjK7y7TUQ@public.gmane.org>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -26,6 +27,8 @@
 #  include <config.h>
 #endif
 
+#define	ACPI_UINPUT
+
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
@@ -37,10 +40,22 @@
 #include <sys/un.h>
 #include <fcntl.h>
 
+#ifdef ACPI_UINPUT
+#include <linux/input.h>
+#include <linux/uinput.h>
+#endif
+
 #include "libhal/libhal.h"
 
 #include "../probing/shared.h"
 
+#ifdef ACPI_UINPUT
+static int out_fd = -1;
+static int uinput_open (void);
+static int uinput_event (unsigned int key);
+static int uinput_close (void);
+#endif
+
 #ifdef ACPI_PROC
 static FILE *
 acpi_get_event_fp_kernel (void)
@@ -91,6 +106,142 @@
 }
 #endif
 
+#ifdef ACPI_UINPUT
+/** Opens the kernel uinput device
+ *  @return			Success
+ */
+static int
+uinput_open (void)
+{
+	int j, i;
+	struct uinput_user_dev udev_spec = { .name = "Virtual ACPI Keyboard" };
+
+	/* try to open uinput from all the different places it could be */
+	out_fd = open ("/dev/uinput", O_WRONLY | O_NDELAY);
+	if (out_fd < 0)		/* try harder */
+		out_fd = open ("/dev/input/uinput", O_WRONLY | O_NDELAY);
+	if (out_fd < 0)		/* try harder */
+		out_fd = open ("/dev/misc/uinput", O_WRONLY | O_NDELAY);
+	if (out_fd < 0) {	/* try harder */
+		printf ("Cannot open uinput (maybe not root?)\n");
+		return FALSE;
+	}
+
+	/* inform that we'll generate key events */
+	j  = ioctl (out_fd, UI_SET_EVBIT, EV_KEY) < 0;
+	j |= ioctl (out_fd, UI_SET_EVBIT, EV_REP) < 0;
+
+	/* set key events we can generate (in this case, all) */
+	for (i = KEY_RESERVED; i < BTN_MISC; i++)
+		j |= ioctl (out_fd, UI_SET_KEYBIT, i) < 0;
+	if (j) {
+		printf ("error setting up keyboard input device (maybe not root?)\n");
+		return FALSE;
+	}
+	/* write down information for creating a new device */
+	if (write (out_fd, &udev_spec, sizeof udev_spec) != sizeof udev_spec) {
+		printf ("error writing input device spec\n");
+		return FALSE;
+	}
+	/* create, and check */
+	if (ioctl (out_fd, UI_DEV_CREATE, 0) < 0) {
+		printf ("error creating input device\n");
+		return FALSE;
+	}
+	return TRUE;
+}
+#endif
+
+/** Writes to the /dev/uinput device a keypress, i.e. down then up
+ *
+ *  @param	key		The event, e.g KEY_BRIGHTNESSUP
+ *  @return			Success
+ */
+static int
+uinput_event (unsigned int key)
+{
+#ifdef ACPI_UINPUT
+	struct input_event event;
+	event.type = EV_KEY;
+	event.code = key;
+	event.value = 1;
+	if (write (out_fd, &event, sizeof event) != sizeof event) {
+		printf ("error writing down %d/%d/%d to device", EV_KEY, event.value, 1);
+		return FALSE;
+	}
+	event.value = 0;
+	if (write (out_fd, &event, sizeof event) != sizeof event) {
+		printf ("error writing up %d/%d/%d to device", EV_KEY, event.value, 0);
+		return FALSE;
+	}
+	return TRUE;
+#endif
+}
+
+#ifdef ACPI_UINPUT
+/** Closes the /dev/uinput device
+ *
+ *  @return			Success
+ */
+static int
+uinput_close (void)
+{
+	static struct input_event event = {
+		.type = EV_SYN,
+		.code = SYN_REPORT
+	};
+	if (!out_fd)
+		return FALSE;
+	write (out_fd, &event, sizeof event);
+	return TRUE;
+}
+#endif
+
+/** If found, does a uinput event and returns the condition name
+ *
+ *  @param	event		The acpi event
+ *  @return			The ButtonPressed condition
+ */
+static char *
+do_hkey_event (unsigned int event)
+{
+	/* Toshiba FnESC */
+	if (event == 0x101) {
+		uinput_event (KEY_MUTE);
+		return "Mute";
+	}
+	/* Toshiba FnF1 */
+	if (event == 0x13b) {
+		uinput_event (KEY_EXIT); /* need KEY_LOCK */
+		return "Lock";
+	}
+	/* Toshiba FnF2 */
+	if (event == 0x13c) {
+		uinput_event (KEY_SEARCH);
+		return "Search";
+	}
+	/* Toshiba FnF3 */
+	if (event == 0x13d) {
+		uinput_event (KEY_SUSPEND);
+		return "Suspend";
+	}
+	/* Toshiba FnF4 */
+	if (event == 0x13e) {
+		uinput_event (KEY_SLEEP);
+		return "Hibernate";
+	}
+	/* Toshiba FnF6 */
+	if (event == 0x140) {
+		uinput_event (KEY_BRIGHTNESSDOWN);
+		return "BrightnessDown";
+	}
+	/* Toshiba FnF7 */
+	if (event == 0x141) {
+		uinput_event (KEY_BRIGHTNESSUP);
+		return "BrightnessUp";
+	}
+	return NULL;
+}
 
 static void
 main_loop (LibHalContext *ctx, FILE *eventfp)
@@ -127,8 +278,18 @@
 			} else if (strncmp (acpi_path, "battery", sizeof ("battery") - 1) == 0) {
 				dbg ("battery event");
 				libhal_device_rescan (ctx, udi, &error);
+			} else if (strncmp (acpi_path, "hkey", sizeof ("hkey") - 1) == 0) {
+				dbg ("hotkey event; acpi_name=%s, acpi_num1=%i, acpi_num2=%i", acpi_name, acpi_num1, acpi_num2);
+				/* we only do events when the button is pressed, not released */
+				if (acpi_num1 == 1) {
+					char *desc;
+					const char *uinput_udi = "/org/freedesktop/Hal/devices/acpi_uinput";
+					desc = do_hkey_event (acpi_num2);
+					if (desc)
+						libhal_device_emit_condition (ctx, 
+							uinput_udi, "ButtonPressed", desc, &error);
+				}
 			}
-
 		} else {
 			dbg ("cannot parse event");
 		}
@@ -156,6 +317,10 @@
 		return 1;
 	}
 
+#ifdef ACPI_UINPUT
+	uinput_open ();
+#endif
+
 	while (1)
 	{
 #ifdef ACPI_PROC
@@ -178,6 +343,10 @@
 		sleep (5);
 	}
 
+#ifdef ACPI_UINPUT
+	uinput_close ();
+#endif
+
 	return 1;
 }
 

Re: [gpm] Untangling the sleep hotkey mess

[Dmitry Torokhov]

On Sunday 08 January 2006 20:14, Richard Hughes wrote:
> On Sun, 2006-01-08 at 14:13 +0000, Richard Hughes wrote:
> > On Sun, 2006-01-08 at 13:47 +0000, Matthew Garrett wrote:
> > > On Sun, Jan 08, 2006 at 12:58:44PM +0000, Richard Hughes wrote:
> > > 
> > > > Can we not go further and define Dock/UnDock,
> > > > BrightnessUp/BrightnessDown, Hibernate, etc?
> > > 
> > > BrightnessUp/BrightnessDown have keycodes defined in 
> > > /usr/src/linux/input.h, so that shouldn't be a problem. I suggested a 
> > > keycode for hibernate (KEY_SUSPEND, with suspend to RAM on KEY_SLEEP). 
> > 
> > Okay, that's good for me.
> > 
> > > I'm less convinced about Dock/Undock - that's a problem that's so far 
> > > from being solved I've no idea what sort of things userspace wants to 
> > > know :)
> > 
> > Sure, just an example of "other stuff". Lock is maybe a better example
> > as gnome-screensaver (or equiv) can respond to this.
> > 
> > The main problem now is implementation.
> 
> Further to the conversation on IRC, I've attached two ACPI patches to
> provoke discussion.
> 
> The first creates the /org/freedesktop/Hal/devices/acpi_uinput like I
> did for the toshiba specific HAL addon.
> The second uses the acpi events for creating uinput events that can be
> captured using gnome-keybindings, and also creates HAL ButtonPressed
> events so that DBUS aware applications (like g-v-m and g-p-m) can
> monitor the events.
>

I am sorry, but the scheme

	acpi->acpid->hal?->uinput->input_core->keyboard->...>userspace

is way too crazy.

Why don't we create an input handler that would feed certain events
from input layer to acpid via bus_acpi_generate_event(). This will
allow grateful migration of acpi buttons and other stuff to use
input layer:

	acpi->input_core->[new handler]->acpid


In the mean time hal can start using /dev/input/eventX to get those
events.

-- 
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [gpm] Untangling the sleep hotkey mess

[Matthew Garrett]

On Mon, Jan 09, 2006 at 12:07:43AM -0500, Dmitry Torokhov wrote:

> Why don't we create an input handler that would feed certain events
> from input layer to acpid via bus_acpi_generate_event().

I'm not convinced that making the input layer depend on ACPI (even 
optionally) would go down terribly well. It also strikes me as *wrong* - 
the sleep key on this keyboard has nothing to do with ACPI, whereas keys 
presented through ACPI are still an input device.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [gpm] Untangling the sleep hotkey mess

[Richard Hughes]

On Mon, 2006-01-09 at 00:07 -0500, Dmitry Torokhov wrote: 
> On Sunday 08 January 2006 20:14, Richard Hughes wrote:
> > On Sun, 2006-01-08 at 14:13 +0000, Richard Hughes wrote:
> > > On Sun, 2006-01-08 at 13:47 +0000, Matthew Garrett wrote:
> > > > On Sun, Jan 08, 2006 at 12:58:44PM +0000, Richard Hughes wrote:
> > > > 
> > > > > Can we not go further and define Dock/UnDock,
> > > > > BrightnessUp/BrightnessDown, Hibernate, etc?
> > > > 
> > > > BrightnessUp/BrightnessDown have keycodes defined in 
> > > > /usr/src/linux/input.h, so that shouldn't be a problem. I suggested a 
> > > > keycode for hibernate (KEY_SUSPEND, with suspend to RAM on KEY_SLEEP). 
> > > 
> > > Okay, that's good for me.
> > > 
> > > > I'm less convinced about Dock/Undock - that's a problem that's so far 
> > > > from being solved I've no idea what sort of things userspace wants to 
> > > > know :)
> > > 
> > > Sure, just an example of "other stuff". Lock is maybe a better example
> > > as gnome-screensaver (or equiv) can respond to this.
> > > 
> > > The main problem now is implementation.
> > 
> > Further to the conversation on IRC, I've attached two ACPI patches to
> > provoke discussion.
> > 
> > The first creates the /org/freedesktop/Hal/devices/acpi_uinput like I
> > did for the toshiba specific HAL addon.
> > The second uses the acpi events for creating uinput events that can be
> > captured using gnome-keybindings, and also creates HAL ButtonPressed
> > events so that DBUS aware applications (like g-v-m and g-p-m) can
> > monitor the events.
> >
> 
> I am sorry, but the scheme
> 
> 	acpi->acpid->hal?->uinput->input_core->keyboard->...>userspace
> 

HAL does not require acpid to be running as it can listen to the socket
directly. Using HAL allows us to emit a ButtonPressed DBUS event for
session/system applications to listen for (like gnome-volume-manager and
gnome-power-manager) and also could use uinput so that stuff like the
"mute" hotkey can be caught by gnome-keybinding-properties so that the
session action could be taken.

So the call chain would look like this:

acpi->hal->gnome-volume-manager                (ButtonPressed)
acpi->hal->uinput->input_core->keyboard->g-k-p (uinput)

Plus, doing all the processing user-side, we can put the policy in HAL
(saying that hkey 0x140 is KEY_BRIGHTNESSDOWN), meaning we don't have to
update the kernel for every new laptop hotkey layout change.

> Why don't we create an input handler that would feed certain events
> from input layer to acpid via bus_acpi_generate_event(). This will
> allow grateful migration of acpi buttons and other stuff to use
> input layer:
> 
> 	acpi->input_core->[new handler]->acpid

What if the application wants a DBUS event rather than listening on /dev/input/X?

> In the mean time hal can start using /dev/input/eventX to get those
> events.

So in the kernel you want to put all the policy so that if
laptopmodel=foo and bios=bar and key=0x140 then output
KEY_BRIGHTNESSDOWN?

I've no problem using any input interface with HAL, as long as it's
unified between all the different acpi modules.

Even in the acpi kernel bits, there appears to be about half a dozen
different ways that buttons *could* get to userspace, without any
cooperation from the desktop guys and the kernel guys to an optimal
solution.

Sorry if this sounds a bit like a rant.

Richard.

-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [gpm] Untangling the sleep hotkey mess

[Dmitry Torokhov]

On Monday 09 January 2006 00:24, Matthew Garrett wrote:
> On Mon, Jan 09, 2006 at 12:07:43AM -0500, Dmitry Torokhov wrote:
> 
> > Why don't we create an input handler that would feed certain events
> > from input layer to acpid via bus_acpi_generate_event().
> 
> I'm not convinced that making the input layer depend on ACPI (even 
> optionally) would go down terribly well. It also strikes me as *wrong* - 
> the sleep key on this keyboard has nothing to do with ACPI, whereas keys 
> presented through ACPI are still an input device.
> 

The handler is just for compatibility with present installations.
New installations will use the new interface, whatver it will be.
But input layer will be a hub of sorts and I am arguing for ACPI
to be converted to use input layer directly.

-- 
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [gpm] Untangling the sleep hotkey mess

[Richard Hughes]

On Mon, 2006-01-09 at 01:09 -0500, Dmitry Torokhov wrote:
> On Monday 09 January 2006 00:24, Matthew Garrett wrote:
> > On Mon, Jan 09, 2006 at 12:07:43AM -0500, Dmitry Torokhov wrote:
> > 
> > > Why don't we create an input handler that would feed certain events
> > > from input layer to acpid via bus_acpi_generate_event().
> > 
> > I'm not convinced that making the input layer depend on ACPI (even 
> > optionally) would go down terribly well. It also strikes me as *wrong* - 
> > the sleep key on this keyboard has nothing to do with ACPI, whereas keys 
> > presented through ACPI are still an input device.
> > 
> 
> The handler is just for compatibility with present installations.
> New installations will use the new interface, whatver it will be.

Like what?

> But input layer will be a hub of sorts and I am arguing for ACPI
> to be converted to use input layer directly.

What does lkml think of ACPI using the input layer directly? 

Richard

-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [gpm] Untangling the sleep hotkey mess

[Yu, Luming]

 

>-----Original Message-----
>From: Richard Hughes [mailto:hughsient-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org] 
>Sent: 2006年1月9日 9:22
>To: Yu, Luming
>Cc: Matthew Garrett; linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 
>gnome-power-manager-list-rDKQcyrBJuzYtjvyW6yDsg@public.gmane.org; hal-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; 
>desktop_portables-qjLDD68F18O7TbgM5vRIOg@public.gmane.org
>Subject: Re: [gpm] Untangling the sleep hotkey mess
>
>On Mon, 2006-01-09 at 09:10 +0800, Yu Luming wrote:
>> >  3. Use the new acpi kernel hotkeys stuff -- not that I 
>really understand
>> >  the interface, nor how to use it correctly. Plus I'm not 
>sure how much
>> >  of the vendor kernel modules can use this new interface, 
>or if it's
>> >  suitable. It all seemed a bit fragile last time I looked.
>> 
>> Please take a look at 
>> http://bugzilla.kernel.org/show_bug.cgi?id=5749
>> 
>> There are some examples to use hotkey.c for specific laptops 
>with dedicated
>> hotkey acpi device objects , and dedicated AML methods.
>> This patch: http://bugzilla.kernel.org/show_bug.cgi?id=5749#c3
>> can be used to replace sony_acpi.c
>> 
>> This patch: http://bugzilla.kernel.org/show_bug.cgi?id=5749#c4
>> can be used to support brightness control for panasonic laptop.
>> 
>> This patch: http://bugzilla.kernel.org/show_bug.cgi?id=5749#c5
>> can be used to support brighness contrl for ASUS laptop.
>> 
>> Please notes, I will update these patch against len's 
>testing tree, and 
>> ask for inclusion.
>
>Why do we want to use the new hotkey stuff when we can just process all
>the events in userspace using acpi events? The hotkey stuff seems
>overcomplicated and fragile in my opinion.
>
>Sending and receiving :::data:in:some::odd:format seems a 
>little unusual
>to me when we can just listen for the event in one place (like HAL) --
>but then maybe I don't understand all the details. It's not 
>like we will
>have different programs registering for different acpi button events,
>right?

The hotkey.c is NOT like what you said above.
It is used to support dedicated hotkey device and dedicated hotkey AML methods
that are used by ODM to implement hotkey function on their own laptops.
As for hotkey event, hotkey.c doesn't handle it yet.  
And so-called event_num in hotkey.c are mainly used as a key for searching and storing.
 
>
>Richard.
>
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [gpm] Untangling the sleep hotkey mess

[Matthew Garrett]

On Mon, Jan 09, 2006 at 09:37:01AM +0800, Yu, Luming wrote:

> The hotkey.c is NOT like what you said above.
> It is used to support dedicated hotkey device and dedicated hotkey AML methods
> that are used by ODM to implement hotkey function on their own laptops.
> As for hotkey event, hotkey.c doesn't handle it yet.  
> And so-called event_num in hotkey.c are mainly used as a key for searching and storing.

Can we please stop all this madness of trying to support twenty thousand 
weird and wonderful laptop configurations in the kernel, merge dev_acpi 
instead and then do it all in userspace? It strikes me as massively more 
maintainable. That way we don't need vendors to patch kernels every time 
a manufacturer tweaks something - a small userspace update can be pushed 
out instead.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [gpm] Untangling the sleep hotkey mess

[Yu Luming]

On Monday 09 January 2006 09:43, Matthew Garrett wrote:
> On Mon, Jan 09, 2006 at 09:37:01AM +0800, Yu, Luming wrote:
>  > The hotkey.c is NOT like what you said above.
>  > It is used to support dedicated hotkey device and dedicated hotkey AML
>  > methods that are used by ODM to implement hotkey function on their own
>  > laptops. As for hotkey event, hotkey.c doesn't handle it yet.
>  > And so-called event_num in hotkey.c are mainly used as a key for
>  > searching and storing.
>
>  Can we please stop all this madness of trying to support twenty thousand
>  weird and wonderful laptop configurations in the kernel, merge dev_acpi
>  instead and then do it all in userspace? It strikes me as massively more
>  maintainable. That way we don't need vendors to patch kernels every time
>  a manufacturer tweaks something - a small userspace update can be pushed
>  out instead.
>
>  --
>  Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

If you don't like the configure data in kernel source,  it can be moved to 
userspace. 

Re: dev_acpi:
The key point is AML code shouldn't be exposed in userspace. It is too ugly.

At this point, I admit hotkey.c is ugly too in the current stage.
But, it is because there are NOT standard defined  in ACPI spec, or ODMs just
ignore the Spec (for example, ACPI  Video Extension). 

-- 
Thanks,
Luming
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [gpm] Untangling the sleep hotkey mess

[Matthew Garrett]

On Mon, Jan 09, 2006 at 10:19:00AM +0800, Yu Luming wrote:

> Re: dev_acpi:
> The key point is AML code shouldn't be exposed in userspace. It is too ugly.

It may be ugly, but it /works/. We can't predict what vendors will do in 
terms of providing hotkey support in future, and exposing a limited 
interface to userspace would probably just result in it having to be 
extended later on, which again forces us into kernel patching.

With my Ubuntu hat on:

dev_acpi would make life a lot easier for us than the current solutions. 
A single userspace application has the advantage that it can be 
maintained in a nice cross-distribution way, and we can rapidly fold in 
extra support based on testing reports.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [gpm] Untangling the sleep hotkey mess

[Yu Luming]

On Monday 09 January 2006 10:30, Matthew Garrett wrote:
> On Mon, Jan 09, 2006 at 10:19:00AM +0800, Yu Luming wrote:
>  > Re: dev_acpi:
>  > The key point is AML code shouldn't be exposed in userspace. It is too
>  > ugly.
>
>  It may be ugly, but it /works/. We can't predict what vendors will do in
>  terms of providing hotkey support in future, and exposing a limited
>  interface to userspace would probably just result in it having to be
>  extended later on, which again forces us into kernel patching.
They should come up with some kinds of specs for hotkey in the future.
Otherwise, the problem won't go away.

>
>  With my Ubuntu hat on:
>
>  dev_acpi would make life a lot easier for us than the current solutions.
>  A single userspace application has the advantage that it can be
>  maintained in a nice cross-distribution way, and we can rapidly fold in
>  extra support based on testing reports.
Comparing with generic hotkey solution, dev_acpi solution cannot prevent any 
trouble in terms of supportability, and manageability.

For dev_acpi,  you won't need kernel patch. But you need to know everything in 
AML world from user space.

For hotkey.c, there is an auto-load-call-back function that will automatically 
detect and install configure data based on well-known acpi device object's 
PNP ID, and well-known hotkey AML method names.
And the manual configure interface is intended to be used as debug tool.

So, if you use hotkey.c you don't need to know anything in AML world from user 
space, if you are lucky. I expect in the future, the well-know acpi device 
PNP ID, and well-know AML methods names would become part of ACPI spec.

PS.  According to my testing, windows do have platform specific hotkey 
drivers.

>
>  --
>  Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

-- 
Thanks,
Luming
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [gpm] Untangling the sleep hotkey mess

[Matthew Garrett]

On Mon, Jan 09, 2006 at 11:13:15AM +0800, Yu Luming wrote:

> Comparing with generic hotkey solution, dev_acpi solution cannot prevent any 
> trouble in terms of supportability, and manageability.
> 
> For dev_acpi,  you won't need kernel patch. But you need to know everything in 
> AML world from user space.

We need to know that in any case. The difference is that in the kernel 
case, adding support for new hotkeys requires upgrading the kernel. In 
the case of it adding a new button type that hasn't been seen before, it 
also requires upgrading userspace. If we do it all in userspace, we only 
have one application to fix up in most cases.

> PS.  According to my testing, windows do have platform specific hotkey 
> drivers.

In the Windows world, vendors can provide customised distributions on a 
per-laptop basis. That's not practical in the Linux world.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [gpm] Untangling the sleep hotkey mess

[Yu Luming]

On Monday 09 January 2006 11:27, Matthew Garrett wrote:
> On Mon, Jan 09, 2006 at 11:13:15AM +0800, Yu Luming wrote:
>  > Comparing with generic hotkey solution, dev_acpi solution cannot prevent
>  > any trouble in terms of supportability, and manageability.
>  >
>  > For dev_acpi,  you won't need kernel patch. But you need to know
>  > everything in AML world from user space.
>
>  We need to know that in any case. The difference is that in the kernel
>  case, adding support for new hotkeys requires upgrading the kernel. In
From practical point of view,  the acpi hotkey won't change for a quite 
long period. For example, I cannot find too much changes on acpi hotkey from
Thinkpad T21 and Thinkpad T42.  And, I don't see any reason for ODM to 
change their well-know ACPI device PNP ID and well-know AML methods names for
acpi hotkey on new platfrom, because they can just implement any platform 
changes in AML code. 
 
>  the case of it adding a new button type that hasn't been seen before, it
>  also requires upgrading userspace. If we do it all in userspace, we only
>  have one application to fix up in most cases.

>
>  > PS.  According to my testing, windows do have platform specific hotkey
>  > drivers.
>
>  In the Windows world, vendors can provide customised distributions on a
>  per-laptop basis. That's not practical in the Linux world.
My points is that if hotkey.c become sucessful, then linux won't need those 
platform specific hotkey drivers for common hotkeys  such as brightness 
control, volume control, and output switch.. 
Does it make sense?

Thanks,
Luming
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [gpm] Untangling the sleep hotkey mess

[Matthew Garrett]

On Mon, Jan 09, 2006 at 11:55:24AM +0800, Yu Luming wrote:

> From practical point of view,  the acpi hotkey won't change for a quite 
> long period. For example, I cannot find too much changes on acpi hotkey from
> Thinkpad T21 and Thinkpad T42.  And, I don't see any reason for ODM to 
> change their well-know ACPI device PNP ID and well-know AML methods names for
> acpi hotkey on new platfrom, because they can just implement any platform 
> changes in AML code. 

Toshiba have entirely changed their ACPI hotkey setup with the new Tecra 
range. The BIOS bears almost no resemblance to the previous one. Sony 
have changed their brightness handling code in newer machines - it seems 
to be impossible to change it through ACPI now.

Rule 1 of laptops: Vendors will do unpredictable and hard to understand 
things. They don't consider what they're providing to be well-known. As 
far as vendors are concerned, it's an internal implementation detail and 
they'll change it whenever they feel like it.

If Intel would like a single driver able to support all future laptops, 
then Intel need to specify hotkey behaviour in the ACPI spec and refuse 
to allow people to claim ACPI compliance unless they implement it. Until 
that happens there's no way of claiming that this stuff will just carry 
on working.

> >  In the Windows world, vendors can provide customised distributions on a
> >  per-laptop basis. That's not practical in the Linux world.
> My points is that if hotkey.c become sucessful, then linux won't need those 
> platform specific hotkey drivers for common hotkeys  such as brightness 
> control, volume control, and output switch.. 

It *will*, it's just they'll all be in a single driver that special 
cases a bunch of manufacturers. The fact that all of this is in one file 
rather than 20 doesn't make it inherently better.
-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [gpm] Untangling the sleep hotkey mess

[Richard Hughes]

On Mon, 2006-01-09 at 02:30 +0000, Matthew Garrett wrote:
> On Mon, Jan 09, 2006 at 10:19:00AM +0800, Yu Luming wrote:
> 
> > Re: dev_acpi:
> > The key point is AML code shouldn't be exposed in userspace. It is too ugly.
> 
> It may be ugly, but it /works/. We can't predict what vendors will do in 
> terms of providing hotkey support in future, and exposing a limited 
> interface to userspace would probably just result in it having to be 
> extended later on, which again forces us into kernel patching.
> 
> With my Ubuntu hat on:
> 
> dev_acpi would make life a lot easier for us than the current solutions. 
> A single userspace application has the advantage that it can be 
> maintained in a nice cross-distribution way, and we can rapidly fold in 
> extra support based on testing reports.

With my Redhat on*:

I think maybe dev_acpi may work well for the power-user case (if you
know *exactly* what you are doing), but just using the simple acpi
events (like we can do now on some machines) works well. This means that
we can get bug reports from users for obscure machines and keyboards
just from looking at an acpid log. Adding support for different keys can
be done just by adding values to an FDI xml file, or a small addon.
I think trying to pre-process the information in the kernel is a bad
thing to do -- debugging, and getting a user to test a kernel patch is
much harder than a similar userspace patch.

In my ideal world:

* All ACPI keyboard events would come through /proc/acpi/event as hkey's
* You could change the brightness on *any* lcdpanel by doing a simple
  #echo 3 > /proc/acpi/video/brightness
* You could get the number of brightness levels supported doing
  #cat /proc/acpi/video/brightness_levels

Richard.

* I don't work for Redhat, but I do lots of work with Fedora and HAL.

-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [gpm] Untangling the sleep hotkey mess

[Dmitry Torokhov]

On 1/9/06, Richard Hughes <hughsient@gmail.com> wrote:
>
> * All ACPI keyboard events would come through /proc/acpi/event as hkey's
> * You could change the brightness on *any* lcdpanel by doing a simple
>  #echo 3 > /proc/acpi/video/brightness
> * You could get the number of brightness levels supported doing
>  #cat /proc/acpi/video/brightness_levels
>

And what do you do if:

1. You don't have ACPI available (other platforms)
2. APM works better for some reason
3. The hotkey (sleep, power, etc) does not come from ACPI?

--
Dmitry

Re: [gpm] Untangling the sleep hotkey mess

[Matthew Garrett]

On Mon, Jan 09, 2006 at 04:14:40PM -0500, Dmitry Torokhov wrote:
> And what do you do if:
> 
> 1. You don't have ACPI available (other platforms)
> 2. APM works better for some reason
> 3. The hotkey (sleep, power, etc) does not come from ACPI?

With most APM machines, pressing the hotkey causes the BIOS to send a 
system suspend event. There's no real way for the OS to tell what's 
caused this (it could be low battery, a closed lid or a sleep key press) 
and so the correct thing to do is to tell userspace that a suspend is 
about to happen and then do it.

But yes, PMU sleep requests are (last time I checked) generally managed 
by keycode 142 being generated and userspace dealing with it. There's no 
equivalent to /proc/acpi/events. This is why I think standardising on 
the input layer is a more sensible approach than trying to turn input 
events into acpi ones.

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

Re: [gpm] Untangling the sleep hotkey mess

[Dmitry Torokhov]

On 1/9/06, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Mon, Jan 09, 2006 at 04:14:40PM -0500, Dmitry Torokhov wrote:
> > And what do you do if:
> >
> > 1. You don't have ACPI available (other platforms)
> > 2. APM works better for some reason
> > 3. The hotkey (sleep, power, etc) does not come from ACPI?
>
> With most APM machines, pressing the hotkey causes the BIOS to send a
> system suspend event. There's no real way for the OS to tell what's
> caused this (it could be low battery, a closed lid or a sleep key press)
> and so the correct thing to do is to tell userspace that a suspend is
> about to happen and then do it.
>
> But yes, PMU sleep requests are (last time I checked) generally managed
> by keycode 142 being generated and userspace dealing with it. There's no
> equivalent to /proc/acpi/events. This is why I think standardising on
> the input layer is a more sensible approach than trying to turn input
> events into acpi ones.
>

I probably was not clear in my original message. The new input handler
for ACPI would be only used for compatibility with cirrent
installations relying on ACPID. This will allow gradual conversion of
ACPI drivers to use input layer instead of sending events dircetly to
/proc/acpi/events.

OTOH there should probably be anotehr generic power interface
filtering out power-related input events from the stream of all input
events in the system and making them available to userspace without
requiring applications (be it individual applications or HAL) to open
zillion of raw /dev/input/evenX devices.

--
Dmitry

Re: [gpm] Untangling the sleep hotkey mess

[Matthew Garrett]

On Mon, Jan 09, 2006 at 04:52:11PM -0500, Dmitry Torokhov wrote:

> I probably was not clear in my original message. The new input handler
> for ACPI would be only used for compatibility with cirrent
> installations relying on ACPID. This will allow gradual conversion of
> ACPI drivers to use input layer instead of sending events dircetly to
> /proc/acpi/events.

Ah, I see what you mean. However, there's still some slight awkwardness 
here, since userspace will be expecting the events to come from the 
button driver (the regexp used is generally button[ /]sleep), and 
that'll be registered even if there's no sleep button (the lid switch 
and power button are still handled via it). I don't believe you can 
register two drivers with the same name, so without gross hacks I can't 
see an easy way to provide this functionality through /proc/acpi without 
requiring userspace to be fixed up /anyway/...

> OTOH there should probably be anotehr generic power interface
> filtering out power-related input events from the stream of all input
> events in the system and making them available to userspace without
> requiring applications (be it individual applications or HAL) to open
> zillion of raw /dev/input/evenX devices.

Sure. As mentioned before, we basically just want keycodes 116, 142 and 
205 (POWER, SLEEP and SUSPEND) - I guess that's not too difficult?
-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [gpm] Untangling the sleep hotkey mess

[Brown, Len]

 
>> But input layer will be a hub of sorts and I am arguing for ACPI
>> to be converted to use input layer directly.
>
>What does lkml think of ACPI using the input layer directly? 

I think it is a good idea.

The only question I have is how to transition.
If I replace the acpi_bus_generate_event() calls for
power/sleep/lid/hotkeys
and replace them with input_report_key(), will there be something up
there
listening for these events when acpid does not get them?

thanks,
-Len

ps. the long term goal is to delete /proc/acpi, including
/proc/acpi/event.
Getting rid of the events that are like keys is a step in that
direction,
but there are a bunch of other steps for non key-like events too.

Re: [gpm] Untangling the sleep hotkey mess

[Matthew Garrett]

On Wed, Jul 26, 2006 at 07:41:26PM -0400, Brown, Len wrote:

> The only question I have is how to transition.
> If I replace the acpi_bus_generate_event() calls for
> power/sleep/lid/hotkeys
> and replace them with input_report_key(), will there be something up
> there
> listening for these events when acpid does not get them?

Right now, hal will get those events and transmit them to further 
applications. I don't think there's a drop in acpid replacement yet, but 
gnome-power-manager (for instance) will currently react accordingly.

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

Re: [gpm] Untangling the sleep hotkey mess

[Dmitry Torokhov]

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

On Wednesday 26 July 2006 19:41, Brown, Len wrote:
> 
> >> But input layer will be a hub of sorts and I am arguing for ACPI
> >> to be converted to use input layer directly.
> >
> >What does lkml think of ACPI using the input layer directly? 
> 
> I think it is a good idea.
> 
> The only question I have is how to transition.
> If I replace the acpi_bus_generate_event() calls for
> power/sleep/lid/hotkeys
> and replace them with input_report_key(), will there be something up
> there
> listening for these events when acpid does not get them?
> 

Let's start with adding reporting through input layer while still
reporintg through /proc/acpi/event, this will allow gradual transition.

What do you think about the patch below (should be applied on top of
cleanup patch which is attached)? I will need to adjust it to
!CONFIG_INPUT, but it can be done later if we agree on principle.
 
-- 
Dmitry

Subject: ACPI: button - register with input layer

ACPI: button - register with input layer

In addition to signalling button/lid events through /proc/acpi/event
create separate input devices and report KEY_POWER, KEY_SLEEP and
SW_LID through input layer.

My sleep button autorepeat but userspace will have to filter duplicate
sleep requests anyway (and discard unprocessed events right after
wakeup).

Unlike /proc/acpi/event interface input device corresponding to LID
switch reports true lid state instead of just a counter. SW_LID is
active when lid is closed.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/acpi/button.c |  172 ++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 125 insertions(+), 47 deletions(-)

Index: work/drivers/acpi/button.c
===================================================================
--- work.orig/drivers/acpi/button.c
+++ work/drivers/acpi/button.c
@@ -29,6 +29,7 @@
 #include <linux/types.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
+#include <linux/input.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
 
@@ -83,7 +84,9 @@ static struct acpi_driver acpi_button_dr
 
 struct acpi_button {
 	struct acpi_device *device;	/* Fixed button kludge */
-	u8 type;
+	unsigned int type;
+	struct input_dev *input;
+	char phys[32];			/* for input device */
 	unsigned long pushed;
 };
 
@@ -245,12 +248,35 @@ static int acpi_button_remove_fs(struct 
 static void acpi_button_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct acpi_button *button = data;
+	struct input_dev *input;
 
 	if (!button || !button->device)
 		return;
 
 	switch (event) {
 	case ACPI_BUTTON_NOTIFY_STATUS:
+		input = button->input;
+
+		if (button->type == ACPI_BUTTON_TYPE_LID) {
+			struct acpi_handle *handle = button->device->handle;
+			unsigned long state;
+
+			if (ACPI_FAILURE(acpi_evaluate_integer(handle, "_LID",
+								NULL, &state)))
+				state = 1;	/* assume open */
+
+			input_report_switch(input, SW_LID, !state);
+
+		} else {
+			int keycode = test_bit(KEY_SLEEP, input->keybit) ?
+						KEY_SLEEP : KEY_POWER;
+
+			input_report_key(input, keycode, 1);
+			input_sync(input);
+			input_report_key(input, keycode, 0);
+		}
+		input_sync(input);
+
 		acpi_bus_generate_event(button->device, event,
 					++button->pushed);
 		break;
@@ -275,11 +301,58 @@ static acpi_status acpi_button_notify_fi
 	return AE_OK;
 }
 
-static int acpi_button_add(struct acpi_device *device)
+static int acpi_button_install_notify_handlers(struct acpi_button *button)
 {
-	int result;
 	acpi_status status;
+
+	switch (button->type) {
+	case ACPI_BUTTON_TYPE_POWERF:
+		status =
+		    acpi_install_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
+						     acpi_button_notify_fixed,
+						     button);
+		break;
+	case ACPI_BUTTON_TYPE_SLEEPF:
+		status =
+		    acpi_install_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
+						     acpi_button_notify_fixed,
+						     button);
+		break;
+	default:
+		status = acpi_install_notify_handler(button->device->handle,
+						     ACPI_DEVICE_NOTIFY,
+						     acpi_button_notify,
+						     button);
+		break;
+	}
+
+	return ACPI_FAILURE(status) ? -ENODEV : 0;
+}
+
+static void acpi_button_remove_notify_handlers(struct acpi_button *button)
+{
+	switch (button->type) {
+	case ACPI_BUTTON_TYPE_POWERF:
+		acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
+						acpi_button_notify_fixed);
+		break;
+	case ACPI_BUTTON_TYPE_SLEEPF:
+		acpi_remove_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
+						acpi_button_notify_fixed);
+		break;
+	default:
+		acpi_remove_notify_handler(button->device->handle,
+					   ACPI_DEVICE_NOTIFY,
+					   acpi_button_notify);
+		break;
+	}
+}
+
+static int acpi_button_add(struct acpi_device *device)
+{
+	int error;
 	struct acpi_button *button;
+	struct input_dev *input;
 
 	if (!device)
 		return -EINVAL;
@@ -291,6 +364,12 @@ static int acpi_button_add(struct acpi_d
 	button->device = device;
 	acpi_driver_data(device) = button;
 
+	button->input = input = input_allocate_device();
+	if (!input) {
+		error = -ENOMEM;
+		goto err_free_button;
+	}
+
 	/*
 	 * Determine the button type (via hid), as fixed-feature buttons
 	 * need to be handled a bit differently than generic-space.
@@ -325,39 +404,48 @@ static int acpi_button_add(struct acpi_d
 	} else {
 		printk(KERN_ERR PREFIX "Unsupported hid [%s]\n",
 			    acpi_device_hid(device));
-		result = -ENODEV;
-		goto end;
+		error = -ENODEV;
+		goto err_free_input;
 	}
 
-	result = acpi_button_add_fs(device);
-	if (result)
-		goto end;
+	error = acpi_button_add_fs(device);
+	if (error)
+		goto err_free_input;
+
+	error = acpi_button_install_notify_handlers(button);
+	if (error)
+		goto err_remove_fs;
+
+	snprintf(button->phys, sizeof(button->phys),
+		 "%s/button/input0", acpi_device_hid(device));
+
+	input->name = acpi_device_name(device);
+	input->phys = button->phys;
+	input->id.bustype = BUS_HOST;
+	input->id.product = button->type;
 
 	switch (button->type) {
+	case ACPI_BUTTON_TYPE_POWER:
 	case ACPI_BUTTON_TYPE_POWERF:
-		status =
-		    acpi_install_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
-						     acpi_button_notify_fixed,
-						     button);
+		input->evbit[0] = BIT(EV_KEY);
+		set_bit(KEY_POWER, input->keybit);
 		break;
+
+	case ACPI_BUTTON_TYPE_SLEEP:
 	case ACPI_BUTTON_TYPE_SLEEPF:
-		status =
-		    acpi_install_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
-						     acpi_button_notify_fixed,
-						     button);
+		input->evbit[0] = BIT(EV_KEY);
+		set_bit(KEY_SLEEP, input->keybit);
 		break;
-	default:
-		status = acpi_install_notify_handler(device->handle,
-						     ACPI_DEVICE_NOTIFY,
-						     acpi_button_notify,
-						     button);
+
+	case ACPI_BUTTON_TYPE_LID:
+		input->evbit[0] = BIT(EV_SW);
+		set_bit(SW_LID, input->swbit);
 		break;
 	}
 
-	if (ACPI_FAILURE(status)) {
-		result = -ENODEV;
-		goto end;
-	}
+	error = input_register_device(input);
+	if (error)
+		goto err_remove_handlers;
 
 	if (device->wakeup.flags.valid) {
 		/* Button's GPE is run-wake GPE */
@@ -372,13 +460,17 @@ static int acpi_button_add(struct acpi_d
 	printk(KERN_INFO PREFIX "%s [%s]\n",
 	       acpi_device_name(device), acpi_device_bid(device));
 
-      end:
-	if (result) {
-		acpi_button_remove_fs(device);
-		kfree(button);
-	}
+	return 0;
 
-	return result;
+ err_remove_handlers:
+	acpi_button_remove_notify_handlers(button);
+ err_remove_fs:
+	acpi_button_remove_fs(device);
+ err_free_input:
+	input_free_device(input);
+ err_free_button:
+	kfree(button);
+	return error;
 }
 
 static int acpi_button_remove(struct acpi_device *device, int type)
@@ -390,23 +482,9 @@ static int acpi_button_remove(struct acp
 
 	button = acpi_driver_data(device);
 
-	/* Unregister for device notifications. */
-	switch (button->type) {
-	case ACPI_BUTTON_TYPE_POWERF:
-		acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
-						acpi_button_notify_fixed);
-		break;
-	case ACPI_BUTTON_TYPE_SLEEPF:
-		acpi_remove_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
-						acpi_button_notify_fixed);
-		break;
-	default:
-		acpi_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
-					   acpi_button_notify);
-		break;
-	}
-
+	acpi_button_remove_notify_handlers(button);
 	acpi_button_remove_fs(device);
+	input_unregister_device(button->input);
 	kfree(button);
 
 	return 0;

[-- Attachment #2: acpi-button-cleanup.patch --]
[-- Type: text/x-diff, Size: 5676 bytes --]

Subject: ACPI: button - general cleanup

ACPI: button - general cleanup

Remove unnecessary casts and initializations, clean up formatting.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/acpi/button.c |   69 ++++++++++++++++----------------------------------
 1 files changed, 23 insertions(+), 46 deletions(-)

Index: work/drivers/acpi/button.c
===================================================================
--- work.orig/drivers/acpi/button.c
+++ work/drivers/acpi/button.c
@@ -62,7 +62,7 @@
 #define _COMPONENT		ACPI_BUTTON_COMPONENT
 ACPI_MODULE_NAME("acpi_button")
 
-    MODULE_AUTHOR("Paul Diefenbaugh");
+MODULE_AUTHOR("Paul Diefenbaugh");
 MODULE_DESCRIPTION(ACPI_BUTTON_DRIVER_NAME);
 MODULE_LICENSE("GPL");
 
@@ -78,7 +78,7 @@ static struct acpi_driver acpi_button_dr
 	.ops = {
 		.add = acpi_button_add,
 		.remove = acpi_button_remove,
-		},
+	},
 };
 
 struct acpi_button {
@@ -109,8 +109,7 @@ static struct proc_dir_entry *acpi_butto
 
 static int acpi_button_info_seq_show(struct seq_file *seq, void *offset)
 {
-	struct acpi_button *button = (struct acpi_button *)seq->private;
-
+	struct acpi_button *button = seq->private;
 
 	if (!button || !button->device)
 		return 0;
@@ -128,22 +127,17 @@ static int acpi_button_info_open_fs(stru
 
 static int acpi_button_state_seq_show(struct seq_file *seq, void *offset)
 {
-	struct acpi_button *button = (struct acpi_button *)seq->private;
+	struct acpi_button *button = seq->private;
 	acpi_status status;
 	unsigned long state;
 
-
 	if (!button || !button->device)
 		return 0;
 
 	status = acpi_evaluate_integer(button->device->handle, "_LID", NULL, &state);
-	if (ACPI_FAILURE(status)) {
-		seq_printf(seq, "state:      unsupported\n");
-	} else {
-		seq_printf(seq, "state:      %s\n",
-			   (state ? "open" : "closed"));
-	}
-
+	seq_printf(seq, "state:      %s\n",
+		   ACPI_FAILURE(status) ? "unsupported" :
+			(state ? "open" : "closed"));
 	return 0;
 }
 
@@ -159,8 +153,7 @@ static struct proc_dir_entry *acpi_lid_d
 static int acpi_button_add_fs(struct acpi_device *device)
 {
 	struct proc_dir_entry *entry = NULL;
-	struct acpi_button *button = NULL;
-
+	struct acpi_button *button;
 
 	if (!device || !acpi_driver_data(device))
 		return -EINVAL;
@@ -228,10 +221,8 @@ static int acpi_button_add_fs(struct acp
 
 static int acpi_button_remove_fs(struct acpi_device *device)
 {
-	struct acpi_button *button = NULL;
-
+	struct acpi_button *button = acpi_driver_data(device);
 
-	button = acpi_driver_data(device);
 	if (acpi_device_dir(device)) {
 		if (button->type == ACPI_BUTTON_TYPE_LID)
 			remove_proc_entry(ACPI_BUTTON_FILE_STATE,
@@ -253,8 +244,7 @@ static int acpi_button_remove_fs(struct 
 
 static void acpi_button_notify(acpi_handle handle, u32 event, void *data)
 {
-	struct acpi_button *button = (struct acpi_button *)data;
-
+	struct acpi_button *button = data;
 
 	if (!button || !button->device)
 		return;
@@ -275,8 +265,7 @@ static void acpi_button_notify(acpi_hand
 
 static acpi_status acpi_button_notify_fixed(void *data)
 {
-	struct acpi_button *button = (struct acpi_button *)data;
-
+	struct acpi_button *button = data;
 
 	if (!button)
 		return AE_BAD_PARAMETER;
@@ -288,18 +277,16 @@ static acpi_status acpi_button_notify_fi
 
 static int acpi_button_add(struct acpi_device *device)
 {
-	int result = 0;
-	acpi_status status = AE_OK;
-	struct acpi_button *button = NULL;
-
+	int result;
+	acpi_status status;
+	struct acpi_button *button;
 
 	if (!device)
 		return -EINVAL;
 
-	button = kmalloc(sizeof(struct acpi_button), GFP_KERNEL);
+	button = kzalloc(sizeof(struct acpi_button), GFP_KERNEL);
 	if (!button)
 		return -ENOMEM;
-	memset(button, 0, sizeof(struct acpi_button));
 
 	button->device = device;
 	acpi_driver_data(device) = button;
@@ -396,9 +383,7 @@ static int acpi_button_add(struct acpi_d
 
 static int acpi_button_remove(struct acpi_device *device, int type)
 {
-	acpi_status status = 0;
-	struct acpi_button *button = NULL;
-
+	struct acpi_button *button;
 
 	if (!device || !acpi_driver_data(device))
 		return -EINVAL;
@@ -408,24 +393,20 @@ static int acpi_button_remove(struct acp
 	/* Unregister for device notifications. */
 	switch (button->type) {
 	case ACPI_BUTTON_TYPE_POWERF:
-		status =
-		    acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
-						    acpi_button_notify_fixed);
+		acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
+						acpi_button_notify_fixed);
 		break;
 	case ACPI_BUTTON_TYPE_SLEEPF:
-		status =
-		    acpi_remove_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
-						    acpi_button_notify_fixed);
+		acpi_remove_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
+						acpi_button_notify_fixed);
 		break;
 	default:
-		status = acpi_remove_notify_handler(device->handle,
-						    ACPI_DEVICE_NOTIFY,
-						    acpi_button_notify);
+		acpi_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY,
+					   acpi_button_notify);
 		break;
 	}
 
 	acpi_button_remove_fs(device);
-
 	kfree(button);
 
 	return 0;
@@ -433,8 +414,7 @@ static int acpi_button_remove(struct acp
 
 static int __init acpi_button_init(void)
 {
-	int result = 0;
-
+	int result;
 
 	acpi_button_dir = proc_mkdir(ACPI_BUTTON_CLASS, acpi_root_dir);
 	if (!acpi_button_dir)
@@ -451,7 +431,6 @@ static int __init acpi_button_init(void)
 
 static void __exit acpi_button_exit(void)
 {
-
 	acpi_bus_unregister_driver(&acpi_button_driver);
 
 	if (acpi_power_dir)
@@ -461,8 +440,6 @@ static void __exit acpi_button_exit(void
 	if (acpi_lid_dir)
 		remove_proc_entry(ACPI_BUTTON_SUBCLASS_LID, acpi_button_dir);
 	remove_proc_entry(ACPI_BUTTON_CLASS, acpi_root_dir);
-
-	return;
 }
 
 module_init(acpi_button_init);

RE: [gpm] Untangling the sleep hotkey mess

[Brown, Len]

>> >> But input layer will be a hub of sorts and I am arguing for ACPI
>> >> to be converted to use input layer directly.
>> >
>> >What does lkml think of ACPI using the input layer directly? 
>> 
>> I think it is a good idea.
>> 
>> The only question I have is how to transition.
>> If I replace the acpi_bus_generate_event() calls for
>> power/sleep/lid/hotkeys
>> and replace them with input_report_key(), will there be something up
>> there
>> listening for these events when acpid does not get them?
>> 
>
>Let's start with adding reporting through input layer while still
>reporintg through /proc/acpi/event, this will allow gradual transition.
>
>What do you think about the patch below (should be applied on top of
>cleanup patch which is attached)? I will need to adjust it to
>!CONFIG_INPUT, but it can be done later if we agree on principle.

sorry for the delayed response -- looks like this one arrived at the
tail end of the ottawa trip...

I agree with this patch in principle.
it would be good to cut over to handing the power/sleep/lid buttons
as input device sooner rather than later.  Hopefully we don't get
all confused with double reporting of the events and can do one
or the other in user-space.

I don't understand some parts of the diff, including why the LID
event is always assumed to be an open event, and some of the diff
seemed to be moving code around and I wasn't clear on why.

thanks,
-Len

Re: [gpm] Untangling the sleep hotkey mess

[Dmitry Torokhov]

On Wednesday 30 August 2006 21:49, Brown, Len wrote:
> >> >> But input layer will be a hub of sorts and I am arguing for ACPI
> >> >> to be converted to use input layer directly.
> >> >
> >> >What does lkml think of ACPI using the input layer directly? 
> >> 
> >> I think it is a good idea.
> >> 
> >> The only question I have is how to transition.
> >> If I replace the acpi_bus_generate_event() calls for
> >> power/sleep/lid/hotkeys
> >> and replace them with input_report_key(), will there be something up
> >> there
> >> listening for these events when acpid does not get them?
> >> 
> >
> >Let's start with adding reporting through input layer while still
> >reporintg through /proc/acpi/event, this will allow gradual transition.
> >
> >What do you think about the patch below (should be applied on top of
> >cleanup patch which is attached)? I will need to adjust it to
> >!CONFIG_INPUT, but it can be done later if we agree on principle.
> 
> sorry for the delayed response -- looks like this one arrived at the
> tail end of the ottawa trip...
> 
> I agree with this patch in principle.
> it would be good to cut over to handing the power/sleep/lid buttons
> as input device sooner rather than later.  Hopefully we don't get
> all confused with double reporting of the events and can do one
> or the other in user-space.
> 
> I don't understand some parts of the diff, including why the LID
> event is always assumed to be an open event,

It is only assumed to be open if we failed to evaluate "_LID" property,
otherwise true lid state is reported. Since I expect some sleep scripts
to be associated with lid close event reporting it as open looks safe.
Or we could not report anything in case of failure, that's probably
the best solution.

> and some of the diff 
> seemed to be moving code around and I wasn't clear on why.

That was done to simplify error handling paths.

So... do you want me to make it work with !CONFIG_INPUT or shoudl I just
add a dependency on INPUT in Kconfig for button driver?

-- 
Dmitry

Re: [gpm] Untangling the sleep hotkey mess

[Len Brown]

On Wednesday 30 August 2006 22:53, Dmitry Torokhov wrote:

> So... do you want me to make it work with !CONFIG_INPUT or shoudl I just
> add a dependency on INPUT in Kconfig for button driver?

If virtually everybody uses INPUT, then I'm fine with the ACPI button driver simply depending
on CONFIG_INPUT.

If nobody uses ACPI on a system without INPUT, then I don't see why we should
muck up the code to suport a case that nobody will test or use.

thanks,
-Len

Re: [gpm] Untangling the sleep hotkey mess

[Dmitry Torokhov]

On Wednesday 30 August 2006 23:03, Len Brown wrote:
> On Wednesday 30 August 2006 22:53, Dmitry Torokhov wrote:
> 
> > So... do you want me to make it work with !CONFIG_INPUT or shoudl I just
> > add a dependency on INPUT in Kconfig for button driver?
> 
> If virtually everybody uses INPUT, then I'm fine with the ACPI button driver simply depending
> on CONFIG_INPUT.
> 
> If nobody uses ACPI on a system without INPUT, then I don't see why we should
> muck up the code to suport a case that nobody will test or use.
> 

INPUT is not used on s390 but I suppose that's ok ;) You need to work
really hard to disable input (embedded, no VTs or anything, etc).

-- 
Dmitry

Re: [gpm] Untangling the sleep hotkey mess

[Richard Hughes]

On Wed, 2006-08-30 at 23:07 -0400, Dmitry Torokhov wrote:
> On Wednesday 30 August 2006 23:03, Len Brown wrote:
> > On Wednesday 30 August 2006 22:53, Dmitry Torokhov wrote:
> > 
> > > So... do you want me to make it work with !CONFIG_INPUT or shoudl I just
> > > add a dependency on INPUT in Kconfig for button driver?
> > 
> > If virtually everybody uses INPUT, then I'm fine with the ACPI button driver simply depending
> > on CONFIG_INPUT.
> > 
> > If nobody uses ACPI on a system without INPUT, then I don't see why we should
> > muck up the code to suport a case that nobody will test or use.
> > 
> 
> INPUT is not used on s390 but I suppose that's ok ;) You need to work
> really hard to disable input (embedded, no VTs or anything, etc).

Is there any feedback on this solution? Also, what's the timescale for
conversion to the input layer - are we talking blue sky or a few months?
I want to make sure userspace is in a position for this change.

Thanks for all your help,

Richard.

Re: [gpm] Untangling the sleep hotkey mess

[Dmitry Torokhov]

On 9/18/06, Richard Hughes <hughsient-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, 2006-08-30 at 23:07 -0400, Dmitry Torokhov wrote:
> > On Wednesday 30 August 2006 23:03, Len Brown wrote:
> > > On Wednesday 30 August 2006 22:53, Dmitry Torokhov wrote:
> > >
> > > > So... do you want me to make it work with !CONFIG_INPUT or shoudl I just
> > > > add a dependency on INPUT in Kconfig for button driver?
> > >
> > > If virtually everybody uses INPUT, then I'm fine with the ACPI button driver simply depending
> > > on CONFIG_INPUT.
> > >
> > > If nobody uses ACPI on a system without INPUT, then I don't see why we should
> > > muck up the code to suport a case that nobody will test or use.
> > >
> >
> > INPUT is not used on s390 but I suppose that's ok ;) You need to work
> > really hard to disable input (embedded, no VTs or anything, etc).
>
> Is there any feedback on this solution? Also, what's the timescale for
> conversion to the input layer - are we talking blue sky or a few months?
> I want to make sure userspace is in a position for this change.
>

I sent the updated patch to Yu Luming, the copy is in the list archives:

http://marc.theaimsgroup.com/?l=linux-acpi&m=115751976924048&w=2

so unless there are objections to the thing I'd like to have it
applied to ACPI tree.

-- 
Dmitry

Re: [gpm] Untangling the sleep hotkey mess

[Richard Hughes]

On Mon, 2006-09-18 at 11:17 -0400, Dmitry Torokhov wrote:
> On 9/18/06, Richard Hughes <hughsient@gmail.com> wrote:
> > On Wed, 2006-08-30 at 23:07 -0400, Dmitry Torokhov wrote:
> > > On Wednesday 30 August 2006 23:03, Len Brown wrote:
> > > > On Wednesday 30 August 2006 22:53, Dmitry Torokhov wrote:
> > > >
> > > > > So... do you want me to make it work with !CONFIG_INPUT or shoudl I just
> > > > > add a dependency on INPUT in Kconfig for button driver?
> > > >
> > > > If virtually everybody uses INPUT, then I'm fine with the ACPI button driver simply depending
> > > > on CONFIG_INPUT.
> > > >
> > > > If nobody uses ACPI on a system without INPUT, then I don't see why we should
> > > > muck up the code to suport a case that nobody will test or use.
> > > >
> > >
> > > INPUT is not used on s390 but I suppose that's ok ;) You need to work
> > > really hard to disable input (embedded, no VTs or anything, etc).
> >
> > Is there any feedback on this solution? Also, what's the timescale for
> > conversion to the input layer - are we talking blue sky or a few months?
> > I want to make sure userspace is in a position for this change.
> >
> 
> I sent the updated patch to Yu Luming, the copy is in the list archives:
> 
> http://marc.theaimsgroup.com/?l=linux-acpi&m=115751976924048&w=2
> 
> so unless there are objections to the thing I'd like to have it
> applied to ACPI tree.

Brilliant, thanks Dmitry.

Richard.