RE: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012

[Mario.Limonciello]

> -----Original Message-----
> From: Y Paritcher <y.linux@xxxxxxxxxxxxx>
> Sent: Monday, June 8, 2020 3:13 PM
> To: Limonciello, Mario
> Cc: linux-kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx;
> mjg59@xxxxxxxxxxxxx; pali@xxxxxxxxxx
> Subject: Re: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
> 
> 
> [EXTERNAL EMAIL]
> 
> On 6/8/20 11:40 AM, Mario.Limonciello@xxxxxxxx wrote:
> >> -----Original Message-----
> >> From: platform-driver-x86-owner@xxxxxxxxxxxxxxx <platform-driver-x86-
> >> owner@xxxxxxxxxxxxxxx> On Behalf Of Y Paritcher
> >> Sent: Sunday, June 7, 2020 11:22 PM
> >> Cc: linux-kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx;
> >> Matthew Garrett; Pali RohÃr
> >> Subject: [PATCH 2/3] platform/x86: dell-wmi: add new keymap type 0x0012
> >>
> >>
> >> [EXTERNAL EMAIL]
> >>
> >> Ignore events with a type of 0x0012 and a code of 0xe035,
> >> this silences the following messages being logged when
> >> pressing the Fn-lock key on a Dell Inspiron 5593:
> >>
> >> dell_wmi: Unknown WMI event type 0x12
> >> dell_wmi: Unknown key with type 0x0012 and code 0xe035 pressed
> >
> > Event type 0x12 is for "System Events".  This is the type of events that
> > you typically would see come in for things "like" the wrong power adapter
> > being plugged in on Windows or stuff about plugging a Thunderbolt dock
> into
> > a port that doesn't support Thunderbolt.
> >
> > A message might look something like (paraphrased)
> > "Your system requires a 180W power adapter to charge effectively, but you
> > plugged in a 60W adapter."
> >
> > There often is extended data with these events.  As such I don't believe
> all
> > information in event type 0x0012 should be treated like scan codes like
> those in
> > 0x10 or 0x11.
> >
> > I would guess that Fn-lock on this machine probably has extended data in
> the next
> > showing whether it was turned on and off.  If it does, perhaps it makes
> sense to
> > send this information to userspace as an evdev switch instead.
> >
> 
> You are right.
> I had assumed (incorrectly) the were the same.
> I turned on dyndbg and got the events with the extended data.
> 
> Fn lock key switched to multimedia keys
> dell_wmi: Received WMI event (02 00 12 00 35 e0 01 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
> the extended data is e0 01
> 
> Fn-lock switched to function keys
> dell_wmi: Received WMI event (02 00 12 00 35 e0 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
> the extended data is e0 00

To be clear - do the function keys not send different scan codes on this laptop
in the two different modes?  I expected that they should be sending separate scan
codes.  If they are not sending different scan codes, then this actually needs
to be captured in the kernel and a translation map is needed which is platform
specific.

> 
> Therefore i agree this should have it's own case in `dell_wmi_process_key`
> but i am
> not sure yet how to deal with it. any suggestion are helpful.
> 
> About sending it to userspace, I just followed what was already done, if
> that is not
> desired we should change it for all the models.

Right, I don't think this was a bad first attempt.  I just think it's different
than the 0x10/0x11 events.

I'm not saying it shouldn't apply to more models, but just that events from
this 0x12 table should be treated differently.

I feel we need a different way to send these types of events to userspace
than a keycode.

I for example think that the power adapter and dock events are also potentially
useful but realistically userspace needs to be able to show translated messages to
a user.

Hans,

Can you please comment here how you would like to see events like this should come
through to userspace?

* Wrong power adapter (you have X and should have Y)
* You have plugged a dock into the wrong port
* Fn-lock mode

> >>
> >> Signed-off-by: Y Paritcher <y.linux@xxxxxxxxxxxxx>
> >> ---
> >>  drivers/platform/x86/dell-wmi.c | 17 +++++++++++++++++
> >>  1 file changed, 17 insertions(+)
> >>
> >> diff --git a/drivers/platform/x86/dell-wmi.c
> b/drivers/platform/x86/dell-
> >> wmi.c
> >> index 0b4f72f923cd..f37e7e9093c2 100644
> >> --- a/drivers/platform/x86/dell-wmi.c
> >> +++ b/drivers/platform/x86/dell-wmi.c
> >> @@ -334,6 +334,14 @@ static const struct key_entry
> >> dell_wmi_keymap_type_0011[] = {
> >>  	{ KE_IGNORE, KBD_LED_AUTO_100_TOKEN, { KEY_RESERVED } },
> >>  };
> >>
> >> +/*
> >> + * Keymap for WMI events of type 0x0012
> >> + */
> >> +static const struct key_entry dell_wmi_keymap_type_0012[] = {
> >> +	/* Fn-lock button pressed */
> >> +	{ KE_IGNORE, 0xe035, { KEY_RESERVED } },
> >> +};
> >> +
> >>  static void dell_wmi_process_key(struct wmi_device *wdev, int type, int
> >> code)
> >>  {
> >>  	struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
> >> @@ -425,6 +433,7 @@ static void dell_wmi_notify(struct wmi_device *wdev,
> >>  			break;
> >>  		case 0x0010: /* Sequence of keys pressed */
> >>  		case 0x0011: /* Sequence of events occurred */
> >> +		case 0x0012: /* Sequence of events occurred */
> >>  			for (i = 2; i < len; ++i)
> >>  				dell_wmi_process_key(wdev, buffer_entry[1],
> >>  						     buffer_entry[i]);
> >> @@ -556,6 +565,7 @@ static int dell_wmi_input_setup(struct wmi_device
> >> *wdev)
> >>  			 ARRAY_SIZE(dell_wmi_keymap_type_0000) +
> >>  			 ARRAY_SIZE(dell_wmi_keymap_type_0010) +
> >>  			 ARRAY_SIZE(dell_wmi_keymap_type_0011) +
> >> +			 ARRAY_SIZE(dell_wmi_keymap_type_0012) +
> >>  			 1,
> >>  			 sizeof(struct key_entry), GFP_KERNEL);
> >>  	if (!keymap) {
> >> @@ -600,6 +610,13 @@ static int dell_wmi_input_setup(struct wmi_device
> >> *wdev)
> >>  		pos++;
> >>  	}
> >>
> >> +	/* Append table with events of type 0x0012 */
> >> +	for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
> >> +		keymap[pos] = dell_wmi_keymap_type_0012[i];
> >> +		keymap[pos].code |= (0x0012 << 16);
> >> +		pos++;
> >> +	}
> >> +
> >>  	/*
> >>  	 * Now append also table with "legacy" events of type 0x0000. Some of
> >>  	 * them are reported also on laptops which have scancodes in DMI.
> >> --
> >> 2.27.0
> >