Re: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s

From: "Pali Rohár" <pali.rohar@gmail.com>
To: Mario_Limonciello@dell.com
Cc: dvhart@infradead.org, linux-kernel@vger.kernel.org,
	platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH] dell-wmi: Add events created by Dell Rugged 2-in-1s
Date: Wed, 27 Jul 2016 22:15:12 +0200	[thread overview]
Message-ID: <201607272215.12385@pali> (raw)
In-Reply-To: <13ec06e37e284263b6bf4fe4a6a04a78@ausx13mpc120.AMER.DELL.COM>

[-- Attachment #1: Type: Text/Plain, Size: 5199 bytes --]

On Wednesday 27 July 2016 21:03:57 Mario_Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Wednesday, July 27, 2016 11:06 AM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: dvhart@infradead.org; linux-kernel@vger.kernel.org;
> > platform-driver- x86@vger.kernel.org
> > Subject: Re: [PATCH] dell-wmi: Add events created by Dell Rugged
> > 2-in-1s
> > 
> > On Wednesday 27 July 2016 17:55:09 Mario_Limonciello@dell.com wrote:
> > > > Hi! I'm not sure if KEY_PROG1/2/3 are good names here as we
> > > > already use
> > > >
> > > >those for other actions (see bios_to_linux_keycode). Also there
> > > >is:
> > > I had missed this, do you have some recommendations on what would
> > > be better codes to map this to?
> > 
> > Problem is that I do not know when those KEY_PROG keys from
> > bios_to_linux_keycode table are emitted. There are missing comments
> > or description.
> > 
> > Are you able to find out description for all those keys in that
> > table? Maybe now (when linux key constants are extended), there
> > could be better candidates...
> 
> I'll ask around.  It's not immediately obvious to me though, maybe
> from old specs?

Do not know if it is old. At least some codes from it are used on my 
E6440 machine. Table itself was added by Rezwanul Kabir in this commit:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5ea2559726b786283236835dc2905c23b36ac91c

Maybe commit message could help you to indentify what it is...

> > > I'll double check what the things that "were" mapping to
> > > KEY_PROG1 etc actually were.  This might be a case of an
> > > expected clash if the functions aren't in current generations.
> > > 
> > > >/* Wifi Catcher */
> > > >
> > > > { KE_KEY,    0xe011, { KEY_PROG2 } },
> > > 
> > > It's worth mentioning that Wifi Catcher hasn't been used for any
> > > Dell laptops for a handful generations.  The rugged 2in1's are
> > > current generation that have these programmable buttons and
> > > don't have wifi catcher.
> > 
> > Anyway, what is "Wifi Catcher"? HW switch buttton which
> > enable/disable wifi? Or something else?
> 
> Wifi catcher was a special hardware switch slider switch.  When the
> machine was turned in S3 the sliding the switch beyond the on
> position would scan for available wifi networks and light up an LED
> if some from your predefined list were found.
> 
> When the machine was on, it would open up the applet that let you
> configure the behavior for the switch in S3.

Hm... maybe it better fit KEY_WLAN then? Just speculation.

> > > So there should be no "real" clash here.
> > 
> > Problem can be in future. This driver is unified for all Dell
> > products with wmi interface and so future product could do some
> > nasty things...
> 
> I agree with Darren here.
> 
> At least on Dell side we're creating new codes for "new" buttons and
> the limitation is really on the kernel side for how many KEY_PROG#
> or similar functions they can be re-assigned to.

Ok.

> Maybe it's time to think of another way to get this information to
> userspace rather that always translating them into key codes?

If event is generated by pressing key, button or hw switch, then input 
key is correct way. If there is no KEY define which fit for new vendor 
hw button, then I think we should start discussion with input subsystem 
how to handle such situation.

> There's a lot that are marked as KE_IGNORE because the kernel can't
> do anything interesting with them as no 1-1 mapping exists to a
> keycode.

Most marked as KE_IGNORE are because duplicate event is sent via 
keyboard controller or via other subsystem (e.g. rfkill).

But events which are not keys or buttons should not be sent via input 
devices. At least I think it is against usage of input devices.

Events like "battery was removed" or "keyboard backlight was changed" or 
"battery lifetime decreased under X %" can be useful for userspace 
applications. I agree. But I think we do not have any subsystem or 
interface for sending them from kernel to userspace.

If we start talking about creating interface for it, I would rather see 
something more generic, not Dell-only specific or created specially for 
Dell machines which will not fit for others...

Darren, what do you think about it?

> Those could still be passed on somehow to have things like
> gnome-settings-daemon or other userspace tools to react however and
> show a notification.

Yes, that can be useful and with some genetic way, other hardware can 
benefit from it too...

> > > > But probably we do not have better names...
> > > 
> > > In this particular case, I was thinking PROG1/2/3 were really the
> > > best option and would be most intuitive because the keys really
> > > are labels "P1" "P2" and "P3".
> > 
> > Probably yes, as I wrote I do not have in my mind better names for
> > now.
> 
> OK, I'll resend with the cosmetic tabbing change and a clearer
> description, but keep the same mapping.

Ok.

-- 
Pali Rohár
pali.rohar@gmail.com

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