x86: czc-tablet: add driver that fixes the buttons on CZC P10T tablet
diff mbox series

Message ID 20180721223630.24559-1-lkundrak@v3.sk
State Under Review
Delegated to: Andy Shevchenko
Headers show
Series
  • x86: czc-tablet: add driver that fixes the buttons on CZC P10T tablet
Related show

Commit Message

Lubomir Rintel July 21, 2018, 10:36 p.m. UTC
This driver switches the P10T tablet to "Android" mode, where the Home
button sends a single sancode instead of a Windows-specific key
combination and the other button doesn't disable the Wi-Fi.

The driver also supports the ViewSonic ViewPad 10 which is almost identical
to P10T.

Complementary hwdb patch with the scan code mapping:
https://github.com/systemd/systemd/commit/40a3078b.patch

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 MAINTAINERS                       |  6 +++
 drivers/platform/x86/Kconfig      |  8 ++++
 drivers/platform/x86/Makefile     |  1 +
 drivers/platform/x86/czc-tablet.c | 80 +++++++++++++++++++++++++++++++
 4 files changed, 95 insertions(+)
 create mode 100644 drivers/platform/x86/czc-tablet.c

Comments

kbuild test robot July 22, 2018, 6:36 p.m. UTC | #1
Hi Lubomir,

I love your patch! Yet something to improve:

[auto build test ERROR on sof-driver-fuweitax/master]
[also build test ERROR on v4.18-rc5 next-20180720]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Lubomir-Rintel/x86-czc-tablet-add-driver-that-fixes-the-buttons-on-CZC-P10T-tablet/20180722-064855
base:   https://github.com/fuweitax/linux master
config: x86_64-randconfig-s5-07222103 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/platform/x86/czc-tablet.c: In function 'czc_tablet_init':
>> drivers/platform/x86/czc-tablet.c:64:2: error: implicit declaration of function 'outb' [-Werror=implicit-function-declaration]
     outb(CZC_EC_ANDROID_KEYS, CZC_EC_EXTRA_PORT);
     ^~~~
   Cyclomatic Complexity 1 drivers/platform/x86/czc-tablet.c:czc_tablet_exit
   Cyclomatic Complexity 3 drivers/platform/x86/czc-tablet.c:czc_tablet_init
   Cyclomatic Complexity 1 drivers/platform/x86/czc-tablet.c:_GLOBAL__sub_D_65535_0_czc_tablet.c
   Cyclomatic Complexity 1 drivers/platform/x86/czc-tablet.c:_GLOBAL__sub_I_65535_1_czc_tablet.c
   cc1: some warnings being treated as errors

vim +/outb +64 drivers/platform/x86/czc-tablet.c

    58	
    59	static int __init czc_tablet_init(void)
    60	{
    61		if (!force && !dmi_check_system(czc_tablet_table))
    62			return -ENODEV;
    63	
  > 64		outb(CZC_EC_ANDROID_KEYS, CZC_EC_EXTRA_PORT);
    65	
    66		return 0;
    67	}
    68	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Andy Shevchenko July 22, 2018, 7:42 p.m. UTC | #2
On Sun, Jul 22, 2018 at 1:36 AM, Lubomir Rintel <lkundrak@v3.sk> wrote:
> This driver switches the P10T tablet to "Android" mode, where the Home
> button sends a single sancode instead of a Windows-specific key
> combination and the other button doesn't disable the Wi-Fi.
>
> The driver also supports the ViewSonic ViewPad 10 which is almost identical
> to P10T.
>
> Complementary hwdb patch with the scan code mapping:
> https://github.com/systemd/systemd/commit/40a3078b.patch

First of all, thanks for the driver.

While I have several comments against style, the main one is about
necessity to have this driver in the first place.

> +/*
> + * The device boots up in "Windows 7" mode, when the home button sends a
> + * Windows specific key sequence (Left Meta + D) and the second button
> + * sends an unknown one while also toggling the Radio Kill Switch.
> + * This is a surprising behavior when the second button is labeled "Back".

I'm not sure switching to Android mode is what we want. Most of the
laptops are trying to be kept compatible with Windows as much as
possible (thus, for example, we utilize Windows Management
Instrumentation for many of laptops and tablets).

> + * The vendor-supplied Android-x86 build switches the device to a "Android"
> + * mode by writing value 0x63 to the I/O port 0x68. This just seems to just
> + * set bit 6 on address 0x96 in the EC region; switching the bit directly
> + * seems to achieve the same result. It uses a "p10t_switcher" to do the
> + * job. It doesn't seem to be able to do anything else, and no other use
> + * of the port 0x68 is known.

Does it have it in ACPI as a method? How exectly it does this?

> + * In the Android mode, the home button sends just a single scancode,
> + * which can be handled in Linux userspace more reasonably and the back
> + * button only sends a scancode without toggling the kill switch.
> + * The scancode can then be mapped either to Back or RF Kill functionality
> + * in userspace, depending on how the button is labeled on that particular
> + * model.
> + */

