[PATCH] Map volume and brightness events on thinkpads

[PATCH] Map volume and brightness events on thinkpads

[Jeremy Katz]

There are standard keycodes for brightness and volume; map the events to
emit them so that things work properly

Signed-off-by: Jeremy Katz <katzj@redhat.com>
---
 drivers/misc/thinkpad_acpi.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index 6c0b2f0..64ae4b4 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -945,15 +945,15 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 		KEY_UNKNOWN,	/* 0x0C: FN+BACKSPACE */
 		KEY_UNKNOWN,	/* 0x0D: FN+INSERT */
 		KEY_UNKNOWN,	/* 0x0E: FN+DELETE */
-		KEY_RESERVED,	/* 0x0F: FN+HOME (brightness up) */
+		KEY_BRIGHTNESSUP,	/* 0x0F: FN+HOME (brightness up) */
 		/* Scan codes 0x10 to 0x1F: Extended ACPI HKEY hot keys */
-		KEY_RESERVED,	/* 0x10: FN+END (brightness down) */
+		KEY_BRIGHTNESSDOWN,	/* 0x10: FN+END (brightness down) */
 		KEY_RESERVED,	/* 0x11: FN+PGUP (thinklight toggle) */
 		KEY_UNKNOWN,	/* 0x12: FN+PGDOWN */
 		KEY_ZOOM,	/* 0x13: FN+SPACE (zoom) */
-		KEY_RESERVED,	/* 0x14: VOLUME UP */
-		KEY_RESERVED,	/* 0x15: VOLUME DOWN */
-		KEY_RESERVED,	/* 0x16: MUTE */
+		KEY_VOLUMEUP,	/* 0x14: VOLUME UP */
+		KEY_VOLUMEDOWN,	/* 0x15: VOLUME DOWN */
+		KEY_MUTE,	/* 0x16: MUTE */
 		KEY_VENDOR,	/* 0x17: Thinkpad/AccessIBM/Lenovo */
 		/* (assignments unknown, please report if found) */
 		KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN,
@@ -974,9 +974,9 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 		KEY_RESERVED,	/* 0x11: FN+PGUP (thinklight toggle) */
 		KEY_UNKNOWN,	/* 0x12: FN+PGDOWN */
 		KEY_ZOOM,	/* 0x13: FN+SPACE (zoom) */
-		KEY_RESERVED,	/* 0x14: VOLUME UP */
-		KEY_RESERVED,	/* 0x15: VOLUME DOWN */
-		KEY_RESERVED,	/* 0x16: MUTE */
+		KEY_VOLUMEUP,	/* 0x14: VOLUME UP */
+		KEY_VOLUMEDOWN,	/* 0x15: VOLUME DOWN */
+		KEY_MUTE,	/* 0x16: MUTE */
 		KEY_VENDOR,	/* 0x17: Thinkpad/AccessIBM/Lenovo */
 		/* (assignments unknown, please report if found) */
 		KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN,
-- 
1.5.3.4

Re: [PATCH] Map volume and brightness events on thinkpads

[Henrique de Moraes Holschuh]

On Mon, 15 Oct 2007, Jeremy Katz wrote:
> There are standard keycodes for brightness and volume; map the events to
> emit them so that things work properly

NAK.  It is the completely wrong thing to do for IBM thinkpads which process
volume and brightness completely in firmware.

And the input subsystem maintainer has made it extremely clear in various
threads that the input devices are *not* to be used as a notification
service for on-screen-display or other such stuff.  If you send volume and
brightness *key* events to userspace, it is supposed to act on them and
raise/lower brightness/volume, which is the wrong thing to do on thinkpads.
Never mind that HAL is ignoring the input maintainer's directions and
violating this.

We should fix the backlight class to be more useful and support poll() or
somesuch, for userspace to track the backlight level in a resource-friendly
way for OSD (the only sane thing to do on an IBM thinkpad with such events).
And an ALSA mixer to provide a proper path to the thinkpad-acpi volume
functionality is also in my schedule for 2.6.25.

As for Lenovo thinkpads, brightness control is to be processed by the ACPI
video module, so brightness hot keys are not to be reported by default there
either.  I am not so sure about the volume keys, but your patch touches the
IBM keymap *and* you provide no testing information for the various Lenovo
models, so I have to NAK it as well until more information is available.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] Map volume and brightness events on thinkpads

[Jeremy Katz]

On Mon, 2007-10-15 at 19:07 -0200, Henrique de Moraes Holschuh wrote:
> On Mon, 15 Oct 2007, Jeremy Katz wrote:
> > There are standard keycodes for brightness and volume; map the events to
> > emit them so that things work properly
> 
> NAK.  It is the completely wrong thing to do for IBM thinkpads which process
> volume and brightness completely in firmware.
> 
> And the input subsystem maintainer has made it extremely clear in various
> threads that the input devices are *not* to be used as a notification
> service for on-screen-display or other such stuff.  If you send volume and
> brightness *key* events to userspace, it is supposed to act on them and
> raise/lower brightness/volume, which is the wrong thing to do on thinkpads.
> Never mind that HAL is ignoring the input maintainer's directions and
> violating this.

It seemed to be doing the right thing here on an X31 (had to give it
back to its owner; will retrieve and test again tomorrow to make sure
everything is matching up).

> We should fix the backlight class to be more useful and support poll() or
> somesuch, for userspace to track the backlight level in a resource-friendly
> way for OSD (the only sane thing to do on an IBM thinkpad with such events).
> And an ALSA mixer to provide a proper path to the thinkpad-acpi volume
> functionality is also in my schedule for 2.6.25.

I don't know that having another mixer is really the right answer.  You
want to have the buttons/hardware volume matching (and showing)
on-screen changes to the system volume.  Otherwise, things are just
confusing to a user.

> As for Lenovo thinkpads, brightness control is to be processed by the ACPI
> video module, so brightness hot keys are not to be reported by default there
> either.  

Brightness events are being reported as is with the X60t I've got...

> I am not so sure about the volume keys, but your patch touches the
> IBM keymap *and* you provide no testing information for the various Lenovo
> models, so I have to NAK it as well until more information is available.

Having the volume keys reported makes it so that we get the on-screen
display of the volume changing and /proc/acpi/ibm/volume matches what
the displayed volume shows.

As far as other Lenovos, if you want it tested out on more of them,
that's easy enough to do as they're very common in the office here.  But
the couple of people that I've given the patched module to haven't had
problems (various T60s)

Jeremy

Re: [PATCH] Map volume and brightness events on thinkpads

[Henrique de Moraes Holschuh]

On Mon, 15 Oct 2007, Jeremy Katz wrote:
> On Mon, 2007-10-15 at 19:07 -0200, Henrique de Moraes Holschuh wrote:
> > On Mon, 15 Oct 2007, Jeremy Katz wrote:
> > > There are standard keycodes for brightness and volume; map the events to
> > > emit them so that things work properly
> > 
> > NAK.  It is the completely wrong thing to do for IBM thinkpads which process
> > volume and brightness completely in firmware.
> > 
> > And the input subsystem maintainer has made it extremely clear in various
> > threads that the input devices are *not* to be used as a notification
> > service for on-screen-display or other such stuff.  If you send volume and
> > brightness *key* events to userspace, it is supposed to act on them and
> > raise/lower brightness/volume, which is the wrong thing to do on thinkpads.
> > Never mind that HAL is ignoring the input maintainer's directions and
> > violating this.
> 
> It seemed to be doing the right thing here on an X31 (had to give it
> back to its owner; will retrieve and test again tomorrow to make sure
> everything is matching up).

Look *very* closedly at what HAL is doing on that X31.  If it seems to do
the "right thing" while reporting keys to userspace over the input layer, it
is because HAL is abusing the input layer and using input events in "passive
mode", just to report brightness and volume changes through OSD or somesuch.
Either that, or you have such an old userpace there, that it doesn't react
to input events at all...

