[PATCH 0/5] platform/x86: intel-vbtn: Fixes + rework to make it work on more devices

[PATCH 0/5] platform/x86: intel-vbtn: Fixes + rework to make it work on more devices

[Hans de Goede]

Hi All,

Here is a series of fixes, mostly aimed at fixing commit: de9647efeaa9
("platform/x86: intel-vbtn: Only activate tablet mode switch on 2-in-1's")
causing the driver to not bind on some devices where it could and
should report SW_TABLET_MODE.

The last commit makes the driver also work on some devices where it
previously would not work because they lack a VBDL method.

Mario, can you test this on a Dell XPS 9360 (for which you wrote the
de9647efeaa9 commit) to ensure that this series does not cause a
regression there?  Also I have a question for you about using the DMI
chassis-type for this / a proposal for dealing with this differently
below the '---' of the commit msg of the 4th patch.

Regards,

Hans

[PATCH 1/5] platform/x86: intel-vbtn: Use acpi_evaluate_integer()

[Hans de Goede]

Use acpi_evaluate_integer() instead of open-coding it.

This is a preparation patch for adding a intel_vbtn_has_switches()
helper function.

Fixes: de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode switch on 2-in-1's")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel-vbtn.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
index b5880936d785..191894d648bb 100644
--- a/drivers/platform/x86/intel-vbtn.c
+++ b/drivers/platform/x86/intel-vbtn.c
@@ -119,28 +119,21 @@ static void detect_tablet_mode(struct platform_device *device)
 	const char *chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
 	struct intel_vbtn_priv *priv = dev_get_drvdata(&device->dev);
 	acpi_handle handle = ACPI_HANDLE(&device->dev);
-	struct acpi_buffer vgbs_output = { ACPI_ALLOCATE_BUFFER, NULL };
-	union acpi_object *obj;
+	unsigned long long vgbs;
 	acpi_status status;
 	int m;
 
 	if (!(chassis_type && strcmp(chassis_type, "31") == 0))
-		goto out;
+		return;
 
-	status = acpi_evaluate_object(handle, "VGBS", NULL, &vgbs_output);
+	status = acpi_evaluate_integer(handle, "VGBS", NULL, &vgbs);
 	if (ACPI_FAILURE(status))
-		goto out;
-
-	obj = vgbs_output.pointer;
-	if (!(obj && obj->type == ACPI_TYPE_INTEGER))
-		goto out;
+		return;
 
-	m = !(obj->integer.value & TABLET_MODE_FLAG);
+	m = !(vgbs & TABLET_MODE_FLAG);
 	input_report_switch(priv->input_dev, SW_TABLET_MODE, m);
-	m = (obj->integer.value & DOCK_MODE_FLAG) ? 1 : 0;
+	m = (vgbs & DOCK_MODE_FLAG) ? 1 : 0;
 	input_report_switch(priv->input_dev, SW_DOCK, m);
-out:
-	kfree(vgbs_output.pointer);
 }
 
 static int intel_vbtn_probe(struct platform_device *device)
-- 
2.26.0

[PATCH 2/5] platform/x86: intel-vbtn: Split keymap into buttons and switches parts

[Hans de Goede]

Split the sparse keymap into 2 separate keymaps, a buttons and a switches
keymap and combine the 2 to a single map again in intel_vbtn_input_setup().

This is a preparation patch for not telling userspace that we have switches
when we do not have them (and for doing the same for the buttons).