> +       outb(CZC_EC_ANDROID_KEYS, CZC_EC_EXTRA_PORT);

This is basically "the driver". The question here, what prevents
userspace to have few liner C-code to perform the same. IOW, why this
is necessary to have in kernel space?
Lubomir Rintel July 30, 2018, 5:20 a.m. UTC | #3
Hi.

Many thanks for the response! Some replies inline.

On Sun, 2018-07-22 at 22:42 +0300, Andy Shevchenko wrote:
> On Sun, Jul 22, 2018 at 1:36 AM, Lubomir Rintel <lkundrak@v3.sk>
> wrote:
> > This driver switches the P10T tablet to "Android" mode, where the
> > Home
> > button sends a single sancode instead of a Windows-specific key
> > combination and the other button doesn't disable the Wi-Fi.
> >
> > The driver also supports the ViewSonic ViewPad 10 which is almost
> > identical
> > to P10T.
> >
> > Complementary hwdb patch with the scan code mapping:
> > https://github.com/systemd/systemd/commit/40a3078b.patch
>
> First of all, thanks for the driver.
>
> While I have several comments against style, the main one is about
> necessity to have this driver in the first place.
>
> > +/*
> > + * The device boots up in "Windows 7" mode, when the home button
> > sends a
> > + * Windows specific key sequence (Left Meta + D) and the second
> > button
> > + * sends an unknown one while also toggling the Radio Kill Switch.
> > + * This is a surprising behavior when the second button is labeled
> > "Back".
>
> I'm not sure switching to Android mode is what we want. Most of the
> laptops are trying to be kept compatible with Windows as much as
> possible (thus, for example, we utilize Windows Management
> Instrumentation for many of laptops and tablets).

Yes. On the other hand, this one is not really a laptop. It's a tablet
and Windows doesn't seem to be that well supported first-class citizen
here.

The versions of the tablet that ship with Windows seem to have the back
button labeled with a Wi-Fi button (Source: Internet) and the manual
states that the button is indeed used to disable wireless on those
models. This behavior is too confusing on the Android models.

> > + * The vendor-supplied Android-x86 build switches the device to a
> > "Android"
> > + * mode by writing value 0x63 to the I/O port 0x68. This just
> > seems to just
> > + * set bit 6 on address 0x96 in the EC region; switching the bit
> > directly
> > + * seems to achieve the same result. It uses a "p10t_switcher" to
> > do the
> > + * job. It doesn't seem to be able to do anything else, and no
> > other use
> > + * of the port 0x68 is known.
>
> Does it have it in ACPI as a method?

No. The relevant bit doesn't even have a name in the EmbeddedControl
region. I've done some poking around with the nearby bits of the EC,
but none of them seem to do anything relevant. (The very next one seems
to turn the key to enter though, it seems to be used by the AWY0001
so that the button could be used for wake from away mode or something...)

> How exectly it does this?

Via /dev/port.

> > + * In the Android mode, the home button sends just a single
> > scancode,
> > + * which can be handled in Linux userspace more reasonably and the
> > back
> > + * button only sends a scancode without toggling the kill switch.
> > + * The scancode can then be mapped either to Back or RF Kill
> > functionality
> > + * in userspace, depending on how the button is labeled on that
> > particular
> > + * model.
> > + */
> > +       outb(CZC_EC_ANDROID_KEYS, CZC_EC_EXTRA_PORT);
>
> This is basically "the driver". The question here, what prevents
> userspace to have few liner C-code to perform the same. IOW, why this
> is necessary to have in kernel space?

Yes. That, and the DMI match. What I'm concerned about is for any stock
Linux distro to work well on these machines. The current behavior is much
too frustrating.

I don't feel too well about a separate driver for this, but I had a
difficult time finding the right spot to do the switch. I thought udev
could do the match, but it doesn't seem to be the right place to access
hardware directly.

Perhaps it's not that terrible to have a separate driver for this.
It's easily disabled and doesn't do anything for people that don't have the
hardware.

Thank you
Lubo

Patch
diff mbox series

diff --git a/MAINTAINERS b/MAINTAINERS
index 07d1576fc766..64bd3f330f85 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3979,6 +3979,12 @@  S:	Supported
 F:	drivers/input/touchscreen/cyttsp*
 F:	include/linux/input/cyttsp.h
 