I am not exactly against the notion of passive input events (i.e. stuff you
are *not* to act upon other than to give user feedback), but you cannot
deliver them as EV_KEY (you cannot tell those apart from proper "please do
change the brightness/volume/whatever" EV_KEY), and the input subsystem is
not to be used like that according to its maintainer.

So, I want no such behaviour in thinkpad-acpi, please.  There are other ways
to adress the needs of userspace, and they are being implemented (although
slowly, I am sorry about this, but my bandwidth is limited).

> > We should fix the backlight class to be more useful and support poll() or
> > somesuch, for userspace to track the backlight level in a resource-friendly
> > way for OSD (the only sane thing to do on an IBM thinkpad with such events).
> > And an ALSA mixer to provide a proper path to the thinkpad-acpi volume
> > functionality is also in my schedule for 2.6.25.
> 
> I don't know that having another mixer is really the right answer.  You

It is on IBM thinkpads, were you *really* have a second hardware mixer, that
operates completely independent of the OS and controls the output signal
level for the speakers and headphone (and does NOT control the output signal
level on the audio output on docks and port replicators, etc).

The volume and mute buttons act directly on that mixer (through the
firmware), and thinkpad-acpi can control that mixer fully.  A mixer is a
mixer, and the proper interface to talk to a mixer is an ALSA mixer
interface.

The fact that an IBM thinkpad also lets you know exactly which mixer control
was pressed *on some models* (T4x and newer, X31 and newer, as long as the
latest BIOS is used) does not mean you are to use that to poke on the AC97
mixer controls.   Heavy hint: Windows does NOT touch the AC97 mixer.
Another heavy hint: you need to track the mixer state and filter the events
to reproduce the mixer behaviour, where mute always mutes (not mute/unmute)
and you unmute with a press of one of the volume up/down buttons (which
won't change volume level when unmuting).

And on Lenovo thinkpads, the same level of control is available.  The
difference is that I am not sure if the firmware is always acting on the
mixer automatically on all models and BIOS revisions.  I will ask Lenovo
about it.   This still asks for a mixer interface to control the internal
speaker/headphone mixer, but it *might* ask for input events for volume
up/down/mute in *some* Lenovo models.  Again, this is not something the
patch in question does correctly.

> want to have the buttons/hardware volume matching (and showing)
> on-screen changes to the system volume.  Otherwise, things are just
> confusing to a user.

Any thinkpad user that looks at that keyboard and thinks that pressing its
keys is to do anything else than control the volume of the built-in speakers
and headphone output, is already confused.  Blame IBM, if you must... but
that's how things are.  Lenovo may change it... or not.  If they do, I will
adapt the driver to do the right thing: that's why the driver *can* report
volume input events.  So that I can enable it by default should it become
the right thing to do on a given thinkpad model.

And really, you will *not* have me agree that pressing a volume key should
act on both mixers at once, ever.  This reduces by half the dynamic range of
the volume control on a part of the curve (the AC97 mixer has more levels
than the internal mixer).

OSDs for volume should be tied to the ALSA mixers.  One will be provided by
thinkpad-acpi soon.  Heck, I can get it ready for 2.6.24 if you convince
Linus and Len to take it in after -rc1.  Otherwise, it is scheduled for
2.6.25.

> > As for Lenovo thinkpads, brightness control is to be processed by the ACPI
> > video module, so brightness hot keys are not to be reported by default there
> > either.  
> 
> Brightness events are being reported as is with the X60t I've got...

They are reported through ACPI events and not by thinkpad-acpi.  Remove the
drivers and watch the ACPI messages.

> > I am not so sure about the volume keys, but your patch touches the
> > IBM keymap *and* you provide no testing information for the various Lenovo
> > models, so I have to NAK it as well until more information is available.
> 
> Having the volume keys reported makes it so that we get the on-screen
> display of the volume changing and /proc/acpi/ibm/volume matches what
> the displayed volume shows.
>
> As far as other Lenovos, if you want it tested out on more of them,
> that's easy enough to do as they're very common in the office here.  But
> the couple of people that I've given the patched module to haven't had
> problems (various T60s)

Empirically testing things without really taking into account the path the
information will travel will not get you the right answers for this problem.
I hope I explained why that happens clearly enough in this mail...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] Map volume and brightness events on thinkpads

[Arjan van de Ven]

On Mon, 15 Oct 2007 19:07:37 -0200
Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote:

> On Mon, 15 Oct 2007, Jeremy Katz wrote:
> > There are standard keycodes for brightness and volume; map the
> > events to emit them so that things work properly
> 
> NAK.  It is the completely wrong thing to do for IBM thinkpads which
> process volume and brightness completely in firmware.
> 
> And the input subsystem maintainer has made it extremely clear in
> various threads that the input devices are *not* to be used as a
> notification service for on-screen-display or other such stuff.  If
> you send volume and brightness *key* events to userspace, it is
> supposed to act on them and raise/lower brightness/volume, which is
> the wrong thing to do on thinkpads. Never mind that HAL is ignoring
> the input maintainer's directions and violating this.
> 
> We should fix the backlight class to be more useful and support
> poll() or somesuch, for userspace to track the backlight level in a
> resource-friendly way for OSD (the only sane thing to do on an IBM
> thinkpad with such events). And an ALSA mixer to provide a proper
> path to the thinkpad-acpi volume functionality is also in my schedule
> for 2.6.25.
> 

there is a huge problem with this though: Depending on your graphics
chip, it's the graphics driver, not the bios, that does the backlight
control......  in fact the trend in hardware (this is what windows
does) is have the OS take over the control of all of this as soon as
possible...

Linux should do this with software control as well for that reason; the
other approach just isn't going to be possible in the long term ;(

Re: [PATCH] Map volume and brightness events on thinkpads

[Jesse Barnes]

On Monday, October 15, 2007 2:07 pm Henrique de Moraes Holschuh wrote:
> We should fix the backlight class to be more useful and support
> poll() or somesuch, for userspace to track the backlight level in a
> resource-friendly way for OSD (the only sane thing to do on an IBM
> thinkpad with such events). And an ALSA mixer to provide a proper
> path to the thinkpad-acpi volume functionality is also in my schedule
> for 2.6.25.
>
> As for Lenovo thinkpads, brightness control is to be processed by the
> ACPI video module, so brightness hot keys are not to be reported by
> default there either.  I am not so sure about the volume keys, but
> your patch touches the IBM keymap *and* you provide no testing
> information for the various Lenovo models, so I have to NAK it as
> well until more information is available.

No, on Lenovo (and in general actually) the firmware should *not* touch 
the backlight.  Otherwise if another driver touches it the driver and 
firmware will be out of sync, causing unexpected and undesirable 
behavior.  We intend to fix this for the Intel driver at least 
(requiring both ACPI video driver and gfx driver updates), others will 
probably follow eventually.

Jesse

Re: [PATCH] Map volume and brightness events on thinkpads

[Henrique de Moraes Holschuh]

On Mon, 15 Oct 2007, Jesse Barnes wrote:
> On Monday, October 15, 2007 2:07 pm Henrique de Moraes Holschuh wrote:
> > As for Lenovo thinkpads, brightness control is to be processed by the
> > ACPI video module, so brightness hot keys are not to be reported by
> > default there either.  I am not so sure about the volume keys, but
> > your patch touches the IBM keymap *and* you provide no testing
> > information for the various Lenovo models, so I have to NAK it as
> > well until more information is available.
> 
> No, on Lenovo (and in general actually) the firmware should *not* touch 
> the backlight.  Otherwise if another driver touches it the driver and 

This is not an option on IBM ThinkPads, unless you patch the DSDT on
non-ancient ACPI-based models, and unless you patch the BIOS (and maybe even
the EC control program) itself on the really ancient models.  It is that
simple.

On *new* Lenovo ThinkPad BIOSes (the z60 that Lenovo has given up on act
just like IBM ThinkPads), the firmware reports brightness changes through
the proper ACPI messages, which are processed by the ACPI video module. So I
will repeat myself here: brightness control is to be processed by the ACPI
video module, so brightness hot keys are not to be reported by default in
Lenovo thinkpads.

You want ACPI video to just pass the messages to userspace when X.org is
driving the backlight?  Fine with me.  That *still* doesn't make it right to
get these messages as hot key presses over the input layer through the
thinkpad-acpi driver.  So the NAK stands.  Any changes should be done to the
ACPI video driver in this case.

If the user has any real need to get these messages from thinkpad-acpi for
whichever particular reasons, he can use the standard interface to input
event drivers to remap the scan codes to whatever he wants.

> firmware will be out of sync, causing unexpected and undesirable 
> behavior.  We intend to fix this for the Intel driver at least 
> (requiring both ACPI video driver and gfx driver updates), others will 
> probably follow eventually.

Good luck, and please interface it properly to the backlight class while
at it.  There's no reason to make the waters mudier :-)

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] Map volume and brightness events on thinkpads

[Jesse Barnes]

On Tuesday, October 16, 2007 1:36 am Henrique de Moraes Holschuh wrote:
> > No, on Lenovo (and in general actually) the firmware should *not*
> > touch the backlight.  Otherwise if another driver touches it the
> > driver and
>
> This is not an option on IBM ThinkPads, unless you patch the DSDT on
> non-ancient ACPI-based models, and unless you patch the BIOS (and
> maybe even the EC control program) itself on the really ancient
> models.  It is that simple.
>

Yeah, unfortunately, "should not" doesn't mean "does not" in this case.  
In cases where we can't control the firmware, we have to use it to 
control the backlight exclusively, or we'll definitely get into 
trouble.

