Re: [PATCH] platform: Introduce button support for the Surface 3
On Mon, Dec 5, 2016 at 5:10 PM, Benjamin Tissoires
wrote:
> The Surface 3 is not following the ACPI spec for PNP0C40, but nearly.
> The device is connected to a I2C device that might have some magic
> but we don't know about.
> Just create the device after the enumeration and use the declared GPIOs
> to provide button support.
>
> This driver is just an adaptation of drivers/input/misc/soc_button_array.c
>
> The Surface Pro 3 is using an ACPI driver and matches against the bid
> of the device ("VGBI"). To prevent this incompatible driver to be used
> on the Surface Pro, we add a match on the Surface 3 bid "TEV2".
>
> link: https://bugzilla.kernel.org/show_bug.cgi?id=102761
>
Pushed to testing with fixed Subject line (Pattern is "platform/x86:
Description").
Thanks.
> Signed-off-by: Benjamin Tissoires
> ---
>
> Hi,
>
> This driver is not in input/misc because I'd prefer keeping it closer
> to the surfacepro3_button, which is already in platform/x86.
> The Surface Pro 3 and non-Pro 3 both share the same PNPId for the button
> device
> with completely different way of handling those :/
>
> Cheers,
> Benjamin
>
> ---
> drivers/platform/x86/Kconfig | 6 +
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/surface3_button.c | 250
> +
> 3 files changed, 257 insertions(+)
> create mode 100644 drivers/platform/x86/surface3_button.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 152aac6..7bf3924 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1023,6 +1023,12 @@ config SURFACE_PRO3_BUTTON
> ---help---
> This driver handles the power/home/volume buttons on the Microsoft
> Surface Pro 3/4 tablet.
>
> +config SURFACE_3_BUTTON
> + tristate "Power/home/volume buttons driver for Microsoft Surface 3
> tablet"
> + depends on ACPI && KEYBOARD_GPIO
> + ---help---
> + This driver handles the power/home/volume buttons on the Microsoft
> Surface 3 tablet.
> +
> config INTEL_PUNIT_IPC
> tristate "Intel P-Unit IPC Driver"
> ---help---
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 38f5419..12c1482 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -67,6 +67,7 @@ obj-$(CONFIG_PVPANIC) += pvpanic.o
> obj-$(CONFIG_ALIENWARE_WMI)+= alienware-wmi.o
> obj-$(CONFIG_INTEL_PMC_IPC)+= intel_pmc_ipc.o
> obj-$(CONFIG_SURFACE_PRO3_BUTTON) += surfacepro3_button.o
> +obj-$(CONFIG_SURFACE_3_BUTTON) += surface3_button.o
> obj-$(CONFIG_INTEL_PUNIT_IPC) += intel_punit_ipc.o
> obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \
>intel_telemetry_pltdrv.o \
> diff --git a/drivers/platform/x86/surface3_button.c
> b/drivers/platform/x86/surface3_button.c
> new file mode 100644
> index 000..8bfd7f6
> --- /dev/null
> +++ b/drivers/platform/x86/surface3_button.c
> @@ -0,0 +1,250 @@
> +/*
> + * Supports for the button array on the Surface tablets.
> + *
> + * (C) Copyright 2016 Red Hat, Inc
> + *
> + * Based on soc_button_array.c:
> + *
> + * {C} Copyright 2014 Intel Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +
> +#define SURFACE_BUTTON_OBJ_NAME"TEV2"
> +#define MAX_NBUTTONS 4
> +
> +/*
> + * Some of the buttons like volume up/down are auto repeat, while others
> + * are not. To support both, we register two platform devices, and put
> + * buttons into them based on whether the key should be auto repeat.
> + */
> +#define BUTTON_TYPES 2
> +
> +/*
> + * Power button, Home button, Volume buttons support is supposed to
> + * be covered by drivers/input/misc/soc_button_array.c, which is implemented
> + * according to "Windows ACPI Design Guide for SoC Platforms".
> + * However surface 3 seems not to obey the specs, instead it uses
> + * device TEV2(MSHW0028) for declaring the GPIOs. The gpios are also slightly
> + * different in which the Home button is active high.
> + * Compared to surfacepro3_button.c which also handles MSHW0028, the Surface
> 3
> + * is a reduce platform and thus uses GPIOs, not ACPI events.
> + * We choose an I2C driver here because we need to access the resources
> + * declared under the device node, while surfacepro3_button.c only needs
> + * the ACPI companion node.
> + */
> +static const struct acpi_device_id surface3_acpi_match[] = {
> + { "MSHW0028", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(acpi, surface3_acpi_match);
> +
> +struct surface3_button_info {
> +
Re: [PATCH] platform: Introduce button support for the Surface 3
On Mon, Dec 05, 2016 at 04:10:33PM +0100, Benjamin Tissoires wrote:
> The Surface 3 is not following the ACPI spec for PNP0C40, but nearly.
> The device is connected to a I2C device that might have some magic
> but we don't know about.
> Just create the device after the enumeration and use the declared GPIOs
> to provide button support.
>
> This driver is just an adaptation of drivers/input/misc/soc_button_array.c
>
> The Surface Pro 3 is using an ACPI driver and matches against the bid
> of the device ("VGBI"). To prevent this incompatible driver to be used
> on the Surface Pro, we add a match on the Surface 3 bid "TEV2".
>
> link: https://bugzilla.kernel.org/show_bug.cgi?id=102761
>
> Signed-off-by: Benjamin Tissoires
> ---
>
> Hi,
>
> This driver is not in input/misc because I'd prefer keeping it closer
> to the surfacepro3_button, which is already in platform/x86.
> The Surface Pro 3 and non-Pro 3 both share the same PNPId for the button
> device
> with completely different way of handling those :/
Hmm, this is unfortunate that, while drivers look like twins, one is a
platform while another is i2c, but making this more generic is probably
not worth it.
Acked-by: Dmitry Torokhov
if you need it.
>
> Cheers,
> Benjamin
>
> ---
> drivers/platform/x86/Kconfig | 6 +
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/surface3_button.c | 250
> +
> 3 files changed, 257 insertions(+)
> create mode 100644 drivers/platform/x86/surface3_button.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 152aac6..7bf3924 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1023,6 +1023,12 @@ config SURFACE_PRO3_BUTTON
> ---help---
> This driver handles the power/home/volume buttons on the Microsoft
> Surface Pro 3/4 tablet.
>
> +config SURFACE_3_BUTTON
> + tristate "Power/home/volume buttons driver for Microsoft Surface 3
> tablet"
> + depends on ACPI && KEYBOARD_GPIO
> + ---help---
> + This driver handles the power/home/volume buttons on the Microsoft
> Surface 3 tablet.
> +
> config INTEL_PUNIT_IPC
> tristate "Intel P-Unit IPC Driver"
> ---help---
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 38f5419..12c1482 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -67,6 +67,7 @@ obj-$(CONFIG_PVPANIC) += pvpanic.o
> obj-$(CONFIG_ALIENWARE_WMI) += alienware-wmi.o
> obj-$(CONFIG_INTEL_PMC_IPC) += intel_pmc_ipc.o
> obj-$(CONFIG_SURFACE_PRO3_BUTTON)+= surfacepro3_button.o
> +obj-$(CONFIG_SURFACE_3_BUTTON) += surface3_button.o
> obj-$(CONFIG_INTEL_PUNIT_IPC) += intel_punit_ipc.o
> obj-$(CONFIG_INTEL_TELEMETRY)+= intel_telemetry_core.o \
> intel_telemetry_pltdrv.o \
> diff --git a/drivers/platform/x86/surface3_button.c
> b/drivers/platform/x86/surface3_button.c
> new file mode 100644
> index 000..8bfd7f6
> --- /dev/null
> +++ b/drivers/platform/x86/surface3_button.c
> @@ -0,0 +1,250 @@
> +/*
> + * Supports for the button array on the Surface tablets.
> + *
> + * (C) Copyright 2016 Red Hat, Inc
> + *
> + * Based on soc_button_array.c:
> + *
> + * {C} Copyright 2014 Intel Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +
> +#define SURFACE_BUTTON_OBJ_NAME "TEV2"
> +#define MAX_NBUTTONS 4
> +
> +/*
> + * Some of the buttons like volume up/down are auto repeat, while others
> + * are not. To support both, we register two platform devices, and put
> + * buttons into them based on whether the key should be auto repeat.
> + */
> +#define BUTTON_TYPES 2
> +
> +/*
> + * Power button, Home button, Volume buttons support is supposed to
> + * be covered by drivers/input/misc/soc_button_array.c, which is implemented
> + * according to "Windows ACPI Design Guide for SoC Platforms".
> + * However surface 3 seems not to obey the specs, instead it uses
> + * device TEV2(MSHW0028) for declaring the GPIOs. The gpios are also slightly
> + * different in which the Home button is active high.
> + * Compared to surfacepro3_button.c which also handles MSHW0028, the Surface
> 3
> + * is a reduce platform and thus uses GPIOs, not ACPI events.
> + * We choose an I2C driver here because we need to access the resources
> + * declared under the device node, while surfacepro3_button.c only needs
> + * the ACPI companion node.
> + */
> +static const struct acpi_device_id surface3_acpi_match[] = {
> + { "MSHW0028", 0 },
> +
Re: [PATCH] platform: Introduce button support for the Surface 3
On Dec 07 2016 or thereabouts, Dmitry Torokhov wrote:
> On Mon, Dec 05, 2016 at 04:10:33PM +0100, Benjamin Tissoires wrote:
> > The Surface 3 is not following the ACPI spec for PNP0C40, but nearly.
> > The device is connected to a I2C device that might have some magic
> > but we don't know about.
> > Just create the device after the enumeration and use the declared GPIOs
> > to provide button support.
> >
> > This driver is just an adaptation of drivers/input/misc/soc_button_array.c
> >
> > The Surface Pro 3 is using an ACPI driver and matches against the bid
> > of the device ("VGBI"). To prevent this incompatible driver to be used
> > on the Surface Pro, we add a match on the Surface 3 bid "TEV2".
> >
> > link: https://bugzilla.kernel.org/show_bug.cgi?id=102761
> >
> > Signed-off-by: Benjamin Tissoires
> > ---
> >
> > Hi,
> >
> > This driver is not in input/misc because I'd prefer keeping it closer
> > to the surfacepro3_button, which is already in platform/x86.
> > The Surface Pro 3 and non-Pro 3 both share the same PNPId for the button
> > device
> > with completely different way of handling those :/
>
> Hmm, this is unfortunate that, while drivers look like twins, one is a
> platform while another is i2c, but making this more generic is probably
> not worth it.
>
> Acked-by: Dmitry Torokhov
>
> if you need it.
Thanks Dmitry. In my first submission few month ago I tried to have
soc_button_array.c export part of its internal. This made this driver
much lower (basically just a static array plus some boilerplate).
However, after second thoughts, this added too much obfuscation in both
drivers and was really not worth it (because no other drivers would need
it too).
Cheers,
Benjamin
>
> >
> > Cheers,
> > Benjamin
> >
> > ---
> > drivers/platform/x86/Kconfig | 6 +
> > drivers/platform/x86/Makefile | 1 +
> > drivers/platform/x86/surface3_button.c | 250
> > +
> > 3 files changed, 257 insertions(+)
> > create mode 100644 drivers/platform/x86/surface3_button.c
> >
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 152aac6..7bf3924 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -1023,6 +1023,12 @@ config SURFACE_PRO3_BUTTON
> > ---help---
> > This driver handles the power/home/volume buttons on the Microsoft
> > Surface Pro 3/4 tablet.
> >
> > +config SURFACE_3_BUTTON
> > + tristate "Power/home/volume buttons driver for Microsoft Surface 3
> > tablet"
> > + depends on ACPI && KEYBOARD_GPIO
> > + ---help---
> > + This driver handles the power/home/volume buttons on the Microsoft
> > Surface 3 tablet.
> > +
> > config INTEL_PUNIT_IPC
> > tristate "Intel P-Unit IPC Driver"
> > ---help---
> > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> > index 38f5419..12c1482 100644
> > --- a/drivers/platform/x86/Makefile
> > +++ b/drivers/platform/x86/Makefile
> > @@ -67,6 +67,7 @@ obj-$(CONFIG_PVPANIC) += pvpanic.o
> > obj-$(CONFIG_ALIENWARE_WMI)+= alienware-wmi.o
> > obj-$(CONFIG_INTEL_PMC_IPC)+= intel_pmc_ipc.o
> > obj-$(CONFIG_SURFACE_PRO3_BUTTON) += surfacepro3_button.o
> > +obj-$(CONFIG_SURFACE_3_BUTTON) += surface3_button.o
> > obj-$(CONFIG_INTEL_PUNIT_IPC) += intel_punit_ipc.o
> > obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \
> >intel_telemetry_pltdrv.o \
> > diff --git a/drivers/platform/x86/surface3_button.c
> > b/drivers/platform/x86/surface3_button.c
> > new file mode 100644
> > index 000..8bfd7f6
> > --- /dev/null
> > +++ b/drivers/platform/x86/surface3_button.c
> > @@ -0,0 +1,250 @@
> > +/*
> > + * Supports for the button array on the Surface tablets.
> > + *
> > + * (C) Copyright 2016 Red Hat, Inc
> > + *
> > + * Based on soc_button_array.c:
> > + *
> > + * {C} Copyright 2014 Intel Corporation
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; version 2
> > + * of the License.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +
> > +#define SURFACE_BUTTON_OBJ_NAME"TEV2"
> > +#define MAX_NBUTTONS 4
> > +
> > +/*
> > + * Some of the buttons like volume up/down are auto repeat, while others
> > + * are not. To support both, we register two platform devices, and put
> > + * buttons into them based on whether the key should be auto repeat.
> > + */
> > +#define BUTTON_TYPES 2
> > +
> > +/*
> > + * Power button, Home button, Volume buttons support is supposed to
> > + * be covered by drivers/input/misc/soc_button_array.c, which is
> > implemented
> > + * according to "W
