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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
> -----Original Message-----
> From: Hans de Goede <[email protected]>
> Sent: Saturday, May 2, 2020 1:30 PM
> To: Darren Hart; Andy Shevchenko; Limonciello, Mario
> Cc: Hans de Goede; [email protected]; platform-driver-
> [email protected]; [email protected]
> 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.
> -----Original Message-----
> From: Hans de Goede <[email protected]>
> Sent: Saturday, May 2, 2020 1:30 PM
> To: Darren Hart; Andy Shevchenko; Limonciello, Mario
> Cc: Hans de Goede; [email protected]; platform-driver-
> [email protected]; [email protected]
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> ---
> 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
Hi,
On 5/4/20 5:37 PM, [email protected] wrote:
>
>
>> -----Original Message-----
>> From: Hans de Goede <[email protected]>
>> Sent: Saturday, May 2, 2020 1:30 PM
>> To: Darren Hart; Andy Shevchenko; Limonciello, Mario
>> Cc: Hans de Goede; [email protected]; platform-driver-
>> [email protected]; [email protected]
>> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
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
>
> -----Original Message-----
> From: [email protected] <platform-driver-x86-
> [email protected]> On Behalf Of Hans de Goede
> Sent: Tuesday, May 5, 2020 4:06 AM
> To: Limonciello, Mario; [email protected]; [email protected]
> Cc: [email protected]; [email protected]; linux-
> [email protected]
> 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, [email protected] wrote:
> >
> >
> >> -----Original Message-----
> >> From: Hans de Goede <[email protected]>
> >> Sent: Saturday, May 2, 2020 1:30 PM
> >> To: Darren Hart; Andy Shevchenko; Limonciello, Mario
> >> Cc: Hans de Goede; [email protected]; platform-driver-
> >> [email protected]; [email protected]
> >> 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 <[email protected]>
> >> ---
> >> 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 <[email protected]>
>
> 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
> >
On Tue, May 5, 2020 at 5:22 PM <[email protected]> wrote:
>
> > -----Original Message-----
> > From: [email protected] <platform-driver-x86-
> > [email protected]> On Behalf Of Hans de Goede
> > Sent: Tuesday, May 5, 2020 4:06 AM
> > To: Limonciello, Mario; [email protected]; [email protected]
> > Cc: [email protected]; [email protected]; linux-
> > [email protected]
> > 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, [email protected] wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Hans de Goede <[email protected]>
> > >> Sent: Saturday, May 2, 2020 1:30 PM
> > >> To: Darren Hart; Andy Shevchenko; Limonciello, Mario
> > >> Cc: Hans de Goede; [email protected]; platform-driver-
> > >> [email protected]; [email protected]
> > >> 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 <[email protected]>
> > >> ---
> > >> 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
Hi,
On 5/5/20 4:27 PM, Andy Shevchenko wrote:
> On Tue, May 5, 2020 at 5:22 PM <[email protected]> wrote:
>>
>>> -----Original Message-----
>>> From: [email protected] <platform-driver-x86-
>>> [email protected]> On Behalf Of Hans de Goede
>>> Sent: Tuesday, May 5, 2020 4:06 AM
>>> To: Limonciello, Mario; [email protected]; [email protected]
>>> Cc: [email protected]; [email protected]; linux-
>>> [email protected]
>>> 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, [email protected] wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Hans de Goede <[email protected]>
>>>>> Sent: Saturday, May 2, 2020 1:30 PM
>>>>> To: Darren Hart; Andy Shevchenko; Limonciello, Mario
>>>>> Cc: Hans de Goede; [email protected]; platform-driver-
>>>>> [email protected]; [email protected]
>>>>> 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 <[email protected]>
>>>>> ---
>>>>> 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