> You want ACPI video to just pass the messages to userspace when X.org
> is driving the backlight?  Fine with me.  That *still* doesn't make
> it right to get these messages as hot key presses over the input
> layer through the thinkpad-acpi driver.  So the NAK stands.  Any
> changes should be done to the ACPI video driver in this case.

So is this really the direction that input is going?  Last time I talked 
with Dmitry, he seemed ok with adding input events for ACPI and other 
firmware hotkeys...

> Good luck, and please interface it properly to the backlight class
> while at it.  There's no reason to make the waters mudier :-)

Yeah, the backlight class needs some changes:  the backlight dirs should 
somehow identify the display they control and only *one* backlight 
driver should be registered for a given display.  Unfortunately we 
don't have enough info in the kernel to identify displays, so those 
changes may have to wait until the DRM grows such knowledge.

Jesse

Re: [PATCH] Map volume and brightness events on thinkpads

[Henrique de Moraes Holschuh]

On Tue, 16 Oct 2007, Jesse Barnes wrote:
> On Tuesday, October 16, 2007 1:36 am Henrique de Moraes Holschuh wrote:
> > You want ACPI video to just pass the messages to userspace when X.org
> > is driving the backlight?  Fine with me.  That *still* doesn't make
> > it right to get these messages as hot key presses over the input
> > layer through the thinkpad-acpi driver.  So the NAK stands.  Any
> > changes should be done to the ACPI video driver in this case.
> 
> So is this really the direction that input is going?  Last time I talked 
> with Dmitry, he seemed ok with adding input events for ACPI and other 
> firmware hotkeys...

Last time the issue was brought up (and I do believe it was because of
thinkpad-acpi :-) ), he made it clear that any events you are to act upon
are fine in input, but events that are just notifications (i.e. the firmware
already did the action) are not.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] Map volume and brightness events on thinkpads

[Jesse Barnes]

On Tuesday, October 16, 2007 2:18 am Henrique de Moraes Holschuh wrote:
> On Tue, 16 Oct 2007, Jesse Barnes wrote:
> > On Tuesday, October 16, 2007 1:36 am Henrique de Moraes Holschuh 
wrote:
> > > You want ACPI video to just pass the messages to userspace when
> > > X.org is driving the backlight?  Fine with me.  That *still*
> > > doesn't make it right to get these messages as hot key presses
> > > over the input layer through the thinkpad-acpi driver.  So the
> > > NAK stands.  Any changes should be done to the ACPI video driver
> > > in this case.
> >
> > So is this really the direction that input is going?  Last time I
> > talked with Dmitry, he seemed ok with adding input events for ACPI
> > and other firmware hotkeys...
>
> Last time the issue was brought up (and I do believe it was because
> of thinkpad-acpi :-) ), he made it clear that any events you are to
> act upon are fine in input, but events that are just notifications
> (i.e. the firmware already did the action) are not.

Ah yeah, I agree with that.  Regular events should be uevents or 
something, not input events.  Actual keyboard keys though (whether they 
generate firmware event messages or actual scancodes) should probably 
go through the input layer.  I thought that's what Jeremy's patch was 
doing, maybe I didn't look closely enough.

Jesse

Re: [PATCH] Map volume and brightness events on thinkpads

[Henrique de Moraes Holschuh]

On Tue, 16 Oct 2007, Jesse Barnes wrote:
> > Last time the issue was brought up (and I do believe it was because
> > of thinkpad-acpi :-) ), he made it clear that any events you are to
> > act upon are fine in input, but events that are just notifications
> > (i.e. the firmware already did the action) are not.
> 
> Ah yeah, I agree with that.  Regular events should be uevents or 
> something, not input events.  Actual keyboard keys though (whether they 
> generate firmware event messages or actual scancodes) should probably 
> go through the input layer.  I thought that's what Jeremy's patch was 
> doing, maybe I didn't look closely enough.

The patch adds keycodes for "keys" that are acually notifications on many
thinkpads.  And that is the problem, please refer to the rest of the thread
for more details...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] Map volume and brightness events on thinkpads

[Jesse Barnes]

On Tuesday, October 16, 2007 11:25 pm Henrique de Moraes Holschuh wrote:
> On Tue, 16 Oct 2007, Jesse Barnes wrote:
> > > Last time the issue was brought up (and I do believe it was
> > > because of thinkpad-acpi :-) ), he made it clear that any events
> > > you are to act upon are fine in input, but events that are just
> > > notifications (i.e. the firmware already did the action) are not.
> >
> > Ah yeah, I agree with that.  Regular events should be uevents or
> > something, not input events.  Actual keyboard keys though (whether
> > they generate firmware event messages or actual scancodes) should
> > probably go through the input layer.  I thought that's what
> > Jeremy's patch was doing, maybe I didn't look closely enough.
>
> The patch adds keycodes for "keys" that are acually notifications on
> many thinkpads.  And that is the problem, please refer to the rest of
> the thread for more details...

Yeah, just read through it.  It really doesn't seem like it matters that 
much whether "keypress+notify" events are delivered via the input layer 
or via some sort of uevent.

However, since we already have userspace code in place handling the 
input layer case, why not just go with it?  It's fairly straightforward 
and already works in some distros.

Jesse

Re: [PATCH] Map volume and brightness events on thinkpads

[Henrique de Moraes Holschuh]

On Tue, 16 Oct 2007, Jesse Barnes wrote:
> However, since we already have userspace code in place handling the 
> input layer case, why not just go with it?  It's fairly straightforward 
> and already works in some distros.

That is answered in the thread, as well...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] Map volume and brightness events on thinkpads

[Matthew Garrett]

On Mon, Oct 15, 2007 at 07:07:37PM -0200, Henrique de Moraes Holschuh wrote:

> And the input subsystem maintainer has made it extremely clear in various
> threads that the input devices are *not* to be used as a notification
> service for on-screen-display or other such stuff.  If you send volume and
> brightness *key* events to userspace, it is supposed to act on them and
> raise/lower brightness/volume, which is the wrong thing to do on thinkpads.
> Never mind that HAL is ignoring the input maintainer's directions and
> violating this.

Reality disagrees. There are already several cases where notifications 
are sent via the keyboard controller, such as the wireless and touchpad 
disable keys on my HP. There are Dells that do the same for brightness 
keys. Unless you want to make the argument that sending keyboard 
controller events through the input layer is the wrong thing to do, it's 
impossible to standardise on a setup where we never see notifications 
through it.

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

Re: [PATCH] Map volume and brightness events on thinkpads

[Henrique de Moraes Holschuh]

On Tue, 16 Oct 2007, Matthew Garrett wrote:
> On Mon, Oct 15, 2007 at 07:07:37PM -0200, Henrique de Moraes Holschuh wrote:
> > And the input subsystem maintainer has made it extremely clear in various
> > threads that the input devices are *not* to be used as a notification
> > service for on-screen-display or other such stuff.  If you send volume and
> > brightness *key* events to userspace, it is supposed to act on them and
> > raise/lower brightness/volume, which is the wrong thing to do on thinkpads.
> > Never mind that HAL is ignoring the input maintainer's directions and
> > violating this.
> 
> Reality disagrees. There are already several cases where notifications 

Yes, it does.  But none of it cannot be fixed, HAL is the really big
offender in userspace, and laptop drivers are the really big offenders in
kernel space.

> are sent via the keyboard controller, such as the wireless and touchpad 
> disable keys on my HP. There are Dells that do the same for brightness 

It is not clear to me if they are notifications or not.  Does the firmware
act on the keys by itself? If it does, then they are notifications.  If it
does not, then they are regular hot keys and there is no controversy whether
they belong on the input layer or not (they do).

> keys. Unless you want to make the argument that sending keyboard 
> controller events through the input layer is the wrong thing to do, it's 
> impossible to standardise on a setup where we never see notifications 
> through it.

Well, I better make it clear once again. Please excuse me the very direct
and acid tone in the rest of this email, I want to make certain points
crystal clear to everyone (and not just you, I do believe you are already
very aware of my position in this):

1. I am not against sending notification events through input (but see point
two below).  However, AFAIK, Dmitry is.  He said as much in the last thread
about it.  I have added him to the CC list, he can make his position clear
in this thread, if I got it wrong.

Until I get a "you can do it" from Dmitry, forget about thinkpad-acpi
sending such events over the input layer.  I believe in a coherent kernel
where the schizophrenia on the use of the various interfaces and subsystems
is kept to a bare minimum.  If a subsystem maintainers tell me not to do
something, I won't do it.

2. I am against sending notification events through input **that look
exactly the same as regular events**.  That is not a wise design choice IMO,
it is a very dirty hack.

