On Monday 08 June 2020 20:57:38 Y Paritcher wrote:
> On 6/8/20 8:26 PM, Mario.Limonciello@xxxxxxxx wrote:
> >> -----Original Message-----
> >> From: Pali Rohár <pali@xxxxxxxxxx>
> >> Sent: Monday, June 8, 2020 6:33 PM
> >> To: Y Paritcher
> >> Cc: linux-kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx;
> >> Matthew Garrett; Limonciello, Mario
> >> Subject: Re: [PATCH v2 2/3] platform/x86: dell-wmi: add new keymap type
> >> 0x0012
> >>
> >>
> >> [EXTERNAL EMAIL]
> >>
> >> On Monday 08 June 2020 19:05:29 Y Paritcher wrote:
> >>> These are events with extended data. The extended data is
> >>> currently ignored as userspace does not have a way to deal
> >>> it.
> >>>
> >>> Ignore event with a type of 0x0012 and a code of 0xe035, as
> >>> the keyboard controller takes care of Fn lock events by itself.
> >>
> >> Nice! This is information which is really important and need to have it
> >> documented.
> >>
> >>> 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
> >>>
> >>> This is consistent with the behavior for the Fn-lock key
> >>> elsewhere in this file.
> >>>
> >>> Signed-off-by: Y Paritcher <y.linux@xxxxxxxxxxxxx>
> >>
> >> I'm fine with this patch now.
> >>
> >> Reviewed-by: Pali Rohár <pali@xxxxxxxxxx>
> >>
> >>> ---
> >>> drivers/platform/x86/dell-wmi.c | 20 +++++++++++++++++++-
> >>> 1 file changed, 19 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-
> >> wmi.c
> >>> index 0b2edfe2767d..6b510f8431a3 100644
> >>> --- a/drivers/platform/x86/dell-wmi.c
> >>> +++ b/drivers/platform/x86/dell-wmi.c
> >>> @@ -334,6 +334,15 @@ 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
> >>> + * They are events with extended data
> >>> + */
> >>> +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);
> >>> @@ -418,10 +427,11 @@ static void dell_wmi_notify(struct wmi_device
> >> *wdev,
> >>>
> >>> switch (buffer_entry[1]) {
> >>> case 0x0000: /* One key pressed or event occurred */
> >>> + case 0x0012: /* Event with extended data occurred */
> >
> > I don't really like this being handled as a key as it's just discarding all
> > that extended data.
It is not ideal. But currently we handle all events as key press. And
non-key events are marked as ignored.
I agree, it should be handled in better way. But that needs to extend
driver. But it is something which should be done in next patches, not in
this one (if somebody has time to do it, etc...).
> >>
> >> Mario, are you able to get some official documentation for these 0x0012
> >> event types? I think it could be really useful for community so they can
> >> understand and add easily new type of code and events. Because currently
> >> we are just guessing what it could be. (It is sequence? Or single event?
> >> Or single event with extended data? It is generic event? Or it is real
> >> keypress? etc...)
> >
> > It's a single event with more data in the subsequent words. It is definitely
> > not a real keypress. It's supposed to be data that a user application would show.
> >
> > Remember the way WMI works on Linux and Windows is different. On Windows
> > userland applications get the events directly. On Linux kernel drivers get the
> > events and either use it internally, pass to another kernel driver or pass to
> > userland in the form of a translated event.
> >
> > So on Windows the whole buffer gets looked at directly by the application and the
> > application will decode it to show a translated string.
> >
> > I can certainly discuss internally about our team releasing a patch to export
> > all these other events. I would like to know what interface to recommend it pass
> > to userspace though, because as I said this is more than just a keycode that
> > comes through in the event. It's not useful to just do dev_info, it really should
> > be something that userspace can act on and show a translated message.
> > I don't think we want to add another 15 Dell specific keycodes to the kernel for the
> > various events and add another 4 more when a laptop introduces another set of keys.
> >
>
> We can treat this as a key for now. This gets rid of the unnecessary log messages.
>
> When adecision is made about how to handle extended events it will be very easy
> just to make this its own case statement and handle with it separately.
I agree. Handle it in the same way like we handle all other events. That
would make it easier to migrate to the new way, once it is there.
> However if you want to pass Fn lock event to userspace there are keys from older
> models of type 0010:
> /* Fn-lock switched to function keys */
> { KE_IGNORE, 0x0, { KEY_RESERVED } },
>
> /* Fn-lock switched to multimedia keys */
> { KE_IGNORE, 0x1, { KEY_RESERVED } },
>
> They should also be changed to be passed to userspace and they don't signify status
> with extended data rather by which event is called.
>
> This means that passing these types of events to userspace is a separate task, that
> might require rethinking the entire design of how all event types are treated.
>
> The current patch will be necessary regardless of what is decided in regards to
> passing the events on, and continues the status quo.
> >>
> >>> if (len > 2)
> >>> dell_wmi_process_key(wdev, 0x0000,
> >>> buffer_entry[2]);
> >>> - /* Other entries could contain additional information */
> >>> + /* Extended data is currently ignored */
> >>> break;
> >>> case 0x0010: /* Sequence of keys pressed */
> >>> case 0x0011: /* Sequence of events occurred */
> >>> @@ -556,6 +566,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 +611,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
> >>>
