| Message ID | 024c7574-12f2-4708-9f90-1192856df401@rwthex-w2-a.rwth-ad.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 09, 2017 at 11:44:33PM +0100, Stefan Brüns wrote: > Currently all key events use autorelease, but this forbids use as a > modifier key. > > As all event codes come in even/odd pairs, we can lookup the key type > (KE_KEY/KE_IGNORE) for the key up event corresponding to the currently > handled key down event. If the key up is ignored, we keep setting the > autorelease flag for the key down. > What is the use-case for using these buttons as modifiers? I'm picturing one of these devices in tablet mode, with a physical Windows button. What other action does a user want to modify by holding the Windows button down? Or is there another scenario we're trying to support here?
On Friday, November 10, 2017 2:34:17 AM CET Darren Hart wrote: > On Thu, Nov 09, 2017 at 11:44:33PM +0100, Stefan Brüns wrote: > > Currently all key events use autorelease, but this forbids use as a > > modifier key. > > > > As all event codes come in even/odd pairs, we can lookup the key type > > (KE_KEY/KE_IGNORE) for the key up event corresponding to the currently > > handled key down event. If the key up is ignored, we keep setting the > > autorelease flag for the key down. > > What is the use-case for using these buttons as modifiers? I'm picturing one > of these devices in tablet mode, with a physical Windows button. What other > action does a user want to modify by holding the Windows button down? Or is > there another scenario we're trying to support here? Windows/KEY_LEFTMETA can be used as a modifier key, e.g. in combination with the Volume Up/Down keys. On Windows, the default for Win + VolumeUp creates a screenshot. You can also use this in combination with an onscreen keyboard. Pressing the hardware button with the hand holding the tablet and typing with the other hand on the OSK is probably easier than hitting both keys on the OSK. Additionally, the Volume Up/Down currently do not autorepeat, as the key is autoreleased on the press event. The XPS 12 does issue distinct press/release events, so this could be done properly. The same apparently holds for some other convertibles, see the links in Patch 1/5. Kind regards, Stefan
On Fri, Nov 10, 2017 at 02:58:36AM +0100, Stefan Brüns wrote: > On Friday, November 10, 2017 2:34:17 AM CET Darren Hart wrote: > > On Thu, Nov 09, 2017 at 11:44:33PM +0100, Stefan Brüns wrote: > > > Currently all key events use autorelease, but this forbids use as a > > > modifier key. > > > > > > As all event codes come in even/odd pairs, we can lookup the key type > > > (KE_KEY/KE_IGNORE) for the key up event corresponding to the currently > > > handled key down event. If the key up is ignored, we keep setting the > > > autorelease flag for the key down. > > > > What is the use-case for using these buttons as modifiers? I'm picturing one > > of these devices in tablet mode, with a physical Windows button. What other > > action does a user want to modify by holding the Windows button down? Or is > > there another scenario we're trying to support here? > > Windows/KEY_LEFTMETA can be used as a modifier key, e.g. in combination with > the Volume Up/Down keys. On Windows, the default for Win + VolumeUp creates a > screenshot. > > You can also use this in combination with an onscreen keyboard. Pressing the > hardware button with the hand holding the tablet and typing with the other > hand on the OSK is probably easier than hitting both keys on the OSK. This all makes sense - good context for the commit message. If no other changes end up being needed, I'm happy to paste it in at merge. If changes are required, please add it in v3. > > Additionally, the Volume Up/Down currently do not autorepeat, as the key is > autoreleased on the press event. The XPS 12 does issue distinct press/release > events, so this could be done properly. The same apparently holds for some > other convertibles, see the links in Patch 1/5. It sounds like this is spotty across intel-vbtn implementations? Some devices emit release, others do not? How would you teach the driver about the differences? Assume autorelease and change the sparse keymap entry for DMI matched platforms with release? Doable... sounds unpleasant to maintain and update. Do we support this fully in userspace already?
On Thu, Nov 09, 2017 at 11:44:33PM +0100, Stefan Brüns wrote: > Currently all key events use autorelease, but this forbids use as a > modifier key. > > As all event codes come in even/odd pairs, we can lookup the key type > (KE_KEY/KE_IGNORE) for the key up event corresponding to the currently > handled key down event. If the key up is ignored, we keep setting the > autorelease flag for the key down. > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> +Dmitry, curious for your take on this approach. > > --- > > Changes in v2: > - New patch, add support for seperate key up/down in intel-vbtn > > drivers/platform/x86/intel-vbtn.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c > index ae55be91a64b..e3f6375af85c 100644 > --- a/drivers/platform/x86/intel-vbtn.c > +++ b/drivers/platform/x86/intel-vbtn.c > @@ -76,14 +76,27 @@ static void notify_handler(acpi_handle handle, u32 event, void *context) > { > struct platform_device *device = context; > struct intel_vbtn_priv *priv = dev_get_drvdata(&device->dev); > + const struct key_entry *ke_rel; > + bool autorelease; > > if (priv->wakeup_mode) { > if (sparse_keymap_entry_from_scancode(priv->input_dev, event)) { > pm_wakeup_hard_event(&device->dev); > return; > } > - } else if (sparse_keymap_report_event(priv->input_dev, event, 1, true)) { > - return; > + } else { > + /* Use the fact press/release come in even/odd pairs */ > + if ((event & 1) && sparse_keymap_report_event(priv->input_dev, > + event, 0, false)) > + return; > + > + ke_rel = sparse_keymap_entry_from_scancode(priv->input_dev, > + event | 1); > + autorelease = !ke_rel || ke_rel->type == KE_IGNORE; > + > + if (sparse_keymap_report_event(priv->input_dev, event, 1, > + autorelease)) > + return; > } > dev_dbg(&device->dev, "unknown event index 0x%x\n", event); > } > -- > 2.15.0 > >
On Friday, November 10, 2017 3:11:37 AM CET Darren Hart wrote: > On Fri, Nov 10, 2017 at 02:58:36AM +0100, Stefan Brüns wrote: > > On Friday, November 10, 2017 2:34:17 AM CET Darren Hart wrote: > > > On Thu, Nov 09, 2017 at 11:44:33PM +0100, Stefan Brüns wrote: > > > > Currently all key events use autorelease, but this forbids use as a > > > > modifier key. > > > > > > > > As all event codes come in even/odd pairs, we can lookup the key type > > > > (KE_KEY/KE_IGNORE) for the key up event corresponding to the currently > > > > handled key down event. If the key up is ignored, we keep setting the > > > > autorelease flag for the key down. > > > > > > What is the use-case for using these buttons as modifiers? I'm picturing > > > one of these devices in tablet mode, with a physical Windows button. > > > What other action does a user want to modify by holding the Windows > > > button down? Or is there another scenario we're trying to support here? > > > > Windows/KEY_LEFTMETA can be used as a modifier key, e.g. in combination > > with the Volume Up/Down keys. On Windows, the default for Win + VolumeUp > > creates a screenshot. > > > > You can also use this in combination with an onscreen keyboard. Pressing > > the hardware button with the hand holding the tablet and typing with the > > other hand on the OSK is probably easier than hitting both keys on the > > OSK. > This all makes sense - good context for the commit message. If no other > changes end up being needed, I'm happy to paste it in at merge. If changes > are required, please add it in v3. > > > Additionally, the Volume Up/Down currently do not autorepeat, as the key > > is > > autoreleased on the press event. The XPS 12 does issue distinct > > press/release events, so this could be done properly. The same apparently > > holds for some other convertibles, see the links in Patch 1/5. > > It sounds like this is spotty across intel-vbtn implementations? Some > devices emit release, others do not? How would you teach the driver about > the differences? Assume autorelease and change the sparse keymap entry for > DMI matched platforms with release? Doable... sounds unpleasant to maintain > and update. Do we support this fully in userspace already? - 0xcc/0xcd SW_TABLET mode seems to be consistent and widespread - 0xc4-0xc7 KEY_VOLUME_{UP,DOWN} seems to be consistent, although not used very often (Lenovo Helix 2, XPS 12, other Dell XPS), others probably use WMI - 0xc2/0xc3 KEY_LEFTMETA is used on Helix 2 and XPS 12, but only the XPS issues 0xc2 on press and 0xc3 on release, the Helix 2 issues both codes on release and none on press. The statements above are verified on the XPS 12, the other findings are from the Internet, search for "unknown event index" + "intel-vbtn". As far as I can see, there are no implementation which do not issue the events in pairs, so currently the quirks table would be empty. Kind regards, Stefan
On Friday, November 10, 2017 3:15:55 AM CET Darren Hart wrote: > On Thu, Nov 09, 2017 at 11:44:33PM +0100, Stefan Brüns wrote: > > Currently all key events use autorelease, but this forbids use as a > > modifier key. > > > > As all event codes come in even/odd pairs, we can lookup the key type > > (KE_KEY/KE_IGNORE) for the key up event corresponding to the currently > > handled key down event. If the key up is ignored, we keep setting the > > autorelease flag for the key down. > > > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > > +Dmitry, curious for your take on this approach. Any response would be appreciated. This is required to make Volume keys work properly. > > --- > > > > Changes in v2: > > - New patch, add support for seperate key up/down in intel-vbtn > > > > drivers/platform/x86/intel-vbtn.c | 17 +++++++++++++++-- > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/platform/x86/intel-vbtn.c > > b/drivers/platform/x86/intel-vbtn.c index ae55be91a64b..e3f6375af85c > > 100644 > > --- a/drivers/platform/x86/intel-vbtn.c > > +++ b/drivers/platform/x86/intel-vbtn.c > > @@ -76,14 +76,27 @@ static void notify_handler(acpi_handle handle, u32 > > event, void *context)> > > { > > > > struct platform_device *device = context; > > struct intel_vbtn_priv *priv = dev_get_drvdata(&device->dev); > > > > + const struct key_entry *ke_rel; > > + bool autorelease; > > > > if (priv->wakeup_mode) { > > > > if (sparse_keymap_entry_from_scancode(priv->input_dev, event)) { > > > > pm_wakeup_hard_event(&device->dev); > > return; > > > > } > > > > - } else if (sparse_keymap_report_event(priv->input_dev, event, 1, true)) > > { > > - return; > > + } else { > > + /* Use the fact press/release come in even/odd pairs */ > > + if ((event & 1) && sparse_keymap_report_event(priv->input_dev, > > + event, 0, false)) > > + return; > > + > > + ke_rel = sparse_keymap_entry_from_scancode(priv->input_dev, > > + event | 1); > > + autorelease = !ke_rel || ke_rel->type == KE_IGNORE; > > + > > + if (sparse_keymap_report_event(priv->input_dev, event, 1, > > + autorelease)) > > + return; > > > > } > > dev_dbg(&device->dev, "unknown event index 0x%x\n", event); > > > > }
diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c index ae55be91a64b..e3f6375af85c 100644 --- a/drivers/platform/x86/intel-vbtn.c +++ b/drivers/platform/x86/intel-vbtn.c @@ -76,14 +76,27 @@ static void notify_handler(acpi_handle handle, u32 event, void *context) { struct platform_device *device = context; struct intel_vbtn_priv *priv = dev_get_drvdata(&device->dev); + const struct key_entry *ke_rel; + bool autorelease; if (priv->wakeup_mode) { if (sparse_keymap_entry_from_scancode(priv->input_dev, event)) { pm_wakeup_hard_event(&device->dev); return; } - } else if (sparse_keymap_report_event(priv->input_dev, event, 1, true)) { - return; + } else { + /* Use the fact press/release come in even/odd pairs */ + if ((event & 1) && sparse_keymap_report_event(priv->input_dev, + event, 0, false)) + return; + + ke_rel = sparse_keymap_entry_from_scancode(priv->input_dev, + event | 1); + autorelease = !ke_rel || ke_rel->type == KE_IGNORE; + + if (sparse_keymap_report_event(priv->input_dev, event, 1, + autorelease)) + return; } dev_dbg(&device->dev, "unknown event index 0x%x\n", event); }
Currently all key events use autorelease, but this forbids use as a modifier key. As all event codes come in even/odd pairs, we can lookup the key type (KE_KEY/KE_IGNORE) for the key up event corresponding to the currently handled key down event. If the key up is ignored, we keep setting the autorelease flag for the key down. Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> --- Changes in v2: - New patch, add support for seperate key up/down in intel-vbtn drivers/platform/x86/intel-vbtn.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)