I did propose ways to fix that properly, though (and to write the patches to
do so).  Two simple and clean ways come to mind:

	1. Add a new EV_* class for such events.
	2. Alternatively, add a flags field to the higher bits of EV_*
	   class that could be used to mark events in a proper way.

And I am sure there are other ways too, so it *can* be done properly if it
is to be done at all.  However, it was not accepted because Dmitry does not
want notifications going over the input subsystem as far as I recall from
that thread.  Get Dmitry to accept a way to send notifications though the
input layer, and I will follow it.

3. We have a backlight class, a LED class, a rf-kill class and ALSA mixers.
Is there a real reason to pester Dmitry about the issue, if we can use these
alternate paths (that are indeed more generic and more suited for the job)?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] Map volume and brightness events on thinkpads

[Matthew Garrett]

On Tue, Oct 16, 2007 at 12:11:53PM -0200, Henrique de Moraes Holschuh wrote:
> On Tue, 16 Oct 2007, Matthew Garrett wrote:
> > are sent via the keyboard controller, such as the wireless and touchpad 
> > disable keys on my HP. There are Dells that do the same for brightness 
> 
> It is not clear to me if they are notifications or not.  Does the firmware
> act on the keys by itself? If it does, then they are notifications.  If it
> does not, then they are regular hot keys and there is no controversy whether
> they belong on the input layer or not (they do).

Yes, the firmware acts upon it.

> 2. I am against sending notification events through input **that look
> exactly the same as regular events**.  That is not a wise design choice IMO,
> it is a very dirty hack.

Userspace is going to have to deal with this case anyway. Some vendors 
simply don't let us distinguish.

> 3. We have a backlight class, a LED class, a rf-kill class and ALSA mixers.
> Is there a real reason to pester Dmitry about the issue, if we can use these
> alternate paths (that are indeed more generic and more suited for the job)?

It's not always going to be possible to tie notifications into a class 
device - in the Dell case, for instance, interacting with the backlight 
requires you to use a 200Kb library so has to be done in userspace.

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

Re: [PATCH] Map volume and brightness events on thinkpads

[Henrique de Moraes Holschuh]

On Tue, 16 Oct 2007, Matthew Garrett wrote:
> On Tue, Oct 16, 2007 at 12:11:53PM -0200, Henrique de Moraes Holschuh wrote:
> > On Tue, 16 Oct 2007, Matthew Garrett wrote:
> > > are sent via the keyboard controller, such as the wireless and touchpad 
> > > disable keys on my HP. There are Dells that do the same for brightness 
> > 
> > It is not clear to me if they are notifications or not.  Does the firmware
> > act on the keys by itself? If it does, then they are notifications.  If it
> > does not, then they are regular hot keys and there is no controversy whether
> > they belong on the input layer or not (they do).
> 
> Yes, the firmware acts upon it.

So, I'd have to say it doesn't belong on the input layer AFAIK.

As I said, I am not against using the input layer for this, but currently
AFAIK it is not to be used for it.

> > 2. I am against sending notification events through input **that look
> > exactly the same as regular events**.  That is not a wise design choice IMO,
> > it is a very dirty hack.
> 
> Userspace is going to have to deal with this case anyway. Some vendors 
> simply don't let us distinguish.

But many do, and the less mud in the water, the better.

> > 3. We have a backlight class, a LED class, a rf-kill class and ALSA mixers.
> > Is there a real reason to pester Dmitry about the issue, if we can use these
> > alternate paths (that are indeed more generic and more suited for the job)?
> 
> It's not always going to be possible to tie notifications into a class 
> device - in the Dell case, for instance, interacting with the backlight 
> requires you to use a 200Kb library so has to be done in userspace.

Perhaps.  But for that Dell case, there is already a proper notification
system in place as well: ACPI backlight events.  You might have to get
userspace to tell the ACPI video driver that it will handle the events
(something X.org already needs, anyway), but that's about it.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] Map volume and brightness events on thinkpads

[Matthew Garrett]

On Tue, Oct 16, 2007 at 12:31:24PM -0200, Henrique de Moraes Holschuh wrote:
> On Tue, 16 Oct 2007, Matthew Garrett wrote:
> > Yes, the firmware acts upon it.
> 
> So, I'd have to say it doesn't belong on the input layer AFAIK.

They're keys that generate scancodes through the keyboard controller. 
Not having them go through the input layer requires actively forcing 
them into some other route, which seems excessive.

> > Userspace is going to have to deal with this case anyway. Some vendors 
> > simply don't let us distinguish.
> 
> But many do, and the less mud in the water, the better.

There's no advantage in having to implement multiple solutions. If we're 
stuck with one of them, we might as well just use it for the other cases 
as well.

> > It's not always going to be possible to tie notifications into a class 
> > device - in the Dell case, for instance, interacting with the backlight 
> > requires you to use a 200Kb library so has to be done in userspace.
> 
> Perhaps.  But for that Dell case, there is already a proper notification
> system in place as well: ACPI backlight events.  You might have to get
> userspace to tell the ACPI video driver that it will handle the events
> (something X.org already needs, anyway), but that's about it.

No, these aren't ACPI keys and the machines don't provide backlight 
control through ACPI. It's not a problem with more recent Dells, but 
machines before mid-2006 or so have this issue.

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

Re: [PATCH] Map volume and brightness events on thinkpads

[Henrique de Moraes Holschuh]

On Tue, 16 Oct 2007, Matthew Garrett wrote:
> On Tue, Oct 16, 2007 at 12:31:24PM -0200, Henrique de Moraes Holschuh wrote:
> > On Tue, 16 Oct 2007, Matthew Garrett wrote:
> > > Yes, the firmware acts upon it.
> > 
> > So, I'd have to say it doesn't belong on the input layer AFAIK.

> They're keys that generate scancodes through the keyboard controller. 

The fact is that we were not used to anything using the keyboard controller
as a message-passing device, but nowadays that is (unfortunately) happening.

It still doesn't mean it belongs inside the stream of data for the keyboard,
maskerading as a key press.

> Not having them go through the input layer requires actively forcing 
> them into some other route, which seems excessive.

I don't think it is excessive at all, actually.  But I am known to take the
longer and harder road if I think the end result will be neater and more
organized.

> > > Userspace is going to have to deal with this case anyway. Some vendors 
> > > simply don't let us distinguish.
> > 
> > But many do, and the less mud in the water, the better.
> 
> There's no advantage in having to implement multiple solutions. If we're 
> stuck with one of them, we might as well just use it for the other cases 
> as well.

IMO we might as well go to the clean road, deploy a generic interface that
CAN do it properly (might as well be input, but Dmitry has some good reasons
not to want it there), and move to it.

> > > It's not always going to be possible to tie notifications into a class 
> > > device - in the Dell case, for instance, interacting with the backlight 
> > > requires you to use a 200Kb library so has to be done in userspace.
> > 
> > Perhaps.  But for that Dell case, there is already a proper notification
> > system in place as well: ACPI backlight events.  You might have to get
> > userspace to tell the ACPI video driver that it will handle the events
> > (something X.org already needs, anyway), but that's about it.
> 
> No, these aren't ACPI keys and the machines don't provide backlight 
> control through ACPI. It's not a problem with more recent Dells, but 
> machines before mid-2006 or so have this issue.

Am I the only one seeing the irony on that comment over the idea of sending
such events through the input layer?  ACPI is not just a way to talk to AML
crap in the BIOS.  Is a (somewhat obtuse, very limited) generic interface to
model the hardware.  But for backlight, it certianly is powerful enough.

If you want an even more generic interface, I am fine with it too (just make
sure to move ACPI over to that interface as well).

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] Map volume and brightness events on thinkpads

[Matthew Garrett]

On Tue, Oct 16, 2007 at 02:56:23PM -0200, Henrique de Moraes Holschuh wrote:
> On Tue, 16 Oct 2007, Matthew Garrett wrote:
> > They're keys that generate scancodes through the keyboard controller. 
> 
> The fact is that we were not used to anything using the keyboard controller
> as a message-passing device, but nowadays that is (unfortunately) happening.
>
> It still doesn't mean it belongs inside the stream of data for the keyboard,
> maskerading as a key press.

But it *is* a key press!
 
> > There's no advantage in having to implement multiple solutions. If we're 
> > stuck with one of them, we might as well just use it for the other cases 
> > as well.
> 
> IMO we might as well go to the clean road, deploy a generic interface that
> CAN do it properly (might as well be input, but Dmitry has some good reasons
> not to want it there), and move to it.

I think Dmitry is wrong here. Input is the right layer for sending 
keypress information to userland.