+CZC TABLET DRIVER
+M:	Lubomir Rintel <lkundrak@v3.sk>
+L:	platform-driver-x86@vger.kernel.org
+S:	Maintained
+F:	drivers/platform/x86/czc-tablet.c
+
 D-LINK DIR-685 TOUCHKEYS DRIVER
 M:	Linus Walleij <linus.walleij@linaro.org>
 L:	linux-input@vger.kernel.org
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index ac4d48830415..4ec8576129dc 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1218,6 +1218,14 @@  config INTEL_CHTDC_TI_PWRBTN
 	  To compile this driver as a module, choose M here: the module
 	  will be called intel_chtdc_ti_pwrbtn.
 
+config CZC_TABLET
+        tristate "CZC Tablet Extras"
+        help
+          This driver adds support for buttons on a CZC ODEON TPC-10 ("P10T")
+          and ViewSonic ViewPad 10 tablet computers.
+
+	  If you have an 10" CZC or ViewSonic tablet, say Y or M here.
+
 endif # X86_PLATFORM_DEVICES
 
 config PMC_ATOM
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 2ba6cb795338..5cb772bbbfb6 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -47,6 +47,7 @@  obj-$(CONFIG_SURFACE3_WMI)	+= surface3-wmi.o
 obj-$(CONFIG_TOPSTAR_LAPTOP)	+= topstar-laptop.o
 obj-$(CONFIG_WMI_BMOF)		+= wmi-bmof.o
 obj-$(CONFIG_INTEL_WMI_THUNDERBOLT)	+= intel-wmi-thunderbolt.o
+obj-$(CONFIG_CZC_TABLET)	+= czc-tablet.o
 
 # toshiba_acpi must link after wmi to ensure that wmi devices are found
 # before toshiba_acpi initializes
diff --git a/drivers/platform/x86/czc-tablet.c b/drivers/platform/x86/czc-tablet.c
new file mode 100644
index 000000000000..8ff94c4bfe43
--- /dev/null
+++ b/drivers/platform/x86/czc-tablet.c
@@ -0,0 +1,80 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * CZC Tablet Support
+ *
+ * Copyright (C) 2018 Lubomir Rintel <lkundrak@v3.sk>
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/dmi.h>
+
+static bool force;
+module_param(force, bool, 0);
+MODULE_PARM_DESC(force, "Disable the DMI check and force the driver to be loaded");
+
+/*
+ * The device boots up in "Windows 7" mode, when the home button sends a
+ * Windows specific key sequence (Left Meta + D) and the second button
+ * sends an unknown one while also toggling the Radio Kill Switch.
+ * This is a surprising behavior when the second button is labeled "Back".
+ *
+ * The vendor-supplied Android-x86 build switches the device to a "Android"
+ * mode by writing value 0x63 to the I/O port 0x68. This just seems to just
+ * set bit 6 on address 0x96 in the EC region; switching the bit directly
+ * seems to achieve the same result. It uses a "p10t_switcher" to do the
+ * job. It doesn't seem to be able to do anything else, and no other use
+ * of the port 0x68 is known.
+ *
+ * In the Android mode, the home button sends just a single scancode,
+ * which can be handled in Linux userspace more reasonably and the back
+ * button only sends a scancode without toggling the kill switch.
+ * The scancode can then be mapped either to Back or RF Kill functionality
+ * in userspace, depending on how the button is labeled on that particular
+ * model.
+ */
+
+#define CZC_EC_EXTRA_PORT   0x68
+
+#define CZC_EC_ANDROID_KEYS 0x63
+
+static const struct dmi_system_id czc_tablet_table[] __initconst = {
+	{
+		.ident = "CZC ODEON TPC-10 (\"P10T\")",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "CZC"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "ODEON*TPC-10"),
+		},
+	},
+	{
+		.ident = "ViewSonic ViewPad 10",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "ViewSonic"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "VPAD10"),
+		},
+	},
+	{ }
+};
+
+static int __init czc_tablet_init(void)
+{
+	if (!force && !dmi_check_system(czc_tablet_table))
+		return -ENODEV;
+
+	outb(CZC_EC_ANDROID_KEYS, CZC_EC_EXTRA_PORT);
+
+	return 0;
+}
+
+static void __exit czc_tablet_exit(void)
+{
+}
+
+module_init(czc_tablet_init);
+module_exit(czc_tablet_exit);
+
+MODULE_DEVICE_TABLE(dmi, czc_tablet_table);
+
+MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>");
+MODULE_DESCRIPTION("CZC Tablet Support");
+MODULE_LICENSE("GPL");