| Message ID | 20180721223630.24559-1-lkundrak@v3.sk (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
| Series | x86: czc-tablet: add driver that fixes the buttons on CZC P10T tablet | expand |
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
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?
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
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");
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