> > No, these aren't ACPI keys and the machines don't provide backlight 
> > control through ACPI. It's not a problem with more recent Dells, but 
> > machines before mid-2006 or so have this issue.
> 
> Am I the only one seeing the irony on that comment over the idea of sending
> such events through the input layer?  ACPI is not just a way to talk to AML
> crap in the BIOS.  Is a (somewhat obtuse, very limited) generic interface to
> model the hardware.  But for backlight, it certianly is powerful enough.

Well, yes, we could have a layer in the kernel to turn the key events 
into ACPI events and then let the video module turn them back into input 
events, but that still wouldn't deal with the fact that legacy Dell 
backlight control isn't going to happen in the kernel.

Anyway. My point was that saying we shouldn't put notification events 
through the input layer is at odds with reality - we already do, and 
they already arrive with EV_KEY. Userspace copes. Who benefits from 
doing it in any other way? Userspace doesn't - we'd need to rewrite 
parts of it to deal with the new setup. The kernel doesn't, because it 
doesn't consume these events itself. Coming up with a "cleaner" 
interface just results in more work for everyone. We should just go with 
the defacto standard, especially since it's one that's been implemented 
by various hardware vendors.

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

Re: [PATCH] Map volume and brightness events on thinkpads

[Linus Torvalds]

On Tue, 16 Oct 2007, Matthew Garrett wrote:
> >
> > It still doesn't mean it belongs inside the stream of data for the keyboard,
> > maskerading as a key press.
> 
> But it *is* a key press!

To get somewhat back on track: volume and brightness (and similar - lid 
close etc) events clearly are keypresses.

However, I would also argue that a keypress that is acted on by the 
firmware automatically is *different* from a keypress that hasn't been 
acted on: one is a "key was pressed *and* hardware did something 
automatically", and the other is just a "key was pressed" event.

IOW, I think the thinkpad issue (and others like it) should be fixed by 
splitting up the KEY_VOLUMEUP "key" into separate KEY_VOLUMEUP and 
KEY_VOLUMEUP_NOTIFY key events, so that downstream user mode (and the 
kernel itself, for that matter) can know whether it's a informational 
message or whether it should be acted upon.

Because clearly we seem to have both cases, and while I think we should 
try to move towards a "user mode does all actions" model where screen 
brightness is under the control of X, that isn't necessarily the case now, 
nor perhaps even reachable on all hw platforms.

Hmm?

		Linus

Re: [PATCH] Map volume and brightness events on thinkpads

[Matthew Garrett]

On Tue, Oct 16, 2007 at 12:14:32PM -0700, Linus Torvalds wrote:

> IOW, I think the thinkpad issue (and others like it) should be fixed by 
> splitting up the KEY_VOLUMEUP "key" into separate KEY_VOLUMEUP and 
> KEY_VOLUMEUP_NOTIFY key events, so that downstream user mode (and the 
> kernel itself, for that matter) can know whether it's a informational 
> message or whether it should be acted upon.

In principle I agree, but userspace already handles it. While it's 
certainly more correct, I'm not sure it provides any other real 
advantage.

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

Re: [PATCH] Map volume and brightness events on thinkpads

[Henrique de Moraes Holschuh]

On Tue, 16 Oct 2007, Matthew Garrett wrote:
> On Tue, Oct 16, 2007 at 12:14:32PM -0700, Linus Torvalds wrote:
> > IOW, I think the thinkpad issue (and others like it) should be fixed by 
> > splitting up the KEY_VOLUMEUP "key" into separate KEY_VOLUMEUP and 
> > KEY_VOLUMEUP_NOTIFY key events, so that downstream user mode (and the 
> > kernel itself, for that matter) can know whether it's a informational 
> > message or whether it should be acted upon.
> 
> In principle I agree, but userspace already handles it. While it's 
> certainly more correct, I'm not sure it provides any other real 
> advantage.

Let's put it in another way:

I'd go with Linus suggestion, as it is not icky (although it is a lot more
limited in scope than adding a flag to EV_* that could be use for any event,
etc).

I am not going with the current icky hackery that makes every consumer of
EV_* need to know when they are real events, or "notification" events using
outside information.  It is not "userspace already handles it", even.  it is
"HAL already handles it", BTW.

And unless Dmitry agrees with Linus' suggestion (maybe as a temporary
stopgap while something better gets written?), we are not going anywhere,
anyway.  

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] Map volume and brightness events on thinkpads

[Dmitry Torokhov]

On 10/16/07, Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote:
>
> And unless Dmitry agrees with Linus' suggestion (maybe as a temporary
> stopgap while something better gets written?), we are not going anywhere,
> anyway.

"Nothing is more permanent than a temporary soution" (not sure who
said this). :)
If we do that we will have to live with it for many years.

-- 
Dmitry

Re: [PATCH] Map volume and brightness events on thinkpads

[Dmitry Torokhov]

On 10/16/07, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>
> On Tue, 16 Oct 2007, Matthew Garrett wrote:
> > >
> > > It still doesn't mean it belongs inside the stream of data for the keyboard,
> > > maskerading as a key press.
> >
> > But it *is* a key press!
>
> To get somewhat back on track: volume and brightness (and similar - lid
> close etc) events clearly are keypresses.
>
> However, I would also argue that a keypress that is acted on by the
> firmware automatically is *different* from a keypress that hasn't been
> acted on: one is a "key was pressed *and* hardware did something
> automatically", and the other is just a "key was pressed" event.
>
> IOW, I think the thinkpad issue (and others like it) should be fixed by
> splitting up the KEY_VOLUMEUP "key" into separate KEY_VOLUMEUP and
> KEY_VOLUMEUP_NOTIFY key events, so that downstream user mode (and the
> kernel itself, for that matter) can know whether it's a informational
> message or whether it should be acted upon.

I agree that these are 2 different events. My argument is that
"VOLUME_UP_NOTIFY" event is similar to "BATTERY_OUT_NOTIFY",
"DOCK_UNDOCK_NOTIFY", etc, etc and should be sent not through input
layer but through a generic (yet to be designed) notification
mechanism. Something lighter than input. Something like uevents over
netlink.


-- 
Dmitry

Re: [PATCH] Map volume and brightness events on thinkpads

[Jeremy Katz]

On Tue, 2007-10-16 at 16:12 -0400, Dmitry Torokhov wrote:
> On 10/16/07, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > On Tue, 16 Oct 2007, Matthew Garrett wrote:
> > > >
> > > > It still doesn't mean it belongs inside the stream of data for the keyboard,
> > > > maskerading as a key press.
> > >
> > > But it *is* a key press!
> >
> > To get somewhat back on track: volume and brightness (and similar - lid
> > close etc) events clearly are keypresses.
> >
> > However, I would also argue that a keypress that is acted on by the
> > firmware automatically is *different* from a keypress that hasn't been
> > acted on: one is a "key was pressed *and* hardware did something
> > automatically", and the other is just a "key was pressed" event.
> >
> > IOW, I think the thinkpad issue (and others like it) should be fixed by
> > splitting up the KEY_VOLUMEUP "key" into separate KEY_VOLUMEUP and
> > KEY_VOLUMEUP_NOTIFY key events, so that downstream user mode (and the
> > kernel itself, for that matter) can know whether it's a informational
> > message or whether it should be acted upon.
> 
> I agree that these are 2 different events. My argument is that
> "VOLUME_UP_NOTIFY" event is similar to "BATTERY_OUT_NOTIFY",
> "DOCK_UNDOCK_NOTIFY", etc, etc and should be sent not through input
> layer but through a generic (yet to be designed) notification
> mechanism. Something lighter than input. Something like uevents over
> netlink.

Except that I'm _always_ going to have to be able to take these events
as input events (because of the hardware that sends them that way), so
why not just have everything be an input event?

The alternative is building up something new from the ground-up and then
having to do translation from input to the new event type (either in the
kernel or in userspace) which ends up meaning more work for little, if
any, gain

Jeremy

Re: [PATCH] Map volume and brightness events on thinkpads

[Dmitry Torokhov]

On 10/16/07, Jeremy Katz <katzj@redhat.com> wrote:
> On Tue, 2007-10-16 at 16:12 -0400, Dmitry Torokhov wrote:
> > On 10/16/07, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > > On Tue, 16 Oct 2007, Matthew Garrett wrote:
> > > > >
> > > > > It still doesn't mean it belongs inside the stream of data for the keyboard,
> > > > > maskerading as a key press.
> > > >
> > > > But it *is* a key press!
> > >
> > > To get somewhat back on track: volume and brightness (and similar - lid
> > > close etc) events clearly are keypresses.
> > >
> > > However, I would also argue that a keypress that is acted on by the
> > > firmware automatically is *different* from a keypress that hasn't been
> > > acted on: one is a "key was pressed *and* hardware did something
> > > automatically", and the other is just a "key was pressed" event.
> > >
> > > IOW, I think the thinkpad issue (and others like it) should be fixed by
> > > splitting up the KEY_VOLUMEUP "key" into separate KEY_VOLUMEUP and
> > > KEY_VOLUMEUP_NOTIFY key events, so that downstream user mode (and the
> > > kernel itself, for that matter) can know whether it's a informational
> > > message or whether it should be acted upon.
> >
> > I agree that these are 2 different events. My argument is that
> > "VOLUME_UP_NOTIFY" event is similar to "BATTERY_OUT_NOTIFY",
> > "DOCK_UNDOCK_NOTIFY", etc, etc and should be sent not through input
> > layer but through a generic (yet to be designed) notification
> > mechanism. Something lighter than input. Something like uevents over
> > netlink.
>
> Except that I'm _always_ going to have to be able to take these events
> as input events (because of the hardware that sends them that way), so
> why not just have everything be an input event?

