Bug 198507 - Unhandled events for Dell inspirot tablet mode
Summary: Unhandled events for Dell inspirot tablet mode
Status: RESOLVED CODE_FIX
Alias: None
Product: Drivers
Classification: Unclassified
Component: Platform_x86 (show other bugs)
Hardware: All Linux
: P2 normal
Assignee: Hans de Goede
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-01-18 15:53 UTC by Marco Martin
Modified: 2020-10-27 14:52 UTC (History)
4 users (show)

See Also:
Kernel Version: 4.13
Subsystem:
Regression: No
Bisected commit-id:


Attachments
DSDT of the inspiron 13-7352 (82.40 KB, application/octet-stream)
2018-01-18 15:53 UTC, Marco Martin
Details
possible patch (2.34 KB, application/mbox)
2018-01-18 15:55 UTC, Marco Martin
Details
query of the initial state (2.86 KB, application/mbox)
2018-01-19 15:04 UTC, Marco Martin
Details

Description Marco Martin 2018-01-18 15:53:38 UTC
Created attachment 273685 [details]
DSDT of the inspiron 13-7352

Reproduced on a Dell Inspiron 13-7352 and an Inspiron 13-7000,
when entering the tablet mode (flipping the screen) an unrecognized event arrives from the intel-vbtn device driver, dmesg output:
intel-vbtn INT33D6:00: unknown event index 0xcc

when exiting from tablet mode:
intel-vbtn INT33D6:00: unknown event index 0xcd
Comment 1 Marco Martin 2018-01-18 15:55:04 UTC
Created attachment 273687 [details]
possible patch

the attached patch does solve the issue even tough intel-vbtn feels the wrong place for a dell specific thing
Comment 2 Mario Limonciello 2018-01-18 19:35:46 UTC
Reviewing the DSDT I don't think that the 0xCC / 0xCD events are likely Dell specific.  The events for switching modes come from the Dell EC (which that part of the implementation are Dell specific).

This is not a Dell driver that works with this INT33D6 interface on Windows, it's an Intel driver.

So after looking this through I think your patch is on the right track, but I'd recommend a few changes.

1) Don't generically set the capability for tablet mode during initialization.
I'm not sure if it's possible to query in advance if tablet mode will be supported.  It might be encoded in a bit for the return of method VGBS.
It should be set when the first 0xCC or 0xCD event comes through. 

2) To initialize the right value of this (tablet mode or not) I believe you can check for the existence of VGBS method and look at it's return.  You can see in _Q1E and _Q1F that UPBT method is called which updates the value of VDBS and can be queried from VGBS

3) I think the looking at the two special tablet cases 0xCC/0xCD separate from sparse_keymap_report is fine, but please add some sort of #define so it's clear what 0xCC means and what 0xCD means.
Comment 3 Marco Martin 2018-01-19 15:04:11 UTC
Created attachment 273727 [details]
query of the initial state

The attached patch follows your advices (if i understood them correctly:)
now the initial state is initialized correctly, I tried to start in both modes. Tough I don't have a machine which has intel-vbtn and not a tablet mode switch to test that case to be handled correctly.
Comment 4 Mario Limonciello 2018-01-19 15:55:56 UTC
Overall looking better.

Minor feedback:
1) When looking at this I found that you might be able to use KE_VSW and then you wouldn't have to special case the handling (sparse key map should handle KE_VSW properly if you put the entries in the keymap table).
Having an #define might not be as important when you change this. A comment in sparse keymap entry would be sufficient to distinguish which ones are entry and which are exit.

2) You have a warning to fix.
drivers/platform/x86/intel-vbtn.c: In function ‘intel_vbtn_probe’:
drivers/platform/x86/intel-vbtn.c:132:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
   union acpi_object *obj = vgbs_output.pointer;

3) Use ACPI_SUCCESS(status) rather than !ACPI_FAILURE(status)
4) You should put the input_report switch into a larger block that only runs if ACPI_TYPE_INTEGER.  In case VGBS returns wrong type for a machine.
For example like this:

diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
index 6b38ade..487b301 100644
--- a/drivers/platform/x86/intel-vbtn.c
+++ b/drivers/platform/x86/intel-vbtn.c
@@ -130,11 +130,12 @@ static int intel_vbtn_probe(struct platform_device *device)
        if (!ACPI_FAILURE(status)) {
                union acpi_object *obj = vgbs_output.pointer;
                input_set_capability(priv->input_dev, EV_SW, SW_TABLET_MODE);
-               if (obj && obj->type == ACPI_TYPE_INTEGER && (obj->integer.value & TABLET_MODE_FLAG))
-                       input_report_switch(priv->input_dev, SW_TABLET_MODE, 0);
-               else
-                       input_report_switch(priv->input_dev, SW_TABLET_MODE, 1);
-               
+               if (obj && obj->type == ACPI_TYPE_INTEGER) {
+                       if (obj->integer.value & TABLET_MODE_FLAG)
+                               input_report_switch(priv->input_dev, SW_TABLET_MODE, 0);
+                       else
+                               input_report_switch(priv->input_dev, SW_TABLET_MODE, 1);
+               }
        }
 
        status = acpi_install_notify_handler(handle,

5) Patch appears to have trailing whitespace.

$ git am ~/Public/0001-Support-intel-vbtn-based-tablet-mode-switch.patch
Applying: Support intel-vbtn based tablet mode switch
.git/rebase-apply/patch:58: trailing whitespace.

warning: 1 line adds whitespace errors.

6) Checkpatch shows patch to not be clean, clean those up.


I checked the HW I have, XPS 9350 and XPS 9370 don't use vbtn, but XPS 9365 does.
It does report tablet mode properly with your patch too.

Once you clean up the stuff above I think you should resubmit this to mailing list.
Comment 5 Hans de Goede 2020-10-27 14:52:53 UTC
In the mean time a similar patch has already been merged upstream:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1c828496228de

So this bug can be closed now, closing.

Note You need to log in before you can comment on or make changes to this bug.