Fixes: de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode switch on 2-in-1's")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel-vbtn.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
index 191894d648bb..634096cef21a 100644
--- a/drivers/platform/x86/intel-vbtn.c
+++ b/drivers/platform/x86/intel-vbtn.c
@@ -40,14 +40,20 @@ static const struct key_entry intel_vbtn_keymap[] = {
 	{ 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 */
+};
+
+static const struct key_entry intel_vbtn_switchmap[] = {
 	{ KE_SW,     0xCA, { .sw = { SW_DOCK, 1 } } },		/* Docked */
 	{ KE_SW,     0xCB, { .sw = { SW_DOCK, 0 } } },		/* Undocked */
 	{ KE_SW,     0xCC, { .sw = { SW_TABLET_MODE, 1 } } },	/* Tablet */
 	{ KE_SW,     0xCD, { .sw = { SW_TABLET_MODE, 0 } } },	/* Laptop */
-	{ KE_END },
 };
 
+#define KEYMAP_LEN \
+	(ARRAY_SIZE(intel_vbtn_keymap) + ARRAY_SIZE(intel_vbtn_switchmap) + 1)
+
 struct intel_vbtn_priv {
+	struct key_entry keymap[KEYMAP_LEN];
 	struct input_dev *input_dev;
 	bool wakeup_mode;
 };
@@ -55,13 +61,29 @@ struct intel_vbtn_priv {
 static int intel_vbtn_input_setup(struct platform_device *device)
 {
 	struct intel_vbtn_priv *priv = dev_get_drvdata(&device->dev);
-	int ret;
+	int ret, keymap_len = 0;
+
+	if (true) {
+		memcpy(&priv->keymap[keymap_len], intel_vbtn_keymap,
+		       ARRAY_SIZE(intel_vbtn_keymap) *
+		       sizeof(struct key_entry));
+		keymap_len += ARRAY_SIZE(intel_vbtn_keymap);
+	}
+
+	if (true) {
+		memcpy(&priv->keymap[keymap_len], intel_vbtn_switchmap,
+		       ARRAY_SIZE(intel_vbtn_switchmap) *
+		       sizeof(struct key_entry));
+		keymap_len += ARRAY_SIZE(intel_vbtn_switchmap);
+	}
+
+	priv->keymap[keymap_len].type = KE_END;
 
 	priv->input_dev = devm_input_allocate_device(&device->dev);
 	if (!priv->input_dev)
 		return -ENOMEM;
 
-	ret = sparse_keymap_setup(priv->input_dev, intel_vbtn_keymap, NULL);
+	ret = sparse_keymap_setup(priv->input_dev, priv->keymap, NULL);
 	if (ret)
 		return ret;
 
-- 
2.26.0

[PATCH 3/5] platform/x86: intel-vbtn: Do not advertise switches to userspace if they are not there

[Hans de Goede]

Commit de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode
switch on 2-in-1's") added a DMI chassis-type check to avoid accidentally
reporting SW_TABLET_MODE = 1 to userspace on laptops (specifically on the
Dell XPS 9360), to avoid e.g. userspace ignoring touchpad events because
userspace thought the device was in tablet-mode.

But if we are not getting the initial status of the switch because the
device does not have a tablet mode, then we really should not advertise
the presence of a tablet-mode switch to userspace at all, as userspace may
use the mere presence of this switch for certain heuristics.

Fixes: de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode switch on 2-in-1's")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel-vbtn.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
index 634096cef21a..500fae82e12c 100644
--- a/drivers/platform/x86/intel-vbtn.c
+++ b/drivers/platform/x86/intel-vbtn.c
@@ -55,6 +55,7 @@ static const struct key_entry intel_vbtn_switchmap[] = {
 struct intel_vbtn_priv {
 	struct key_entry keymap[KEYMAP_LEN];
 	struct input_dev *input_dev;
+	bool has_switches;
 	bool wakeup_mode;
 };
 
@@ -70,7 +71,7 @@ static int intel_vbtn_input_setup(struct platform_device *device)
 		keymap_len += ARRAY_SIZE(intel_vbtn_keymap);
 	}
 
-	if (true) {
+	if (priv->has_switches) {
 		memcpy(&priv->keymap[keymap_len], intel_vbtn_switchmap,
 		       ARRAY_SIZE(intel_vbtn_switchmap) *
 		       sizeof(struct key_entry));
@@ -138,16 +139,12 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 
 static void detect_tablet_mode(struct platform_device *device)
 {
-	const char *chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
 	struct intel_vbtn_priv *priv = dev_get_drvdata(&device->dev);
 	acpi_handle handle = ACPI_HANDLE(&device->dev);
 	unsigned long long vgbs;
 	acpi_status status;
 	int m;
 
-	if (!(chassis_type && strcmp(chassis_type, "31") == 0))
-		return;
-
 	status = acpi_evaluate_integer(handle, "VGBS", NULL, &vgbs);
 	if (ACPI_FAILURE(status))
 		return;
@@ -158,6 +155,19 @@ static void detect_tablet_mode(struct platform_device *device)
 	input_report_switch(priv->input_dev, SW_DOCK, m);
 }
 
+static bool intel_vbtn_has_switches(acpi_handle handle)
+{
+	const char *chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
+	unsigned long long vgbs;
+	acpi_status status;
+
+	if (!(chassis_type && strcmp(chassis_type, "31") == 0))
+		return false;
+
+	status = acpi_evaluate_integer(handle, "VGBS", NULL, &vgbs);
+	return ACPI_SUCCESS(status);
+}
+
 static int intel_vbtn_probe(struct platform_device *device)
 {
 	acpi_handle handle = ACPI_HANDLE(&device->dev);
@@ -176,13 +186,16 @@ static int intel_vbtn_probe(struct platform_device *device)
 		return -ENOMEM;
 	dev_set_drvdata(&device->dev, priv);
 
+	priv->has_switches = intel_vbtn_has_switches(handle);
+
 	err = intel_vbtn_input_setup(device);
 	if (err) {
 		pr_err("Failed to setup Intel Virtual Button\n");
 		return err;
 	}
 
-	detect_tablet_mode(device);
+	if (priv->has_switches)
+		detect_tablet_mode(device);
 
 	status = acpi_install_notify_handler(handle,
 					     ACPI_DEVICE_NOTIFY,
-- 
2.26.0

[PATCH 4/5] platform/x86: intel-vbtn: Also handle tablet-mode switch on "Detachable" and "Portable" chassis-types

[Hans de Goede]

Commit de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode
switch on 2-in-1's") added a DMI chassis-type check to avoid accidentally
reporting SW_TABLET_MODE = 1 to userspace on laptops.

Some devices with a detachable keyboard and using the intel-vbnt (INT33D6)
interface to report if they are in tablet mode (keyboard detached) or not,
report 32 / "Detachable" as chassis-type, e.g. the HP Pavilion X2 series.

Other devices with a detachable keyboard and using the intel-vbnt (INT33D6)
interface to report SW_TABLET_MODE, report 8 / "Portable" as chassis-type.
The Dell Venue 11 Pro 7130 is an example of this.

Extend the DMI chassis-type check to also accept Portables and Detachables
so that the intel-vbtn driver will report SW_TABLET_MODE on these devices.

Note the chassis-type check was originally added to avoid a false-positive
tablet-mode report on the Dell XPS 9360 laptop. To the best of my knowledge
that laptop is using a chassis-type of 9 / "Laptop", so after this commit
we still ignore the tablet-switch for that chassis-type.

Fixes: de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode switch on 2-in-1's")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Looking at the Microsoft Windows documentation for tablet-mode reporting:
https://docs.microsoft.com/en-us/windows-hardware/drivers/gpiobtn/button-implementation

Then the presence of a tablet-mode switch is indicated by the presence
of a PNP0C60 compatible ACPI devices. There are 2 ways in which this device
can report the tablet-mode. 1. Directly providing a GpioInt resource inside
the PNP0C60 device, 2. Through injecting events from a Windows driver.

It seems that the intel-vbtn / the INT33D6 ACPI device is the ACPI side
of Intel's generic solution for the case where the tablet-mode comes from
the embedded-controller and needs to be "injected".

This all suggests that it might be better to replace the chassis-type
check with a acpi_dev_present("PNP0C60", NULL, -1) check.

Mario, can you provide an acpidump and alsa-info.sh output for the
Dell XPS 9360, so that I can check if that might help with the issue
there, and thus is a potential candidate to replace the chassis-type
check?
---
 drivers/platform/x86/intel-vbtn.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
index 500fae82e12c..4921fc15dc6c 100644
--- a/drivers/platform/x86/intel-vbtn.c
+++ b/drivers/platform/x86/intel-vbtn.c
@@ -158,12 +158,22 @@ static void detect_tablet_mode(struct platform_device *device)
 static bool intel_vbtn_has_switches(acpi_handle handle)
 {
 	const char *chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
+	unsigned long chassis_type_int;
 	unsigned long long vgbs;
 	acpi_status status;
 
-	if (!(chassis_type && strcmp(chassis_type, "31") == 0))
+	if (kstrtoul(chassis_type, 10, &chassis_type_int))
 		return false;
 
+	switch (chassis_type_int) {
+	case  8: /* Portable */
+	case 31: /* Convertible */
+	case 32: /* Detachable */
+		break;
+	default:
+		return false;
+	}
+
 	status = acpi_evaluate_integer(handle, "VGBS", NULL, &vgbs);
 	return ACPI_SUCCESS(status);
 }
-- 
2.26.0

[PATCH 5/5] platform/x86: intel-vbtn: Fix probe failure on devices with only switches

[Hans de Goede]

On some devices the INT33D6 vbtn device is only used to report
tablet-mode / docked status (switches) and there are no vbtn managed
buttons.

On these devices there is no VBDL object.

Move the VBDL check to a intel_vbtn_has_buttons() helper and only exit
from intel_vbtn_probe() with -ENODEV when there are both no buttons and
no switches. Also only report the buttons being present to userspace if
the has_buttons check has succeeded.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel-vbtn.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
index 4921fc15dc6c..4efc70b693a7 100644
--- a/drivers/platform/x86/intel-vbtn.c
+++ b/drivers/platform/x86/intel-vbtn.c
@@ -55,6 +55,7 @@ static const struct key_entry intel_vbtn_switchmap[] = {
 struct intel_vbtn_priv {
 	struct key_entry keymap[KEYMAP_LEN];
 	struct input_dev *input_dev;
+	bool has_buttons;
 	bool has_switches;
 	bool wakeup_mode;
 };
@@ -64,7 +65,7 @@ static int intel_vbtn_input_setup(struct platform_device *device)
 	struct intel_vbtn_priv *priv = dev_get_drvdata(&device->dev);
 	int ret, keymap_len = 0;
 
-	if (true) {
+	if (priv->has_buttons) {
 		memcpy(&priv->keymap[keymap_len], intel_vbtn_keymap,
 		       ARRAY_SIZE(intel_vbtn_keymap) *
 		       sizeof(struct key_entry));
@@ -155,6 +156,14 @@ static void detect_tablet_mode(struct platform_device *device)
 	input_report_switch(priv->input_dev, SW_DOCK, m);
 }
 
+static bool intel_vbtn_has_buttons(acpi_handle handle)
+{
+	acpi_status status;
+
+	status = acpi_evaluate_object(handle, "VBDL", NULL, NULL);
+	return ACPI_SUCCESS(status);
+}
+
 static bool intel_vbtn_has_switches(acpi_handle handle)
 {
 	const char *chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
@@ -181,12 +190,15 @@ static bool intel_vbtn_has_switches(acpi_handle handle)
 static int intel_vbtn_probe(struct platform_device *device)
 {
 	acpi_handle handle = ACPI_HANDLE(&device->dev);
+	bool has_buttons, has_switches;
 	struct intel_vbtn_priv *priv;
 	acpi_status status;
 	int err;
 
-	status = acpi_evaluate_object(handle, "VBDL", NULL, NULL);
-	if (ACPI_FAILURE(status)) {
+	has_buttons = intel_vbtn_has_buttons(handle);
+	has_switches = intel_vbtn_has_switches(handle);
+
+	if (!has_buttons && !has_switches) {
 		dev_warn(&device->dev, "failed to read Intel Virtual Button driver\n");
 		return -ENODEV;
 	}
@@ -196,7 +208,8 @@ static int intel_vbtn_probe(struct platform_device *device)
 		return -ENOMEM;
 	dev_set_drvdata(&device->dev, priv);
 
-	priv->has_switches = intel_vbtn_has_switches(handle);
+	priv->has_buttons = has_buttons;
+	priv->has_switches = has_switches;
 
 	err = intel_vbtn_input_setup(device);
 	if (err) {
-- 
2.26.0

RE: [PATCH 4/5] platform/x86: intel-vbtn: Also handle tablet-mode switch on "Detachable" and "Portable" chassis-types

[Mario.Limonciello]

> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Saturday, May 2, 2020 1:30 PM
> To: Darren Hart; Andy Shevchenko; Limonciello, Mario
> Cc: Hans de Goede; linux-acpi@vger.kernel.org; platform-driver-
> x86@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 4/5] platform/x86: intel-vbtn: Also handle tablet-mode switch
> on "Detachable" and "Portable" chassis-types
> 
> 
> [EXTERNAL EMAIL]
> 
> Commit de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode
> switch on 2-in-1's") added a DMI chassis-type check to avoid accidentally
> reporting SW_TABLET_MODE = 1 to userspace on laptops.
> 
> Some devices with a detachable keyboard and using the intel-vbnt (INT33D6)
> interface to report if they are in tablet mode (keyboard detached) or not,
> report 32 / "Detachable" as chassis-type, e.g. the HP Pavilion X2 series.
> 
> Other devices with a detachable keyboard and using the intel-vbnt (INT33D6)
> interface to report SW_TABLET_MODE, report 8 / "Portable" as chassis-type.
> The Dell Venue 11 Pro 7130 is an example of this.
> 
> Extend the DMI chassis-type check to also accept Portables and Detachables
> so that the intel-vbtn driver will report SW_TABLET_MODE on these devices.
> 
> Note the chassis-type check was originally added to avoid a false-positive
> tablet-mode report on the Dell XPS 9360 laptop. To the best of my knowledge
> that laptop is using a chassis-type of 9 / "Laptop", so after this commit
> we still ignore the tablet-switch for that chassis-type.

Yes that's correct.

> 
> Fixes: de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode
> switch on 2-in-1's")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Looking at the Microsoft Windows documentation for tablet-mode reporting:
> https://docs.microsoft.com/en-us/windows-hardware/drivers/gpiobtn/button-
> implementation
> 
> Then the presence of a tablet-mode switch is indicated by the presence
> of a PNP0C60 compatible ACPI devices. There are 2 ways in which this device
> can report the tablet-mode. 1. Directly providing a GpioInt resource inside
> the PNP0C60 device, 2. Through injecting events from a Windows driver.
> 
> It seems that the intel-vbtn / the INT33D6 ACPI device is the ACPI side
> of Intel's generic solution for the case where the tablet-mode comes from
> the embedded-controller and needs to be "injected".
> 
> This all suggests that it might be better to replace the chassis-type
> check with a acpi_dev_present("PNP0C60", NULL, -1) check.
> 
> Mario, can you provide an acpidump and alsa-info.sh output for the
> Dell XPS 9360, so that I can check if that might help with the issue
> there, and thus is a potential candidate to replace the chassis-type
> check?

Unfortunately with WFH right now, I don't have access to a XPS 9630 to
double check the patch series.

However I do agree this should be a good approach.

Reviewed-by: Mario Limonciello <Mario.limonciello@dell.com>

> ---
>  drivers/platform/x86/intel-vbtn.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-
> vbtn.c
> index 500fae82e12c..4921fc15dc6c 100644
> --- a/drivers/platform/x86/intel-vbtn.c
> +++ b/drivers/platform/x86/intel-vbtn.c
> @@ -158,12 +158,22 @@ static void detect_tablet_mode(struct platform_device
> *device)
>  static bool intel_vbtn_has_switches(acpi_handle handle)
>  {
>  	const char *chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
> +	unsigned long chassis_type_int;
>  	unsigned long long vgbs;
>  	acpi_status status;
> 
> -	if (!(chassis_type && strcmp(chassis_type, "31") == 0))
> +	if (kstrtoul(chassis_type, 10, &chassis_type_int))
>  		return false;
> 
> +	switch (chassis_type_int) {
> +	case  8: /* Portable */
> +	case 31: /* Convertible */
> +	case 32: /* Detachable */
> +		break;
> +	default:
> +		return false;
> +	}
> +
>  	status = acpi_evaluate_integer(handle, "VGBS", NULL, &vgbs);
>  	return ACPI_SUCCESS(status);
>  }
> --
> 2.26.0

RE: [PATCH 0/5] platform/x86: intel-vbtn: Fixes + rework to make it work on more devices

[Mario.Limonciello]

> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Saturday, May 2, 2020 1:30 PM
> To: Darren Hart; Andy Shevchenko; Limonciello, Mario
> Cc: Hans de Goede; linux-acpi@vger.kernel.org; platform-driver-
> x86@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 0/5] platform/x86: intel-vbtn: Fixes + rework to make it work
> on more devices
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi All,
> 
> Here is a series of fixes, mostly aimed at fixing commit: de9647efeaa9
> ("platform/x86: intel-vbtn: Only activate tablet mode switch on 2-in-1's")
> causing the driver to not bind on some devices where it could and
> should report SW_TABLET_MODE.
> 
> The last commit makes the driver also work on some devices where it
> previously would not work because they lack a VBDL method.
> 
> Mario, can you test this on a Dell XPS 9360 (for which you wrote the
> de9647efeaa9 commit) to ensure that this series does not cause a
> regression there?

Unfortunately I can't double check that with WFH, I don't have access to this hardware.

> Also I have a question for you about using the DMI
> chassis-type for this / a proposal for dealing with this differently
> below the '---' of the commit msg of the 4th patch.


OK will look.

Re: [PATCH 4/5] platform/x86: intel-vbtn: Also handle tablet-mode switch on "Detachable" and "Portable" chassis-types

[Hans de Goede]

Hi,

On 5/4/20 5:37 PM, Mario.Limonciello@dell.com wrote:
> 
> 
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: Saturday, May 2, 2020 1:30 PM
>> To: Darren Hart; Andy Shevchenko; Limonciello, Mario
>> Cc: Hans de Goede; linux-acpi@vger.kernel.org; platform-driver-
>> x86@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: [PATCH 4/5] platform/x86: intel-vbtn: Also handle tablet-mode switch
>> on "Detachable" and "Portable" chassis-types
>>
>>
>> [EXTERNAL EMAIL]
>>
>> Commit de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode
>> switch on 2-in-1's") added a DMI chassis-type check to avoid accidentally
>> reporting SW_TABLET_MODE = 1 to userspace on laptops.
>>
>> Some devices with a detachable keyboard and using the intel-vbnt (INT33D6)
>> interface to report if they are in tablet mode (keyboard detached) or not,
>> report 32 / "Detachable" as chassis-type, e.g. the HP Pavilion X2 series.
>>
>> Other devices with a detachable keyboard and using the intel-vbnt (INT33D6)
>> interface to report SW_TABLET_MODE, report 8 / "Portable" as chassis-type.
>> The Dell Venue 11 Pro 7130 is an example of this.
>>
>> Extend the DMI chassis-type check to also accept Portables and Detachables
>> so that the intel-vbtn driver will report SW_TABLET_MODE on these devices.
>>
>> Note the chassis-type check was originally added to avoid a false-positive
>> tablet-mode report on the Dell XPS 9360 laptop. To the best of my knowledge
>> that laptop is using a chassis-type of 9 / "Laptop", so after this commit
>> we still ignore the tablet-switch for that chassis-type.
> 
> Yes that's correct.
> 
>>
>> Fixes: de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode
>> switch on 2-in-1's")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Looking at the Microsoft Windows documentation for tablet-mode reporting:
>> https://docs.microsoft.com/en-us/windows-hardware/drivers/gpiobtn/button-
>> implementation
>>
>> Then the presence of a tablet-mode switch is indicated by the presence
>> of a PNP0C60 compatible ACPI devices. There are 2 ways in which this device
>> can report the tablet-mode. 1. Directly providing a GpioInt resource inside
>> the PNP0C60 device, 2. Through injecting events from a Windows driver.
>>
>> It seems that the intel-vbtn / the INT33D6 ACPI device is the ACPI side
>> of Intel's generic solution for the case where the tablet-mode comes from
>> the embedded-controller and needs to be "injected".
>>
>> This all suggests that it might be better to replace the chassis-type
>> check with a acpi_dev_present("PNP0C60", NULL, -1) check.
>>
>> Mario, can you provide an acpidump and alsa-info.sh output for the
>> Dell XPS 9360, so that I can check if that might help with the issue
>> there, and thus is a potential candidate to replace the chassis-type
>> check?
> 
> Unfortunately with WFH right now, I don't have access to a XPS 9630 to
> double check the patch series.
> 
> However I do agree this should be a good approach.

Ok, so lets stick with the chassis-type check (as amended by this patch)
for now then. Then once you are able to go to your office again, we
can examine the acpi_dev_present("PNP0C60", NULL, -1) alternative.

> Reviewed-by: Mario Limonciello <Mario.limonciello@dell.com>

Thank you.

Regards,

Hans




>> ---
>>   drivers/platform/x86/intel-vbtn.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-
>> vbtn.c
>> index 500fae82e12c..4921fc15dc6c 100644
>> --- a/drivers/platform/x86/intel-vbtn.c
>> +++ b/drivers/platform/x86/intel-vbtn.c
>> @@ -158,12 +158,22 @@ static void detect_tablet_mode(struct platform_device
>> *device)
>>   static bool intel_vbtn_has_switches(acpi_handle handle)
>>   {
>>   	const char *chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
>> +	unsigned long chassis_type_int;
>>   	unsigned long long vgbs;
>>   	acpi_status status;
>>
>> -	if (!(chassis_type && strcmp(chassis_type, "31") == 0))
>> +	if (kstrtoul(chassis_type, 10, &chassis_type_int))
>>   		return false;
>>
>> +	switch (chassis_type_int) {
>> +	case  8: /* Portable */
>> +	case 31: /* Convertible */
>> +	case 32: /* Detachable */
>> +		break;
>> +	default:
>> +		return false;
>> +	}
>> +
>>   	status = acpi_evaluate_integer(handle, "VGBS", NULL, &vgbs);
>>   	return ACPI_SUCCESS(status);
>>   }
>> --
>> 2.26.0
> 

RE: [PATCH 4/5] platform/x86: intel-vbtn: Also handle tablet-mode switch on "Detachable" and "Portable" chassis-types

[Mario.Limonciello]

> -----Original Message-----
> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
> owner@vger.kernel.org> On Behalf Of Hans de Goede
> Sent: Tuesday, May 5, 2020 4:06 AM
> To: Limonciello, Mario; dvhart@infradead.org; andy@infradead.org
> Cc: linux-acpi@vger.kernel.org; platform-driver-x86@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 4/5] platform/x86: intel-vbtn: Also handle tablet-mode
> switch on "Detachable" and "Portable" chassis-types
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi,
> 
> On 5/4/20 5:37 PM, Mario.Limonciello@dell.com wrote:
> >
> >
> >> -----Original Message-----
> >> From: Hans de Goede <hdegoede@redhat.com>
> >> Sent: Saturday, May 2, 2020 1:30 PM
> >> To: Darren Hart; Andy Shevchenko; Limonciello, Mario
> >> Cc: Hans de Goede; linux-acpi@vger.kernel.org; platform-driver-
> >> x86@vger.kernel.org; linux-kernel@vger.kernel.org
> >> Subject: [PATCH 4/5] platform/x86: intel-vbtn: Also handle tablet-mode
> switch
> >> on "Detachable" and "Portable" chassis-types
> >>
> >>
> >> [EXTERNAL EMAIL]
> >>
> >> Commit de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode
> >> switch on 2-in-1's") added a DMI chassis-type check to avoid accidentally
> >> reporting SW_TABLET_MODE = 1 to userspace on laptops.
> >>
> >> Some devices with a detachable keyboard and using the intel-vbnt (INT33D6)
> >> interface to report if they are in tablet mode (keyboard detached) or not,
> >> report 32 / "Detachable" as chassis-type, e.g. the HP Pavilion X2 series.
> >>
> >> Other devices with a detachable keyboard and using the intel-vbnt (INT33D6)
> >> interface to report SW_TABLET_MODE, report 8 / "Portable" as chassis-type.
> >> The Dell Venue 11 Pro 7130 is an example of this.
> >>
> >> Extend the DMI chassis-type check to also accept Portables and Detachables
> >> so that the intel-vbtn driver will report SW_TABLET_MODE on these devices.
> >>
> >> Note the chassis-type check was originally added to avoid a false-positive
> >> tablet-mode report on the Dell XPS 9360 laptop. To the best of my knowledge
> >> that laptop is using a chassis-type of 9 / "Laptop", so after this commit
> >> we still ignore the tablet-switch for that chassis-type.
> >
> > Yes that's correct.
> >
> >>
> >> Fixes: de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode
> >> switch on 2-in-1's")
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >> Looking at the Microsoft Windows documentation for tablet-mode reporting:
> >> https://docs.microsoft.com/en-us/windows-hardware/drivers/gpiobtn/button-
> >> implementation
> >>
> >> Then the presence of a tablet-mode switch is indicated by the presence
> >> of a PNP0C60 compatible ACPI devices. There are 2 ways in which this device
> >> can report the tablet-mode. 1. Directly providing a GpioInt resource inside
> >> the PNP0C60 device, 2. Through injecting events from a Windows driver.
> >>
> >> It seems that the intel-vbtn / the INT33D6 ACPI device is the ACPI side
> >> of Intel's generic solution for the case where the tablet-mode comes from
> >> the embedded-controller and needs to be "injected".
> >>
> >> This all suggests that it might be better to replace the chassis-type
> >> check with a acpi_dev_present("PNP0C60", NULL, -1) check.
> >>
> >> Mario, can you provide an acpidump and alsa-info.sh output for the
> >> Dell XPS 9360, so that I can check if that might help with the issue
> >> there, and thus is a potential candidate to replace the chassis-type
> >> check?
> >
> > Unfortunately with WFH right now, I don't have access to a XPS 9630 to
> > double check the patch series.
> >
> > However I do agree this should be a good approach.
> 
> Ok, so lets stick with the chassis-type check (as amended by this patch)
> for now then. Then once you are able to go to your office again, we
> can examine the acpi_dev_present("PNP0C60", NULL, -1) alternative.

I know XPS 13's are pretty popular, perhaps someone on the mailing list who has
one can share ACPI dump in the interim.

> 
> > Reviewed-by: Mario Limonciello <Mario.limonciello@dell.com>
> 
> Thank you.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> >> ---
> >>   drivers/platform/x86/intel-vbtn.c | 12 +++++++++++-
> >>   1 file changed, 11 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-
> >> vbtn.c
> >> index 500fae82e12c..4921fc15dc6c 100644
> >> --- a/drivers/platform/x86/intel-vbtn.c
> >> +++ b/drivers/platform/x86/intel-vbtn.c
> >> @@ -158,12 +158,22 @@ static void detect_tablet_mode(struct platform_device
> >> *device)
> >>   static bool intel_vbtn_has_switches(acpi_handle handle)
> >>   {
> >>   	const char *chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
> >> +	unsigned long chassis_type_int;
> >>   	unsigned long long vgbs;
> >>   	acpi_status status;
> >>
> >> -	if (!(chassis_type && strcmp(chassis_type, "31") == 0))
> >> +	if (kstrtoul(chassis_type, 10, &chassis_type_int))
> >>   		return false;
> >>
> >> +	switch (chassis_type_int) {
> >> +	case  8: /* Portable */
> >> +	case 31: /* Convertible */
> >> +	case 32: /* Detachable */
> >> +		break;
> >> +	default:
> >> +		return false;
> >> +	}
> >> +
> >>   	status = acpi_evaluate_integer(handle, "VGBS", NULL, &vgbs);
> >>   	return ACPI_SUCCESS(status);
> >>   }
> >> --
> >> 2.26.0
> >

Re: [PATCH 4/5] platform/x86: intel-vbtn: Also handle tablet-mode switch on "Detachable" and "Portable" chassis-types

[Andy Shevchenko]

On Tue, May 5, 2020 at 5:22 PM <Mario.Limonciello@dell.com> wrote:
>
> > -----Original Message-----
> > From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
> > owner@vger.kernel.org> On Behalf Of Hans de Goede
> > Sent: Tuesday, May 5, 2020 4:06 AM
> > To: Limonciello, Mario; dvhart@infradead.org; andy@infradead.org
> > Cc: linux-acpi@vger.kernel.org; platform-driver-x86@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH 4/5] platform/x86: intel-vbtn: Also handle tablet-mode
> > switch on "Detachable" and "Portable" chassis-types
> >
> >
> > [EXTERNAL EMAIL]
> >
> > Hi,
> >
> > On 5/4/20 5:37 PM, Mario.Limonciello@dell.com wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Hans de Goede <hdegoede@redhat.com>
> > >> Sent: Saturday, May 2, 2020 1:30 PM
> > >> To: Darren Hart; Andy Shevchenko; Limonciello, Mario
> > >> Cc: Hans de Goede; linux-acpi@vger.kernel.org; platform-driver-
> > >> x86@vger.kernel.org; linux-kernel@vger.kernel.org
> > >> Subject: [PATCH 4/5] platform/x86: intel-vbtn: Also handle tablet-mode
> > switch
> > >> on "Detachable" and "Portable" chassis-types
> > >>
> > >>
> > >> [EXTERNAL EMAIL]
> > >>
> > >> Commit de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode
> > >> switch on 2-in-1's") added a DMI chassis-type check to avoid accidentally
> > >> reporting SW_TABLET_MODE = 1 to userspace on laptops.
> > >>
> > >> Some devices with a detachable keyboard and using the intel-vbnt (INT33D6)
> > >> interface to report if they are in tablet mode (keyboard detached) or not,
> > >> report 32 / "Detachable" as chassis-type, e.g. the HP Pavilion X2 series.
> > >>
> > >> Other devices with a detachable keyboard and using the intel-vbnt (INT33D6)
> > >> interface to report SW_TABLET_MODE, report 8 / "Portable" as chassis-type.
> > >> The Dell Venue 11 Pro 7130 is an example of this.
> > >>
> > >> Extend the DMI chassis-type check to also accept Portables and Detachables
> > >> so that the intel-vbtn driver will report SW_TABLET_MODE on these devices.
> > >>
> > >> Note the chassis-type check was originally added to avoid a false-positive
> > >> tablet-mode report on the Dell XPS 9360 laptop. To the best of my knowledge
> > >> that laptop is using a chassis-type of 9 / "Laptop", so after this commit
> > >> we still ignore the tablet-switch for that chassis-type.
> > >
> > > Yes that's correct.
> > >
> > >>
> > >> Fixes: de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode
> > >> switch on 2-in-1's")
> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > >> ---
> > >> Looking at the Microsoft Windows documentation for tablet-mode reporting:
> > >> https://docs.microsoft.com/en-us/windows-hardware/drivers/gpiobtn/button-
> > >> implementation
> > >>
> > >> Then the presence of a tablet-mode switch is indicated by the presence
> > >> of a PNP0C60 compatible ACPI devices. There are 2 ways in which this device
> > >> can report the tablet-mode. 1. Directly providing a GpioInt resource inside
> > >> the PNP0C60 device, 2. Through injecting events from a Windows driver.
> > >>
> > >> It seems that the intel-vbtn / the INT33D6 ACPI device is the ACPI side
> > >> of Intel's generic solution for the case where the tablet-mode comes from
> > >> the embedded-controller and needs to be "injected".
> > >>
> > >> This all suggests that it might be better to replace the chassis-type
> > >> check with a acpi_dev_present("PNP0C60", NULL, -1) check.
> > >>
> > >> Mario, can you provide an acpidump and alsa-info.sh output for the
> > >> Dell XPS 9360, so that I can check if that might help with the issue
> > >> there, and thus is a potential candidate to replace the chassis-type
> > >> check?
> > >
> > > Unfortunately with WFH right now, I don't have access to a XPS 9630 to
> > > double check the patch series.
> > >
> > > However I do agree this should be a good approach.
> >
> > Ok, so lets stick with the chassis-type check (as amended by this patch)
> > for now then. Then once you are able to go to your office again, we
> > can examine the acpi_dev_present("PNP0C60", NULL, -1) alternative.
>
> I know XPS 13's are pretty popular, perhaps someone on the mailing list who has
> one can share ACPI dump in the interim.

https://github.com/intel/dptfxtract/issues/13
?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/5] platform/x86: intel-vbtn: Also handle tablet-mode switch on "Detachable" and "Portable" chassis-types

[Hans de Goede]

Hi,

On 5/5/20 4:27 PM, Andy Shevchenko wrote:
> On Tue, May 5, 2020 at 5:22 PM <Mario.Limonciello@dell.com> wrote:
>>
>>> -----Original Message-----
>>> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
>>> owner@vger.kernel.org> On Behalf Of Hans de Goede
>>> Sent: Tuesday, May 5, 2020 4:06 AM
>>> To: Limonciello, Mario; dvhart@infradead.org; andy@infradead.org
>>> Cc: linux-acpi@vger.kernel.org; platform-driver-x86@vger.kernel.org; linux-
>>> kernel@vger.kernel.org
>>> Subject: Re: [PATCH 4/5] platform/x86: intel-vbtn: Also handle tablet-mode
>>> switch on "Detachable" and "Portable" chassis-types
>>>
>>>
>>> [EXTERNAL EMAIL]
>>>
>>> Hi,
>>>
>>> On 5/4/20 5:37 PM, Mario.Limonciello@dell.com wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Hans de Goede <hdegoede@redhat.com>
>>>>> Sent: Saturday, May 2, 2020 1:30 PM
>>>>> To: Darren Hart; Andy Shevchenko; Limonciello, Mario
>>>>> Cc: Hans de Goede; linux-acpi@vger.kernel.org; platform-driver-
>>>>> x86@vger.kernel.org; linux-kernel@vger.kernel.org
>>>>> Subject: [PATCH 4/5] platform/x86: intel-vbtn: Also handle tablet-mode
>>> switch
>>>>> on "Detachable" and "Portable" chassis-types
>>>>>
>>>>>
>>>>> [EXTERNAL EMAIL]
>>>>>
>>>>> Commit de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode
>>>>> switch on 2-in-1's") added a DMI chassis-type check to avoid accidentally
>>>>> reporting SW_TABLET_MODE = 1 to userspace on laptops.
>>>>>
>>>>> Some devices with a detachable keyboard and using the intel-vbnt (INT33D6)
>>>>> interface to report if they are in tablet mode (keyboard detached) or not,
>>>>> report 32 / "Detachable" as chassis-type, e.g. the HP Pavilion X2 series.
>>>>>
>>>>> Other devices with a detachable keyboard and using the intel-vbnt (INT33D6)
>>>>> interface to report SW_TABLET_MODE, report 8 / "Portable" as chassis-type.
>>>>> The Dell Venue 11 Pro 7130 is an example of this.
>>>>>
>>>>> Extend the DMI chassis-type check to also accept Portables and Detachables
>>>>> so that the intel-vbtn driver will report SW_TABLET_MODE on these devices.
>>>>>
>>>>> Note the chassis-type check was originally added to avoid a false-positive
>>>>> tablet-mode report on the Dell XPS 9360 laptop. To the best of my knowledge
>>>>> that laptop is using a chassis-type of 9 / "Laptop", so after this commit
>>>>> we still ignore the tablet-switch for that chassis-type.
>>>>
>>>> Yes that's correct.
>>>>
>>>>>
>>>>> Fixes: de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode
>>>>> switch on 2-in-1's")
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>> Looking at the Microsoft Windows documentation for tablet-mode reporting:
>>>>> https://docs.microsoft.com/en-us/windows-hardware/drivers/gpiobtn/button-
>>>>> implementation
>>>>>
>>>>> Then the presence of a tablet-mode switch is indicated by the presence
>>>>> of a PNP0C60 compatible ACPI devices. There are 2 ways in which this device
>>>>> can report the tablet-mode. 1. Directly providing a GpioInt resource inside
>>>>> the PNP0C60 device, 2. Through injecting events from a Windows driver.
>>>>>
>>>>> It seems that the intel-vbtn / the INT33D6 ACPI device is the ACPI side
>>>>> of Intel's generic solution for the case where the tablet-mode comes from
>>>>> the embedded-controller and needs to be "injected".
>>>>>
>>>>> This all suggests that it might be better to replace the chassis-type
>>>>> check with a acpi_dev_present("PNP0C60", NULL, -1) check.
>>>>>
>>>>> Mario, can you provide an acpidump and alsa-info.sh output for the
>>>>> Dell XPS 9360, so that I can check if that might help with the issue
>>>>> there, and thus is a potential candidate to replace the chassis-type
>>>>> check?
>>>>
>>>> Unfortunately with WFH right now, I don't have access to a XPS 9630 to
>>>> double check the patch series.
>>>>
>>>> However I do agree this should be a good approach.
>>>
>>> Ok, so lets stick with the chassis-type check (as amended by this patch)
>>> for now then. Then once you are able to go to your office again, we
>>> can examine the acpi_dev_present("PNP0C60", NULL, -1) alternative.
>>
>> I know XPS 13's are pretty popular, perhaps someone on the mailing list who has
>> one can share ACPI dump in the interim.
> 
> https://github.com/intel/dptfxtract/issues/13

Good one.

So this has:

         Device (CIND)
         {
             Name (_HID, "INT33D3" /* Intel GPIO Buttons */)  // _HID: Hardware I
             Name (_CID, "PNP0C60" /* Display Sensor Device */)  // _CID: Compati
             Method (_STA, 0, Serialized)  // _STA: Status
             {
                 If ((OSYS >= 0x07DC))
                 {
                     Return (0x0F)
                 }

                 Return (Zero)
             }
         }

And OSYS >= 0x07DC checks for "Windows 2012" which Linux does advertise,
so despite not having a tablet-mode(switch) the XPS 9360 still has a
PNP0C60 ACPI device and will report 0xf (present) as status for it,
so a acpi_dev_present("PNP0C60", NULL, -1) check will succeed on it.

Conclusion: such a check is not a valid alternative for checking DMI
chassis-types (and from that pov this series thus is ready for merging).

Regards,

Hans