Not all hardware, just some.

>
> The alternative is building up something new from the ground-up and then
> having to do translation from input to the new event type (either in the
> kernel or in userspace) which ends up meaning more work for little, if
> any, gain
>

No, I dont think you need to translate. You ahev 2 types of events -
ones require you to take action, others don't. Ones requests (reqular
keypresses), others just notifications.

Right now we have instance where sending events through input device
is simp-ly convenient because driver already has it. But in other
scenarios, when there is no input device in sight, input layer is not
the best transport. Input devices are quite havy (several kilobytes)
you may not want to add them everywhere.

-- 
Dmitry

Re: [PATCH] Map volume and brightness events on thinkpads

[Linus Torvalds]

On Tue, 16 Oct 2007, Dmitry Torokhov wrote:
> 
> I agree that these are 2 different events. My argument is that
> "VOLUME_UP_NOTIFY" event is similar to "BATTERY_OUT_NOTIFY",
> "DOCK_UNDOCK_NOTIFY", etc, etc and should be sent not through input
> layer but through a generic (yet to be designed) notification
> mechanism. Something lighter than input. Something like uevents over
> netlink.

Well, I'd argue that:

 - it's going to be the same entity that cares in both cases (ie anybody 
   who is ready to accept VOLUME_UP keypresses is also the exact same 
   party that also wants to know if VOLUME_UP happened *independently*)

   Ergo: making it a separate "generic" notification is actually totally 
   counterproductive, because it just adds complexity.

 - it really is a keypress. Yes, it's a keypress with side effects, but 
   it still tends to have a distinct source, and as such it is interesting 
   *as* a keypress. 

   IOW: it should have all the same "incidental" side effects as any other 
   keypress. Example: I think it's reasonable to consider it an event as 
   far as the screen saver is concerned. In other words, it's not *just* a 
   "volume was raised" event. It's a "volume was raised, and the user 
   actually pressed a key to do so".

So I do think they are keypresses, although I also suspect that like many 
other magical keys, the "NOTIFY" version is often also totally hidden by 
hardware/firmware interactions (ie I'm pretty sure that many of those 
special keys will never be visible at all to the OS, because the firmware 
hides the fact that they were pressed entirely!)

		Linus

Re: [PATCH] Map volume and brightness events on thinkpads

[Dmitry Torokhov]

On 10/16/07, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> On Tue, 16 Oct 2007, Dmitry Torokhov wrote:
> >
> > I agree that these are 2 different events. My argument is that
> > "VOLUME_UP_NOTIFY" event is similar to "BATTERY_OUT_NOTIFY",
> > "DOCK_UNDOCK_NOTIFY", etc, etc and should be sent not through input
> > layer but through a generic (yet to be designed) notification
> > mechanism. Something lighter than input. Something like uevents over
> > netlink.
>
> Well, I'd argue that:
>
>  - it's going to be the same entity that cares in both cases (ie anybody
>   who is ready to accept VOLUME_UP keypresses is also the exact same
>   party that also wants to know if VOLUME_UP happened *independently*)
>
>   Ergo: making it a separate "generic" notification is actually totally
>   counterproductive, because it just adds complexity.

That is a good argument. If there are no other users for this other
transport then I agree, inventing it just for keypress notifications
is bad idea.

>  - it really is a keypress. Yes, it's a keypress with side effects, but
>   it still tends to have a distinct source, and as such it is interesting
>   *as* a keypress.
>
>   IOW: it should have all the same "incidental" side effects as any other
>   keypress. Example: I think it's reasonable to consider it an event as
>   far as the screen saver is concerned. In other words, it's not *just* a
>   "volume was raised" event. It's a "volume was raised, and the user
>   actually pressed a key to do so".
>

This is a good argument for having 2 separate types of events but not
for which transport shoudl be used to deliver it.

> So I do think they are keypresses, although I also suspect that like many
> other magical keys, the "NOTIFY" version is often also totally hidden by
> hardware/firmware interactions (ie I'm pretty sure that many of those
> special keys will never be visible at all to the OS, because the firmware
> hides the fact that they were pressed entirely!)
>

Yes, on my old Inspiron brightness is completely handled by firmware.
There is no ACPI, nor keyboard events generated whatsoever.

OK, I guess I would like to hear once again from userspace guys -
DBUS/HAL/etc. Do they see a need for a generic interface that can be
used for all kinds of events, not only related to keypresses. If they
say that they only care about notifications arising from keypresses
then I will add EV_NOTIFY type of events to input layer. What events
would we need? I can imagine:

KEY_BRIGHTNESSUP_NOTIFY
KEY_BRIGHTNESSDOWN_NOTIFY
KEY_BRIGHTNESSMIN_NOTIFY
KEY_BRIGHTNESSMAX_NOTIFY
KEY_VOLUMEUP_NOTIFY
KEY_VOLUMEDOWN_NOTIFY
KEY_MUTE_NOTIFY
KEY_DISPLAYSWITCH_NOTIFY
KEY_WIFI_NOTIFY

What else? And it better have "key" in its name, BATTERY_OUT_NOTIFY
won't fly ;)

-- 
Dmitry

Re: [PATCH] Map volume and brightness events on thinkpads

[Matthew Garrett]

On Wed, Oct 17, 2007 at 11:57:18AM -0400, Dmitry Torokhov wrote:
> say that they only care about notifications arising from keypresses
> then I will add EV_NOTIFY type of events to input layer. What events
> would we need? I can imagine:

Why use EV_NOTIFY? They're still keys. I'd also quibble with the need 
for adding _NOTIFY varients of already existing keys - the existing 
consumers can all cope already.

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

Re: [PATCH] Map volume and brightness events on thinkpads

[Henrique de Moraes Holschuh]

On Wed, 17 Oct 2007, Matthew Garrett wrote:
> On Wed, Oct 17, 2007 at 11:57:18AM -0400, Dmitry Torokhov wrote:
> > say that they only care about notifications arising from keypresses
> > then I will add EV_NOTIFY type of events to input layer. What events
> > would we need? I can imagine:
> 
> Why use EV_NOTIFY? They're still keys. I'd also quibble with the need 

Why not?  Why the heck do we have to keep the worst of the current mess,
when many drivers CAN (and sometimes have to) tell appart the two classes of
events?  Make notifications a separate thing from regular events, please.

The use of the same path for notifications and events are already a big
enough concession to the HAL model.  AFAIK, HAL is the only reason why on
most systems the userspace consumer of events and notification events are
the same.  HAL gets them all (as well as ACPI events, and whatever else its
helpers poll the system for), and then distributes them over to various
other paths.

On a HAL-less system, it is far less clear that anything that needs the real
events would bother with the notifications.  The only class of applications
that have an use for the notifications are OSD applets and the like.

> for adding _NOTIFY varients of already existing keys - the existing 
> consumers can all cope already.

If one adds EV_NOTIFY, one can just allow for the same KEY constants that
are valid for EV_KEY, and be done with that, I suppose.  But my take is that
Dmitry wants to limit the number of notifications going over input.

I'd rather we added an EV_NOTIFY *bit* that gets or-ed to the real EV_*
type, so that one can turn any event into a notification.  This is certainly
to be useful at least for EV_KEY and EV_SWITCH.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] Map volume and brightness events on thinkpads

[Dmitry Torokhov]

On 10/17/07, Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote:
> On Wed, 17 Oct 2007, Matthew Garrett wrote:
> > On Wed, Oct 17, 2007 at 11:57:18AM -0400, Dmitry Torokhov wrote:
> > > say that they only care about notifications arising from keypresses
> > > then I will add EV_NOTIFY type of events to input layer. What events
> > > would we need? I can imagine:
> >
> > Why use EV_NOTIFY? They're still keys. I'd also quibble with the need
>
> Why not?  Why the heck do we have to keep the worst of the current mess,
> when many drivers CAN (and sometimes have to) tell appart the two classes of
> events?  Make notifications a separate thing from regular events, please.
>
> The use of the same path for notifications and events are already a big
> enough concession to the HAL model.  AFAIK, HAL is the only reason why on
> most systems the userspace consumer of events and notification events are
> the same.  HAL gets them all (as well as ACPI events, and whatever else its
> helpers poll the system for), and then distributes them over to various
> other paths.

