[PATCH v2 1/5] platform/x86: intel-vbtn: support SW_TABLET_MODE

[PATCH v2 1/5] platform/x86: intel-vbtn: support SW_TABLET_MODE

[Stefan Brüns]

Event code 0xcc is emitted by several convertibles (Dell XPS 12 9Q33 BIOS
A8, Dell XPS 13 2in1 9365, HP Spectre x360, Lenovo Thinkpad Helix) when
entering tablet mode, and 0xcd on return to laptop mode.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

---

Changes in v2:
- Correct whitespace in key_entry struct

The changes where tested on an XPS 12 with BIOS version A8 (2015-03-03).
An earlier BIOS version (A2, ~2013) did not report event, at least
not using the INT33D6 plattform device.

There are other convertible laptops apparenly using the same event
codes:
https://wiki.gentoo.org/wiki/HP_Spectre_x360_(2015)
https://bugs.launchpad.net/ubuntu/+source/unity8/+bug/1678200
https://forums.opensuse.org/showthread.php/526850-Touchpad-and-trackpoint-no-longer-working-after-reattaching-convertible-keyboard

---
 drivers/platform/x86/intel-vbtn.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
index 58c5ff36523a..ae55be91a64b 100644
--- a/drivers/platform/x86/intel-vbtn.c
+++ b/drivers/platform/x86/intel-vbtn.c
@@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = {
 	{ KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },		/* volume-up key release */
 	{ KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },		/* volume-down key press */
 	{ KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },	/* volume-down key release */
+	{ KE_SW,     0xCC, { .sw = { SW_TABLET_MODE, 1 } } },	/* Tablet */
+	{ KE_SW,     0xCD, { .sw = { SW_TABLET_MODE, 0 } } },	/* Laptop */
 	{ KE_END },
 };
 
-- 
2.15.0

[PATCH v2 2/5] platform/x86: intel-vbtn: Support separate press/release events

[Stefan Brüns]

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(-)

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

[PATCH v2 3/5] Input: add KEY_ROTATE_LOCK_TOGGLE

[Stefan Brüns]

The key has the same use as the SW_ROTATE_LOCK, but is used on devices
where the state is not tracked by the hardware but has to be handled
in software.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

---

Changes in v2:
- New patch, add support for KEY_ROTATE_LOCK_TOGGLE

 include/uapi/linux/input-event-codes.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index f4058bd4c373..ba5d709a8923 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -593,6 +593,7 @@
 #define BTN_DPAD_RIGHT		0x223
 
 #define KEY_ALS_TOGGLE		0x230	/* Ambient light sensor */
+#define KEY_ROTATE_LOCK_TOGGLE	0x231	/* Display rotation lock */
 
 #define KEY_BUTTONCONFIG		0x240	/* AL Button Configuration */
 #define KEY_TASKMANAGER		0x241	/* AL Task/Project Manager */
-- 
2.15.0

[PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE

[Stefan Brüns]

The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but not
on BIOS A2).

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---

Changes in v2:
- Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
- Use separate up/down events

 drivers/platform/x86/intel-vbtn.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