I have a nagging feeling that we rely on HAL and windows environments
too much nowadays. Last night I tried to susped my laptop while at KDM
prompt but apparently some service is only running in context of user
session and so nothing was there to recognize that suspend button was
pressed (Fedora 8).

>
> On a HAL-less system, it is far less clear that anything that needs the real
> events would bother with the notifications.  The only class of applications
> that have an use for the notifications are OSD applets and the like.
>
> > for adding _NOTIFY varients of already existing keys - the existing
> > consumers can all cope already.
>
> If one adds EV_NOTIFY, one can just allow for the same KEY constants that
> are valid for EV_KEY, and be done with that, I suppose.  But my take is that
> Dmitry wants to limit the number of notifications going over input.
>

That was not my primary goal. We have bitmaps of supported events in
input_dev structure. If we were to reuse KEY_* constants for EV_NOTIFY
then I have to make this bitmap the same size as input_dev->keybit,
which is 512 bit at the moment (and I am thinking that we do need to
extend it to 1024). I expect that we need much less KEY_*_NOTIFY
events so that we could easily fit them into 32 bits.

> I'd rather we added an EV_NOTIFY *bit* that gets or-ed to the real EV_*
> type, so that one can turn any event into a notification.  This is certainly
> to be useful at least for EV_KEY and EV_SWITCH.
>

The problem with this scenario is that userspace can never be sure if
input device is capable of only KEY_BLAH, only KEY_BLAH_NOTIFY or
both. Normally we let userspace query device for all supported events.

-- 
Dmitry

Re: [PATCH] Map volume and brightness events on thinkpads

[Henrique de Moraes Holschuh]

On Wed, 17 Oct 2007, Dmitry Torokhov wrote:
> > The use of the same path for notifications and events are already a big
> > enough concession to the HAL model.  AFAIK, HAL is the only reason why on
> > most systems the userspace consumer of events and notification events are
> > the same.  HAL gets them all (as well as ACPI events, and whatever else its
> > helpers poll the system for), and then distributes them over to various
> > other paths.
> 
> I have a nagging feeling that we rely on HAL and windows environments
> too much nowadays. Last night I tried to susped my laptop while at KDM
> prompt but apparently some service is only running in context of user
> session and so nothing was there to recognize that suspend button was
> pressed (Fedora 8).

Hrnf, you are not the only one.

> That was not my primary goal. We have bitmaps of supported events in
> input_dev structure. If we were to reuse KEY_* constants for EV_NOTIFY
> then I have to make this bitmap the same size as input_dev->keybit,
> which is 512 bit at the moment (and I am thinking that we do need to
> extend it to 1024). I expect that we need much less KEY_*_NOTIFY
> events so that we could easily fit them into 32 bits.

This would be a good reason to limit what can be a notification event, yes,
since it will increase the size of every input device.

> > I'd rather we added an EV_NOTIFY *bit* that gets or-ed to the real EV_*
> > type, so that one can turn any event into a notification.  This is certainly
> > to be useful at least for EV_KEY and EV_SWITCH.
> 
> The problem with this scenario is that userspace can never be sure if
> input device is capable of only KEY_BLAH, only KEY_BLAH_NOTIFY or
> both. Normally we let userspace query device for all supported events.

Everything capable of EV_foo would be able to issue EV_foo notifications.
While this is not optimal, it is probably good enough.  Still, I was
thinking about it, and a doubt came to mind: would it cause problems for a
bitmap to share the function for EV_foo and EV_foo notifications?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] Map volume and brightness events on thinkpads

[Dmitry Torokhov]

On 10/17/07, Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote:

> Still, I was
> thinking about it, and a doubt came to mind: would it cause problems for a
> bitmap to share the function for EV_foo and EV_foo notifications?
>

Not sure if I follow... Are you talking about bringing KEY_*_NOTIFY
into EV_KEY "namespace"? Could you elaborate?

-- 
Dmitry

Re: [PATCH] Map volume and brightness events on thinkpads

[Henrique de Moraes Holschuh]

On Thu, 18 Oct 2007, Dmitry Torokhov wrote:
> On 10/17/07, Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote:
> > Still, I was
> > thinking about it, and a doubt came to mind: would it cause problems for a
> > bitmap to share the function for EV_foo and EV_foo notifications?
> 
> Not sure if I follow... Are you talking about bringing KEY_*_NOTIFY
> into EV_KEY "namespace"? Could you elaborate?

Suppose we define a "EV_* is a notify event" bit to set in the event type
field of an input event.

Now, any type of event can be a notify event or a normal event, depending on
wether this bit is set.

However, the input layer keeps track of which events of a given type can be
sent by an input device using bitmaps, for every type of event.  And this
bitmap now would mean "input device may issue a normal event or a notify
event", not just "input device may issue a normal event".

I am not sure if that would cause trouble?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] Map volume and brightness events on thinkpads

[Dmitry Torokhov]

On 10/18/07, Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote:
> On Thu, 18 Oct 2007, Dmitry Torokhov wrote:
> > On 10/17/07, Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote:
> > > Still, I was
> > > thinking about it, and a doubt came to mind: would it cause problems for a
> > > bitmap to share the function for EV_foo and EV_foo notifications?
> >
> > Not sure if I follow... Are you talking about bringing KEY_*_NOTIFY
> > into EV_KEY "namespace"? Could you elaborate?
>
> Suppose we define a "EV_* is a notify event" bit to set in the event type
> field of an input event.
>
> Now, any type of event can be a notify event or a normal event, depending on
> wether this bit is set.
>
> However, the input layer keeps track of which events of a given type can be
> sent by an input device using bitmaps, for every type of event.  And this
> bitmap now would mean "input device may issue a normal event or a notify
> event", not just "input device may issue a normal event".
>
> I am not sure if that would cause trouble?
>

Like I said this would prevent userspace to know true capabilities of
the input device
in question. Probably simply adding separate key notify events (such
as KEY_BRIGHTNESSUP_NOTIFY) to EV_KEY instead of creating EV_NOTIFY is
not such a bad idea - this way we can fix keymap from userspace (if
needed) instead of needing to modify the krenel.

So, EV_KEY (and extending KEY_MAX to 1024) or EV_NOTIFY?

-- 
Dmitry

Re: [PATCH] Map volume and brightness events on thinkpads

[Henrique de Moraes Holschuh]

On Tue, 23 Oct 2007, Dmitry Torokhov wrote:
> in question. Probably simply adding separate key notify events (such
> as KEY_BRIGHTNESSUP_NOTIFY) to EV_KEY instead of creating EV_NOTIFY is
> not such a bad idea - this way we can fix keymap from userspace (if
> needed) instead of needing to modify the krenel.

That is a killer argument for EV_KEY over EV_NOTIFY if I have ever seen one.

> So, EV_KEY (and extending KEY_MAX to 1024) or EV_NOTIFY?

EV_KEY, please.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] Map volume and brightness events on thinkpads

[Kok, Auke]

Dmitry Torokhov wrote:
> On 10/16/07, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>
>> On Tue, 16 Oct 2007, Matthew Garrett wrote:
>>>> It still doesn't mean it belongs inside the stream of data for the keyboard,
>>>> maskerading as a key press.
>>> But it *is* a key press!
>> To get somewhat back on track: volume and brightness (and similar - lid
>> close etc) events clearly are keypresses.
>>
>> However, I would also argue that a keypress that is acted on by the
>> firmware automatically is *different* from a keypress that hasn't been
>> acted on: one is a "key was pressed *and* hardware did something
>> automatically", and the other is just a "key was pressed" event.
>>
>> IOW, I think the thinkpad issue (and others like it) should be fixed by
>> splitting up the KEY_VOLUMEUP "key" into separate KEY_VOLUMEUP and
>> KEY_VOLUMEUP_NOTIFY key events, so that downstream user mode (and the
>> kernel itself, for that matter) can know whether it's a informational
>> message or whether it should be acted upon.
> 
> I agree that these are 2 different events. My argument is that
> "VOLUME_UP_NOTIFY" event is similar to "BATTERY_OUT_NOTIFY",
> "DOCK_UNDOCK_NOTIFY", etc, etc and should be sent not through input
> layer but through a generic (yet to be designed) notification
> mechanism. Something lighter than input. Something like uevents over
> netlink.

absolutely

real life example: hook up one of those fancy usb keyboards with volume buttons to
your thinkpad. The volume keys on the thinkpad do adjust the volume, the ones on
the USB keyboard do not - software needs to be able to distinguish between them,
and sane defaults for these events need to not overlap.

Cheers,

Auke

Re: [PATCH] Map volume and brightness events on thinkpads

[Matthew Garrett]

On Tue, Oct 16, 2007 at 01:48:54PM -0700, Kok, Auke wrote:

> real life example: hook up one of those fancy usb keyboards with volume buttons to
> your thinkpad. The volume keys on the thinkpad do adjust the volume, the ones on
> the USB keyboard do not - software needs to be able to distinguish between them,
> and sane defaults for these events need to not overlap.

That's not a problem - they'll be coming from different input devices.

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

Re: [PATCH] Map volume and brightness events on thinkpads

[Dmitry Torokhov]

On 10/16/07, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Tue, Oct 16, 2007 at 01:48:54PM -0700, Kok, Auke wrote:
>
> > real life example: hook up one of those fancy usb keyboards with volume buttons to
> > your thinkpad. The volume keys on the thinkpad do adjust the volume, the ones on
> > the USB keyboard do not - software needs to be able to distinguish between them,
> > and sane defaults for these events need to not overlap.
>
> That's not a problem - they'll be coming from different input devices.
>

You could connect an external PS/2 keyboard and map some keys to
control the volume/brightness there. It most likely will be the same
device from the kerenl POV.

More important - is brightness and volume are the only notifications
userspace is interested in? Aren't there other events that might be
interesting to userspace?

-- 
Dmitry

Re: [PATCH] Map volume and brightness events on thinkpads

[Dave Airlie]

> > But it *is* a key press!
>
> To get somewhat back on track: volume and brightness (and similar - lid
> close etc) events clearly are keypresses.
>
> However, I would also argue that a keypress that is acted on by the
> firmware automatically is *different* from a keypress that hasn't been
> acted on: one is a "key was pressed *and* hardware did something
> automatically", and the other is just a "key was pressed" event.
>

We also have cases where userspace may have told ACPI to stop the
firmware acting on the keypress which may or may not be known to the
piece of the kernel dealing with keypresses... hence just pass the key
up to userspace and let it deal with it.

Dave.

Re: [PATCH] Map volume and brightness events on thinkpads

[Dmitry Torokhov]

Hi Henrique,

On 10/16/07, Henrique de Moraes Holschuh <hmh@hmh.eng.br> wrote:
> On Tue, 16 Oct 2007, Matthew Garrett wrote:
> > On Mon, Oct 15, 2007 at 07:07:37PM -0200, Henrique de Moraes Holschuh wrote:
> > > And the input subsystem maintainer has made it extremely clear in various
> > > threads that the input devices are *not* to be used as a notification
> > > service for on-screen-display or other such stuff.

Yes, I think different transport (like netlink) is better suited as a
generic notification transport. Input devices are pretty "heavy"
objects to add them everywhere.

>  If you send volume and
> > > brightness *key* events to userspace, it is supposed to act on them and
> > > raise/lower brightness/volume, which is the wrong thing to do on thinkpads.
> > > Never mind that HAL is ignoring the input maintainer's directions and
> > > violating this.
> >
> > Reality disagrees. There are already several cases where notifications
>
> Yes, it does.  But none of it cannot be fixed, HAL is the really big
> offender in userspace, and laptop drivers are the really big offenders in
> kernel space.
>
> > are sent via the keyboard controller, such as the wireless and touchpad
> > disable keys on my HP. There are Dells that do the same for brightness
>
> It is not clear to me if they are notifications or not.  Does the firmware
> act on the keys by itself? If it does, then they are notifications.  If it
> does not, then they are regular hot keys and there is no controversy whether
> they belong on the input layer or not (they do).
>
> > keys. Unless you want to make the argument that sending keyboard
> > controller events through the input layer is the wrong thing to do, it's
> > impossible to standardise on a setup where we never see notifications
> > through it.
>

I want to add the ability to add "filetrs" to i8042 keyboard ports so
that certain bytes that represent state changes (battery events,
wireless, etc.) can be diverted from keyboard serio port (and atkbd
driver) and be processed by a separate driver(s).

> Well, I better make it clear once again. Please excuse me the very direct
> and acid tone in the rest of this email, I want to make certain points
> crystal clear to everyone (and not just you, I do believe you are already
> very aware of my position in this):
>
> 1. I am not against sending notification events through input (but see point
> two below).  However, AFAIK, Dmitry is.  He said as much in the last thread
> about it.  I have added him to the CC list, he can make his position clear
> in this thread, if I got it wrong.
>
> Until I get a "you can do it" from Dmitry, forget about thinkpad-acpi
> sending such events over the input layer.  I believe in a coherent kernel
> where the schizophrenia on the use of the various interfaces and subsystems
> is kept to a bare minimum.  If a subsystem maintainers tell me not to do
> something, I won't do it.
>
> 2. I am against sending notification events through input **that look
> exactly the same as regular events**.  That is not a wise design choice IMO,
> it is a very dirty hack.
>
> I did propose ways to fix that properly, though (and to write the patches to
> do so).  Two simple and clean ways come to mind:
>
>        1. Add a new EV_* class for such events.
>        2. Alternatively, add a flags field to the higher bits of EV_*
>           class that could be used to mark events in a proper way.
>
> And I am sure there are other ways too, so it *can* be done properly if it
> is to be done at all.  However, it was not accepted because Dmitry does not
> want notifications going over the input subsystem as far as I recall from
> that thread.  Get Dmitry to accept a way to send notifications though the
> input layer, and I will follow it.
>
> 3. We have a backlight class, a LED class, a rf-kill class and ALSA mixers.
> Is there a real reason to pester Dmitry about the issue, if we can use these
> alternate paths (that are indeed more generic and more suited for the job)?
>

My position is that we should not cram all notifications into input
layer. We need something lighter and more flexible, that can mix
together battery charging, brightness increased, network link appeared
type of events. Maybe something like dbus strings + originating sysfs
device.

-- 
Dmitry

Re: [PATCH] Map volume and brightness events on thinkpads

[Matthew Garrett]

On Tue, Oct 16, 2007 at 10:27:11AM -0400, Dmitry Torokhov wrote:

> I want to add the ability to add "filetrs" to i8042 keyboard ports so
> that certain bytes that represent state changes (battery events,
> wireless, etc.) can be diverted from keyboard serio port (and atkbd
> driver) and be processed by a separate driver(s).

This seems to add complexity. I'm not sure why the input layer isn't the 
natural way of reporting events generated by a user pressing a key on 
the keyboard?

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

Re: [PATCH] Map volume and brightness events on thinkpads

[Dmitry Torokhov]

On 10/16/07, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Tue, Oct 16, 2007 at 10:27:11AM -0400, Dmitry Torokhov wrote:
>
> > I want to add the ability to add "filetrs" to i8042 keyboard ports so
> > that certain bytes that represent state changes (battery events,
> > wireless, etc.) can be diverted from keyboard serio port (and atkbd
> > driver) and be processed by a separate driver(s).
>
> This seems to add complexity. I'm not sure why the input layer isn't the
> natural way of reporting events generated by a user pressing a key on
> the keyboard?
>

I think there was a laptop that sent vierd bytes over KBC port when
you plug AC cord into it and pulled it out again. So it's not only
keys. Don't have a reference handy though.

-- 
Dmitry

Re: [PATCH] Map volume and brightness events on thinkpads

[Matthew Garrett]

On Tue, Oct 16, 2007 at 11:54:31AM -0400, Dmitry Torokhov wrote:

> I think there was a laptop that sent vierd bytes over KBC port when
> you plug AC cord into it and pulled it out again. So it's not only
> keys. Don't have a reference handy though.

That rings a bell. We can already get that information from other 
sources, though, so I'm not sure it's necessary to implement a way of 
splitting it out.

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

Re: [PATCH] Map volume and brightness events on thinkpads

[Matthew Garrett]

On Mon, Oct 15, 2007 at 04:45:10PM -0400, Jeremy Katz wrote:
> There are standard keycodes for brightness and volume; map the events to
> emit them so that things work properly

We've been doing this in Ubuntu for a couple of years now, with no 
obvious difficulty.

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

Re: [PATCH] Map volume and brightness events on thinkpads

[Renato S. Yamane]

Matthew Garrett wrote:
> On Mon, Oct 15, 2007 at 04:45:10PM -0400, Jeremy Katz wrote:
>> There are standard keycodes for brightness and volume; map the events to
>> emit them so that things work properly
> 
> We've been doing this in Ubuntu for a couple of years now, with no 
> obvious difficulty.

One detail: any Kernel don't recognize multimedia keys or couple keys 
(Fn+Fx) on my Toshiba M45-S355.
If I use xev, I can't get any output.

Regards,
Renato