index e3f6375af85c..a484bcc6393b 100644
--- a/drivers/platform/x86/intel-vbtn.c
+++ b/drivers/platform/x86/intel-vbtn.c
@@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = {
 	{ KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },		/* volume-up key release */
 	{ KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },		/* volume-down key press */
 	{ KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },	/* volume-down key release */
+	{ KE_KEY,    0xC8, { KEY_ROTATE_LOCK_TOGGLE } },	/* rotate-lock key press */
+	{ KE_KEY,    0xC9, { KEY_ROTATE_LOCK_TOGGLE } },	/* rotate-lock key release */
 	{ KE_SW,     0xCC, { .sw = { SW_TABLET_MODE, 1 } } },	/* Tablet */
 	{ KE_SW,     0xCD, { .sw = { SW_TABLET_MODE, 0 } } },	/* Laptop */
 	{ KE_END },
-- 
2.15.0

[PATCH v2 5/5] platform/x86: intel-vbtn: support panel front button

[Stefan Brüns]

The Lenovo Helix 2 and Dell XPS 12 (9Q33) have an extra button on the
front showing a 'Windows' logo, both reporting event codes 0xC2/0xC3
on press/release. On the Dell, both press/release are distinct events
while on the Helix 2 both events are generated on release.

Tested on XPS 12, for info on the Helix 2 see:
https://www.spinics.net/lists/ibm-acpi-devel/msg03982.html

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

---

Changes in v2:
- Emit KEY_LEFTMETA instead of KEY_MENU

 drivers/platform/x86/intel-vbtn.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
index a484bcc6393b..0861efe490d0 100644
--- a/drivers/platform/x86/intel-vbtn.c
+++ b/drivers/platform/x86/intel-vbtn.c
@@ -38,6 +38,8 @@ static const struct acpi_device_id intel_vbtn_ids[] = {
 static const struct key_entry intel_vbtn_keymap[] = {
 	{ KE_KEY, 0xC0, { KEY_POWER } },	/* power key press */
 	{ KE_IGNORE, 0xC1, { KEY_POWER } },	/* power key release */
+	{ KE_KEY, 0xC2, { KEY_LEFTMETA } },		/* 'Windows' key press */
+	{ KE_KEY, 0xC3, { KEY_LEFTMETA } },		/* 'Windows' key release */
 	{ KE_KEY, 0xC4, { KEY_VOLUMEUP } },		/* volume-up key press */
 	{ KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },		/* volume-up key release */
 	{ KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },		/* volume-down key press */
-- 
2.15.0

Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE

[Bastien Nocera]

On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but
> not
> on BIOS A2).
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> ---
> 
> Changes in v2:
> - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
> - Use separate up/down events
> 
>  drivers/platform/x86/intel-vbtn.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
> index e3f6375af85c..a484bcc6393b 100644
> --- a/drivers/platform/x86/intel-vbtn.c
> +++ b/drivers/platform/x86/intel-vbtn.c
> @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = {
>  	{ KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },		/* volume-up key release */
>  	{ KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },		/* volume-down key press */
>  	{ KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },	/* volume-down key release */
> +	{ KE_KEY,    0xC8, { KEY_ROTATE_LOCK_TOGGLE } },	/* rotate-lock key press */
> +	{ KE_KEY,    0xC9, { KEY_ROTATE_LOCK_TOGGLE } },	/* rotate-lock key release */

How are those events sent? When pressing and releasing the key, do you
receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and
releasing the first time, and 0xC9 when pressing and releasing a second
time?

If the former, then it's not going to work. The release is supposed to
be ignored, as you send the event with sparse_keymap_report_event().

If the latter, and there's an actual state, does it disable a device
on-board, or activate an LED? If so, then it would need to be a switch,
not a key.

>  	{ KE_SW,     0xCC, { .sw = { SW_TABLET_MODE, 1 } } },	/* Tablet */
>  	{ KE_SW,     0xCD, { .sw = { SW_TABLET_MODE, 0 } } },	/* Laptop */
>  	{ KE_END },

Re: [PATCH v2 3/5] Input: add KEY_ROTATE_LOCK_TOGGLE

[Bastien Nocera]

On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> The key has the same use as the SW_ROTATE_LOCK, but is used on
> devices
> where the state is not tracked by the hardware but has to be handled
> in software.

I'll let the input and hid subsystem maintainers have the final say
here, though I think that sending out Win+O would be easier, as this is
already something that desktops need to support for those devices that
send the actual Win+O key combination when those buttons are pressed
there.

If we're going with the separate keycode, could you also file bugs
against xkbd-common and xkeyboard-config to get the key added to the
list of XF86 keysyms and to the default evdev keymap? Otherwise Wayland
and X11 based desktops won't be able to use it.

Cheers

> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> 
> ---
> 
> Changes in v2:
> - New patch, add support for KEY_ROTATE_LOCK_TOGGLE
> 
>  include/uapi/linux/input-event-codes.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/linux/input-event-codes.h
> b/include/uapi/linux/input-event-codes.h
> index f4058bd4c373..ba5d709a8923 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -593,6 +593,7 @@
>  #define BTN_DPAD_RIGHT		0x223
>  
>  #define KEY_ALS_TOGGLE		0x230	/* Ambient light
> sensor */
> +#define KEY_ROTATE_LOCK_TOGGLE	0x231	/* Display
> rotation lock */
>  
>  #define KEY_BUTTONCONFIG		0x240	/* AL Button
> Configuration */
>  #define KEY_TASKMANAGER		0x241	/* AL
> Task/Project Manager */

Re: [PATCH v2 5/5] platform/x86: intel-vbtn: support panel front button

[Bastien Nocera]

On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> The Lenovo Helix 2 and Dell XPS 12 (9Q33) have an extra button on the
> front showing a 'Windows' logo, both reporting event codes 0xC2/0xC3
> on press/release. On the Dell, both press/release are distinct events
> while on the Helix 2 both events are generated on release.
> 
> Tested on XPS 12, for info on the Helix 2 see:
> https://www.spinics.net/lists/ibm-acpi-devel/msg03982.html
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

Same problem as mentioned in patch 3. Pretty sure you need to set the
Windows key release to KEY_IGNORE.

Or better, teach the intel-vbtn driver which buttons should be
autoreleased, and which ones should send key presses and key releases
separately. This would allow handling long presses in the upper layers.

> ---
> 
> Changes in v2:
> - Emit KEY_LEFTMETA instead of KEY_MENU
> 
>  drivers/platform/x86/intel-vbtn.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel-vbtn.c
> b/drivers/platform/x86/intel-vbtn.c
> index a484bcc6393b..0861efe490d0 100644
> --- a/drivers/platform/x86/intel-vbtn.c
> +++ b/drivers/platform/x86/intel-vbtn.c
> @@ -38,6 +38,8 @@ static const struct acpi_device_id intel_vbtn_ids[]
> = {
>  static const struct key_entry intel_vbtn_keymap[] = {
>  	{ KE_KEY, 0xC0, { KEY_POWER } },	/* power key press
> */
>  	{ KE_IGNORE, 0xC1, { KEY_POWER } },	/* power key
> release */
> +	{ KE_KEY, 0xC2, { KEY_LEFTMETA } },		/*
> 'Windows' key press */
> +	{ KE_KEY, 0xC3, { KEY_LEFTMETA } },		/*
> 'Windows' key release */
>  	{ KE_KEY, 0xC4, { KEY_VOLUMEUP } },		/*
> volume-up key press */
>  	{ KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },		/*
> volume-up key release */
>  	{ KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },		/*
> volume-down key press */

Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE

[Darren Hart]

On Fri, Nov 10, 2017 at 12:30:46AM +0100, Bastien Nocera wrote:
> On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but
> > not
> > on BIOS A2).
> > 
> > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> > ---
> > 
> > Changes in v2:
> > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
> > - Use separate up/down events
> > 
> >  drivers/platform/x86/intel-vbtn.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
> > index e3f6375af85c..a484bcc6393b 100644
> > --- a/drivers/platform/x86/intel-vbtn.c
> > +++ b/drivers/platform/x86/intel-vbtn.c
> > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = {
> >  	{ KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },		/* volume-up key release */
> >  	{ KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },		/* volume-down key press */
> >  	{ KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },	/* volume-down key release */
> > +	{ KE_KEY,    0xC8, { KEY_ROTATE_LOCK_TOGGLE } },	/* rotate-lock key press */
> > +	{ KE_KEY,    0xC9, { KEY_ROTATE_LOCK_TOGGLE } },	/* rotate-lock key release */
> 
> How are those events sent? When pressing and releasing the key, do you
> receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and
> releasing the first time, and 0xC9 when pressing and releasing a second
> time?
> 
> If the former, then it's not going to work. The release is supposed to
> be ignored, as you send the event with sparse_keymap_report_event().

I expect the former, which is consistent with the volume keys preceding it (also
ignoring the release).

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE

[Stefan Brüns]

[-- Attachment #1: Type: text/plain, Size: 2399 bytes --]

On Friday, November 10, 2017 12:30:46 AM CET Bastien Nocera wrote:
> On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but
> > not
> > on BIOS A2).
> > 
> > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> > ---
> > 
> > Changes in v2:
> > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
> > - Use separate up/down events
> > 
> >  drivers/platform/x86/intel-vbtn.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/intel-vbtn.c
> > b/drivers/platform/x86/intel-vbtn.c index e3f6375af85c..a484bcc6393b
> > 100644
> > --- a/drivers/platform/x86/intel-vbtn.c
> > +++ b/drivers/platform/x86/intel-vbtn.c
> > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = {
> > 
> >  	{ KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },		/* volume-up key release */
> >  	{ KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },		/* volume-down key press */
> >  	{ KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },	/* volume-down key release 
*/
> > 
> > +	{ KE_KEY,    0xC8, { KEY_ROTATE_LOCK_TOGGLE } },	/* rotate-lock key
> > press */ +	{ KE_KEY,    0xC9, { KEY_ROTATE_LOCK_TOGGLE } },	/*
> > rotate-lock key release */
> How are those events sent? When pressing and releasing the key, do you
> receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and
> releasing the first time, and 0xC9 when pressing and releasing a second
> time?
> 
> If the former, then it's not going to work. The release is supposed to
> be ignored, as you send the event with sparse_keymap_report_event().
> 
> If the latter, and there's an actual state, does it disable a device
> on-board, or activate an LED? If so, then it would need to be a switch,
> not a key.

Do you think I don't test the patches before sending? Let me tell you, it 
*does* work.

You could also read the cover letter, where you find more details, putting the 
patches in relation to each other.

Just in case its not yet clear:
The codes are emitted when pressing a button. It is a button, not a switch. 
There is no state handled in hardware. On press (as noted by the code 
comment), event code 0xc8 is emitted. On release, event code 0xc9 is emitted.

Regards,

Stefan
-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019

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

Re: [PATCH v2 5/5] platform/x86: intel-vbtn: support panel front button

[Stefan Brüns]

[-- Attachment #1: Type: text/plain, Size: 1253 bytes --]

On Friday, November 10, 2017 12:40:33 AM CET Bastien Nocera wrote:
> On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> > The Lenovo Helix 2 and Dell XPS 12 (9Q33) have an extra button on the
> > front showing a 'Windows' logo, both reporting event codes 0xC2/0xC3
> > on press/release. On the Dell, both press/release are distinct events
> > while on the Helix 2 both events are generated on release.
> > 
> > Tested on XPS 12, for info on the Helix 2 see:
> > https://www.spinics.net/lists/ibm-acpi-devel/msg03982.html
> > 
> > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> 
> Same problem as mentioned in patch 3. Pretty sure you need to set the
> Windows key release to KEY_IGNORE.
> 
> Or better, teach the intel-vbtn driver which buttons should be
> autoreleased, and which ones should send key presses and key releases
> separately. This would allow handling long presses in the upper layers.

First, I explicitly mentioned on the XPS 12, press/release are distinct 
events. Care to read?

Second, if you read patch 2/5, you see support for press/release vs 
autorelease.

Regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019

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

Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE

[Stefan Brüns]

[-- Attachment #1: Type: text/plain, Size: 2091 bytes --]

On Friday, November 10, 2017 12:46:30 AM CET Darren Hart wrote:
> On Fri, Nov 10, 2017 at 12:30:46AM +0100, Bastien Nocera wrote:
> > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but
> > > not
> > > on BIOS A2).
> > > 
> > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> > > ---
> > > 
> > > Changes in v2:
> > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
> > > - Use separate up/down events
> > > 
> > >  drivers/platform/x86/intel-vbtn.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/platform/x86/intel-vbtn.c
> > > b/drivers/platform/x86/intel-vbtn.c index e3f6375af85c..a484bcc6393b
> > > 100644
> > > --- a/drivers/platform/x86/intel-vbtn.c
> > > +++ b/drivers/platform/x86/intel-vbtn.c
> > > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = {
> > > 
> > >  	{ KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },		/* volume-up key release 
*/
> > >  	{ KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },		/* volume-down key press */
> > >  	{ KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },	/* volume-down key release 
*/
> > > 
> > > +	{ KE_KEY,    0xC8, { KEY_ROTATE_LOCK_TOGGLE } },	/* rotate-lock key
> > > press */ +	{ KE_KEY,    0xC9, { KEY_ROTATE_LOCK_TOGGLE } },	/*
> > > rotate-lock key release */> 
> > How are those events sent? When pressing and releasing the key, do you
> > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and
> > releasing the first time, and 0xC9 when pressing and releasing a second
> > time?
> > 
> > If the former, then it's not going to work. The release is supposed to
> > be ignored, as you send the event with sparse_keymap_report_event().
> 
> I expect the former, which is consistent with the volume keys preceding it
> (also ignoring the release).

Read the whole series and the cover letter, and stop making assumptions.

Regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019

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

Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE

[Bastien Nocera]

On Fri, 2017-11-10 at 01:15 +0100, Stefan Brüns wrote:
> On Friday, November 10, 2017 12:30:46 AM CET Bastien Nocera wrote:
> > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8,
> > > but
> > > not
> > > on BIOS A2).
> > > 
> > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> > > ---
> > > 
> > > Changes in v2:
> > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
> > > - Use separate up/down events
> > > 
> > >  drivers/platform/x86/intel-vbtn.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/platform/x86/intel-vbtn.c
> > > b/drivers/platform/x86/intel-vbtn.c index
> > > e3f6375af85c..a484bcc6393b
> > > 100644
> > > --- a/drivers/platform/x86/intel-vbtn.c
> > > +++ b/drivers/platform/x86/intel-vbtn.c
> > > @@ -42,6 +42,8 @@ static const struct key_entry
> > > intel_vbtn_keymap[] = {
> > > 
> > >  	{ KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },		/*
> > > volume-up key release */
> > >  	{ KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },		/*
> > > volume-down key press */
> > >  	{ KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },	/*
> > > volume-down key release 
> 
> */
> > > 
> > > +	{ KE_KEY,    0xC8, { KEY_ROTATE_LOCK_TOGGLE } },	
> > > /* rotate-lock key
> > > press */ +	{ KE_KEY,    0xC9, { KEY_ROTATE_LOCK_TOGGLE }
> > > },	/*
> > > rotate-lock key release */
> > 
> > How are those events sent? When pressing and releasing the key, do
> > you
> > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing
> > and
> > releasing the first time, and 0xC9 when pressing and releasing a
> > second
> > time?
> > 
> > If the former, then it's not going to work. The release is supposed
> > to
> > be ignored, as you send the event with
> > sparse_keymap_report_event().
> > 
> > If the latter, and there's an actual state, does it disable a
> > device
> > on-board, or activate an LED? If so, then it would need to be a
> > switch,
> > not a key.
> 
> Do you think I don't test the patches before sending? Let me tell
> you, it 
> *does* work.
> 
> You could also read the cover letter, where you find more details,
> putting the 
> patches in relation to each other.

I guess this was so clear that Darren made the same assumption as me. I
also never said that it wouldn't work, which is why I used "pretty
sure", and asked questions.

> Just in case its not yet clear:
> The codes are emitted when pressing a button. It is a button, not a
> switch. 
> There is no state handled in hardware. On press (as noted by the
> code 
> comment), event code 0xc8 is emitted. On release, event code 0xc9 is
> emitted.

>From the looks of things, some devices only send the keypresses, and
never the key release, otherwise what would be the point of ignoring
the key release in that table, say for the volume buttons? It's clear
as mud, and there's no comments in the driver to explain that.

If you don't want to answer questions, or are going to be as pissy as
your replies have been, I'm happy to not continue reviewing your
patches. I find patch 2 unreadable, but it might be my tiny tiny little
brain.

Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE

[Darren Hart]

On Fri, Nov 10, 2017 at 01:15:09AM +0100, Stefan Brüns wrote:
> On Friday, November 10, 2017 12:30:46 AM CET Bastien Nocera wrote:
> > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but
> > > not
> > > on BIOS A2).
> > > 
> > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> > > ---
> > > 
> > > Changes in v2:
> > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
> > > - Use separate up/down events
> > > 
> > >  drivers/platform/x86/intel-vbtn.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/platform/x86/intel-vbtn.c
> > > b/drivers/platform/x86/intel-vbtn.c index e3f6375af85c..a484bcc6393b
> > > 100644
> > > --- a/drivers/platform/x86/intel-vbtn.c
> > > +++ b/drivers/platform/x86/intel-vbtn.c
> > > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = {
> > > 
> > >  	{ KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },		/* volume-up key release */
> > >  	{ KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },		/* volume-down key press */
> > >  	{ KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },	/* volume-down key release 
> */
> > > 
> > > +	{ KE_KEY,    0xC8, { KEY_ROTATE_LOCK_TOGGLE } },	/* rotate-lock key
> > > press */ +	{ KE_KEY,    0xC9, { KEY_ROTATE_LOCK_TOGGLE } },	/*
> > > rotate-lock key release */
> > How are those events sent? When pressing and releasing the key, do you
> > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and
> > releasing the first time, and 0xC9 when pressing and releasing a second
> > time?
> > 
> > If the former, then it's not going to work. The release is supposed to
> > be ignored, as you send the event with sparse_keymap_report_event().
> > 
> > If the latter, and there's an actual state, does it disable a device
> > on-board, or activate an LED? If so, then it would need to be a switch,
> > not a key.
> 
> Do you think I don't test the patches before sending? Let me tell you, it 
> *does* work.
> 
> You could also read the cover letter, where you find more details, putting the 
> patches in relation to each other.

Dude, Knock it off. Unacceptable.

The number of patches we receive which have not seen any testing are
higher than you might think. Bastien took the time to review and ask
questions in the context of his experience with patches dealing with
input. These are very reasonable and necessary questions given we
cannot independently verify patches for most of these platforms.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v2 2/5] platform/x86: intel-vbtn: Support separate press/release events

[Darren Hart]

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?

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v2 3/5] Input: add KEY_ROTATE_LOCK_TOGGLE

[Darren Hart]

On Thu, Nov 09, 2017 at 11:44:34PM +0100, Stefan Brüns wrote:
> The key has the same use as the SW_ROTATE_LOCK, but is used on devices
> where the state is not tracked by the hardware but has to be handled
> in software.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> 

Dmitry, this lands in the middle of series that otherwise applies to
drivers/platform/x86. OK with you if we carry this one?

> ---
> 
> Changes in v2:
> - New patch, add support for KEY_ROTATE_LOCK_TOGGLE
> 
>  include/uapi/linux/input-event-codes.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> index f4058bd4c373..ba5d709a8923 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -593,6 +593,7 @@
>  #define BTN_DPAD_RIGHT		0x223
>  
>  #define KEY_ALS_TOGGLE		0x230	/* Ambient light sensor */
> +#define KEY_ROTATE_LOCK_TOGGLE	0x231	/* Display rotation lock */
>  
>  #define KEY_BUTTONCONFIG		0x240	/* AL Button Configuration */
>  #define KEY_TASKMANAGER		0x241	/* AL Task/Project Manager */
> -- 
> 2.15.0
> 
> 

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE

[Darren Hart]

On Fri, Nov 10, 2017 at 01:15:09AM +0100, Stefan Brüns wrote:
> On Friday, November 10, 2017 12:30:46 AM CET Bastien Nocera wrote:
> > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but
> > > not
> > > on BIOS A2).
> > > 
> > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> > > ---
> > > 
> > > Changes in v2:
> > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
> > > - Use separate up/down events
> > > 
> > >  drivers/platform/x86/intel-vbtn.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/platform/x86/intel-vbtn.c
> > > b/drivers/platform/x86/intel-vbtn.c index e3f6375af85c..a484bcc6393b
> > > 100644
> > > --- a/drivers/platform/x86/intel-vbtn.c
> > > +++ b/drivers/platform/x86/intel-vbtn.c
> > > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = {
> > > 
> > >  	{ KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },		/* volume-up key release */
> > >  	{ KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },		/* volume-down key press */
> > >  	{ KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },	/* volume-down key release 
> */
> > > 
> > > +	{ KE_KEY,    0xC8, { KEY_ROTATE_LOCK_TOGGLE } },	/* rotate-lock key
> > > press */ +	{ KE_KEY,    0xC9, { KEY_ROTATE_LOCK_TOGGLE } },	/*
> > > rotate-lock key release */
> > How are those events sent? When pressing and releasing the key, do you
> > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and
> > releasing the first time, and 0xC9 when pressing and releasing a second
> > time?
> > 
> > If the former, then it's not going to work. The release is supposed to
> > be ignored, as you send the event with sparse_keymap_report_event().
> > 
> > If the latter, and there's an actual state, does it disable a device
> > on-board, or activate an LED? If so, then it would need to be a switch,
> > not a key.
> 
> Do you think I don't test the patches before sending? Let me tell you, it 
> *does* work.
> 
> You could also read the cover letter, where you find more details, putting the 
> patches in relation to each other.

A point of process. If there is context that is needed to explain the
patch, it belongs in the patch, not just in the cover letter. The cover
letter is effectively lost once the patches are merged.

> 
> Just in case its not yet clear:
> The codes are emitted when pressing a button. It is a button, not a switch. 
> There is no state handled in hardware. On press (as noted by the code 
> comment), event code 0xc8 is emitted. On release, event code 0xc9 is emitted.

This sounds like the "former" scenario Bastien described, for which I understand
the use case to be:

User presses and releases the rotate lock button to prevent the accelerometer for
triggering screen rotation.

User presses and releases the rotate lock button to allow the accelerometer to
trigger screen rotation.

Is that correct?

If so, why do we need to emit two KEY_ROTATE_LOCK_TOGGLE events each time
instead of just the press event like the volume buttons?

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v2 2/5] platform/x86: intel-vbtn: Support separate press/release events

[Stefan Brüns]

[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]

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

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019

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

Re: [PATCH v2 2/5] platform/x86: intel-vbtn: Support separate press/release events

[Darren Hart]

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?

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v2 2/5] platform/x86: intel-vbtn: Support separate press/release events

[Darren Hart]

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
> 
> 

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE

[Stefan Brüns]

[-- Attachment #1: Type: text/plain, Size: 3702 bytes --]

On Friday, November 10, 2017 2:54:22 AM CET Darren Hart wrote:
> On Fri, Nov 10, 2017 at 01:15:09AM +0100, Stefan Brüns wrote:
> > On Friday, November 10, 2017 12:30:46 AM CET Bastien Nocera wrote:
> > > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> > > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but
> > > > not
> > > > on BIOS A2).
> > > > 
> > > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> > > > ---
> > > > 
> > > > Changes in v2:
> > > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
> > > > - Use separate up/down events
> > > > 
> > > >  drivers/platform/x86/intel-vbtn.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/platform/x86/intel-vbtn.c
> > > > b/drivers/platform/x86/intel-vbtn.c index e3f6375af85c..a484bcc6393b
> > > > 100644
> > > > --- a/drivers/platform/x86/intel-vbtn.c
> > > > +++ b/drivers/platform/x86/intel-vbtn.c
> > > > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] =
> > > > {
> > > > 
> > > >  	{ KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },		/* volume-up key release 
*/
> > > >  	{ KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },		/* volume-down key press 
*/
> > > >  	{ KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },	/* volume-down key 
release
> > 
> > */
> > 
> > > > +	{ KE_KEY,    0xC8, { KEY_ROTATE_LOCK_TOGGLE } },	/* rotate-lock key
> > > > press */ +	{ KE_KEY,    0xC9, { KEY_ROTATE_LOCK_TOGGLE } },	/*
> > > > rotate-lock key release */
> > > 
> > > How are those events sent? When pressing and releasing the key, do you
> > > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and
> > > releasing the first time, and 0xC9 when pressing and releasing a second
> > > time?
> > > 
> > > If the former, then it's not going to work. The release is supposed to
> > > be ignored, as you send the event with sparse_keymap_report_event().
> > > 
> > > If the latter, and there's an actual state, does it disable a device
> > > on-board, or activate an LED? If so, then it would need to be a switch,
> > > not a key.
> > 
> > Do you think I don't test the patches before sending? Let me tell you, it
> > *does* work.
> > 
> > You could also read the cover letter, where you find more details, putting
> > the patches in relation to each other.
> 
> A point of process. If there is context that is needed to explain the
> patch, it belongs in the patch, not just in the cover letter. The cover
> letter is effectively lost once the patches are merged.
> 
> > Just in case its not yet clear:
> > The codes are emitted when pressing a button. It is a button, not a
> > switch.
> > There is no state handled in hardware. On press (as noted by the code
> > comment), event code 0xc8 is emitted. On release, event code 0xc9 is
> > emitted.
> This sounds like the "former" scenario Bastien described, for which I
> understand the use case to be:
> 
> User presses and releases the rotate lock button to prevent the
> accelerometer for triggering screen rotation.
> 
> User presses and releases the rotate lock button to allow the accelerometer
> to trigger screen rotation.
> 
> Is that correct?
> 
> If so, why do we need to emit two KEY_ROTATE_LOCK_TOGGLE events each time
> instead of just the press event like the volume buttons?

Volume buttons *should* send separate press/release events, to allow software 
autorepeat.

For the rotate lock button, I see no reason *not* to report the actual state 
instead of doing an autorelease.

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019

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

Re: [PATCH v2 2/5] platform/x86: intel-vbtn: Support separate press/release events

[Stefan Brüns]

[-- Attachment #1: Type: text/plain, Size: 3108 bytes --]

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

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019

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

Re: [PATCH v2 3/5] Input: add KEY_ROTATE_LOCK_TOGGLE

[Brüns, Stefan]

On Freitag, 10. November 2017 00:34:53 CET Bastien Nocera wrote:
> On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> > The key has the same use as the SW_ROTATE_LOCK, but is used on
> > devices
> > where the state is not tracked by the hardware but has to be handled
> > in software.
> 
> I'll let the input and hid subsystem maintainers have the final say
> here, though I think that sending out Win+O would be easier, as this is
> already something that desktops need to support for those devices that
> send the actual Win+O key combination when those buttons are pressed
> there.
> 
> If we're going with the separate keycode, could you also file bugs
> against xkbd-common and xkeyboard-config to get the key added to the
> list of XF86 keysyms and to the default evdev keymap? Otherwise Wayland
> and X11 based desktops won't be able to use it.

Filed for xkeyboard-config:
https://bugs.freedesktop.org/show_bug.cgi?id=103998

and xkb-common:
https://github.com/xkbcommon/libxkbcommon/issues/55

Regards,

Stefan

Re: [PATCH v2 3/5] Input: add KEY_ROTATE_LOCK_TOGGLE

[Jason Gerecke]

Looks like this might also be needed for the MobileStudio Pro...

https://bugzilla.kernel.org/show_bug.cgi?id=197991

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one  /
(That is to say, eight) to the two,     /
But you can’t take seven from three,    /
So you look at the sixty-fours....



On Thu, Nov 30, 2017 at 9:51 AM, Brüns, Stefan
<Stefan.Bruens@rwth-aachen.de> wrote:
> On Freitag, 10. November 2017 00:34:53 CET Bastien Nocera wrote:
>> On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
>> > The key has the same use as the SW_ROTATE_LOCK, but is used on
>> > devices
>> > where the state is not tracked by the hardware but has to be handled
>> > in software.
>>
>> I'll let the input and hid subsystem maintainers have the final say
>> here, though I think that sending out Win+O would be easier, as this is
>> already something that desktops need to support for those devices that
>> send the actual Win+O key combination when those buttons are pressed
>> there.
>>
>> If we're going with the separate keycode, could you also file bugs
>> against xkbd-common and xkeyboard-config to get the key added to the
>> list of XF86 keysyms and to the default evdev keymap? Otherwise Wayland
>> and X11 based desktops won't be able to use it.
>
> Filed for xkeyboard-config:
> https://bugs.freedesktop.org/show_bug.cgi?id=103998
>
> and xkb-common:
> https://github.com/xkbcommon/libxkbcommon/issues/55
>
> Regards,
>
> Stefan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/5] platform/x86: intel-vbtn: Support separate press/release events

[Stefan Brüns]

[-- Attachment #1: Type: text/plain, Size: 2497 bytes --]

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);
> >  
> >  }


-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019

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

Re: [PATCH v2 3/5] Input: add KEY_ROTATE_LOCK_TOGGLE

[Jason Gerecke]

Any movement on this? Are we just waiting on an ACK/NAK from Dmitry?

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one  /
(That is to say, eight) to the two,     /
But you can’t take seven from three,    /
So you look at the sixty-fours....


On Tue, Dec 5, 2017 at 10:50 AM, Jason Gerecke <killertofu@gmail.com> wrote:
> Looks like this might also be needed for the MobileStudio Pro...
>
> https://bugzilla.kernel.org/show_bug.cgi?id=197991
>
> Jason
> ---
> Now instead of four in the eights place /
> you’ve got three, ‘Cause you added one  /
> (That is to say, eight) to the two,     /
> But you can’t take seven from three,    /
> So you look at the sixty-fours....
>
>
>
> On Thu, Nov 30, 2017 at 9:51 AM, Brüns, Stefan
> <Stefan.Bruens@rwth-aachen.de> wrote:
>> On Freitag, 10. November 2017 00:34:53 CET Bastien Nocera wrote:
>>> On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
>>> > The key has the same use as the SW_ROTATE_LOCK, but is used on
>>> > devices
>>> > where the state is not tracked by the hardware but has to be handled
>>> > in software.
>>>
>>> I'll let the input and hid subsystem maintainers have the final say
>>> here, though I think that sending out Win+O would be easier, as this is
>>> already something that desktops need to support for those devices that
>>> send the actual Win+O key combination when those buttons are pressed
>>> there.
>>>
>>> If we're going with the separate keycode, could you also file bugs
>>> against xkbd-common and xkeyboard-config to get the key added to the
>>> list of XF86 keysyms and to the default evdev keymap? Otherwise Wayland
>>> and X11 based desktops won't be able to use it.
>>
>> Filed for xkeyboard-config:
>> https://bugs.freedesktop.org/show_bug.cgi?id=103998
>>
>> and xkb-common:
>> https://github.com/xkbcommon/libxkbcommon/issues/55
>>
>> Regards,
>>
>> Stefan
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html