linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Support for buttons on newer MS Surface devices
@ 2019-07-02  0:37 Maximilian Luz
  2019-07-02  0:37 ` [PATCH v2 1/2] platform: Fix device check for surfacepro3_button Maximilian Luz
                   ` (3 more replies)
  0 siblings, 4 replies; 20+ messages in thread
From: Maximilian Luz @ 2019-07-02  0:37 UTC (permalink / raw)
  Cc: linux-kernel, linux-input, platform-driver-x86, Dmitry Torokhov,
	Hans de Goede, Chen Yu, Darren Hart, Andy Shevchenko,
	Benjamin Tissoires, Maximilian Luz

This series adds suport for power and volume buttons on 5th and 6th
generation Microsoft Surface devices. Specifically, it adds support for
the power-button on the Surface Laptop 1 and Laptop 2, as well as
support for power- and (on-device) volume-buttons on the Surface Pro 5
(2017), Pro 6, and Book 2.

These devices use the same MSHW0040 device as on the Surface Pro 4,
however, whereas the Pro 4 uses an ACPI notify handler, the newer
devices use GPIO interrupts to signal these events.

The first patch of this series ensures that the surfacepro3_button
driver, used for MSHW0040 on the Pro 4, does not probe for the newer
devices. The second patch adapts soc_button_array to implement the
actual button support.

I think the changes to soc_button_array in the second patch warrant a
thorough review. I've tried to make things a bit more generic to be able
to integrate arbitrary ACPI GPIO power-/volume-button devices more
easily, I'm not sure if there may be reasons against this.

These patches have also been tested on various Surface devices via the
github.com/jakeday/linux-surface patchset.

Changes since v1:
  - [PATCH 1/2] platform: Fix device check for surfacepro3_button
    No changes.

  - [PATCH 2/2] input: soc_button_array for newer surface devices
    Ensure the patch compiles without CONFIG_ACPI.

Maximilian Luz (2):
  platform: Fix device check for surfacepro3_button
  input: soc_button_array for newer surface devices

 drivers/input/misc/soc_button_array.c     | 145 ++++++++++++++++++++--
 drivers/platform/x86/surfacepro3_button.c |  38 ++++++
 2 files changed, 171 insertions(+), 12 deletions(-)

-- 
2.22.0


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2 1/2] platform: Fix device check for surfacepro3_button
  2019-07-02  0:37 [PATCH 0/2] Support for buttons on newer MS Surface devices Maximilian Luz
@ 2019-07-02  0:37 ` Maximilian Luz
  2019-07-02  1:14   ` Yu Chen
  2019-07-18 17:43   ` Andy Shevchenko
  2019-07-02  0:37 ` [PATCH v2 2/2] input: soc_button_array for newer surface devices Maximilian Luz
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 20+ messages in thread
From: Maximilian Luz @ 2019-07-02  0:37 UTC (permalink / raw)
  Cc: linux-kernel, linux-input, platform-driver-x86, Dmitry Torokhov,
	Hans de Goede, Chen Yu, Darren Hart, Andy Shevchenko,
	Benjamin Tissoires, Maximilian Luz

Do not use the surfacepro3_button driver on newer Microsoft Surface
models, only use it on the Surface Pro 3 and 4. Newer models (5th, 6th
and possibly future generations) use the same device as the Surface Pro
4 to represent their volume and power buttons (MSHW0040), but their
acutal implementation is significantly different. This patch ensures
that the surfacepro3_button driver is only used on the Pro 3 and 4
models, allowing a different driver to bind on other models.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---
 drivers/platform/x86/surfacepro3_button.c | 38 +++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/platform/x86/surfacepro3_button.c b/drivers/platform/x86/surfacepro3_button.c
index 47c6d000465a..0e2c7dfafd9f 100644
--- a/drivers/platform/x86/surfacepro3_button.c
+++ b/drivers/platform/x86/surfacepro3_button.c
@@ -20,6 +20,12 @@
 #define SURFACE_BUTTON_OBJ_NAME		"VGBI"
 #define SURFACE_BUTTON_DEVICE_NAME	"Surface Pro 3/4 Buttons"
 
+#define MSHW0040_DSM_REVISION		0x01
+#define MSHW0040_DSM_GET_OMPR		0x02	// get OEM Platform Revision
+static const guid_t MSHW0040_DSM_UUID =
+	GUID_INIT(0x6fd05c69, 0xcde3, 0x49f4, 0x95, 0xed, 0xab, 0x16, 0x65,
+		  0x49, 0x80, 0x35);
+
 #define SURFACE_BUTTON_NOTIFY_TABLET_MODE	0xc8
 
 #define SURFACE_BUTTON_NOTIFY_PRESS_POWER	0xc6
@@ -142,6 +148,34 @@ static int surface_button_resume(struct device *dev)
 }
 #endif
 
+/*
+ * Surface Pro 4 and Surface Book 2 / Surface Pro 2017 use the same device
+ * ID (MSHW0040) for the power/volume buttons. Make sure this is the right
+ * device by checking for the _DSM method and OEM Platform Revision.
+ */
+static int surface_button_check_MSHW0040(struct acpi_device *dev)
+{
+	acpi_handle handle = dev->handle;
+	union acpi_object *result;
+	u64 oem_platform_rev = 0;
+
+	// get OEM platform revision
+	result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID,
+					 MSHW0040_DSM_REVISION,
+					 MSHW0040_DSM_GET_OMPR,
+					 NULL, ACPI_TYPE_INTEGER);
+
+	if (result) {
+		oem_platform_rev = result->integer.value;
+		ACPI_FREE(result);
+	}
+
+	dev_dbg(&dev->dev, "OEM Platform Revision %llu\n", oem_platform_rev);
+
+	return oem_platform_rev == 0 ? 0 : -ENODEV;
+}
+
+
 static int surface_button_add(struct acpi_device *device)
 {
 	struct surface_button *button;
@@ -154,6 +188,10 @@ static int surface_button_add(struct acpi_device *device)
 	    strlen(SURFACE_BUTTON_OBJ_NAME)))
 		return -ENODEV;
 
+	error = surface_button_check_MSHW0040(device);
+	if (error)
+		return error;
+
 	button = kzalloc(sizeof(struct surface_button), GFP_KERNEL);
 	if (!button)
 		return -ENOMEM;
-- 
2.22.0


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2 2/2] input: soc_button_array for newer surface devices
  2019-07-02  0:37 [PATCH 0/2] Support for buttons on newer MS Surface devices Maximilian Luz
  2019-07-02  0:37 ` [PATCH v2 1/2] platform: Fix device check for surfacepro3_button Maximilian Luz
@ 2019-07-02  0:37 ` Maximilian Luz
  2019-07-04 15:31   ` Maximilian Luz
  2019-07-16  7:21   ` Dmitry Torokhov
  2019-07-02 17:13 ` [PATCH 0/2] Support for buttons on newer MS Surface devices Andy Shevchenko
  2019-07-20 15:15 ` Maximilian Luz
  3 siblings, 2 replies; 20+ messages in thread
From: Maximilian Luz @ 2019-07-02  0:37 UTC (permalink / raw)
  Cc: linux-kernel, linux-input, platform-driver-x86, Dmitry Torokhov,
	Hans de Goede, Chen Yu, Darren Hart, Andy Shevchenko,
	Benjamin Tissoires, Maximilian Luz

Power and volume button support for 5th and 6th genration Microsoft
Surface devices via soc_button_array.

Note that these devices use the same MSHW0040 device as on the Surface
Pro 4, however the implementation is different (GPIOs vs. ACPI
notifications). Thus some checking is required to ensure we only load
this driver on the correct devices.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---
 drivers/input/misc/soc_button_array.c | 145 +++++++++++++++++++++++---
 1 file changed, 133 insertions(+), 12 deletions(-)

diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
index 5e59f8e57f8e..8bd6909f8d1f 100644
--- a/drivers/input/misc/soc_button_array.c
+++ b/drivers/input/misc/soc_button_array.c
@@ -25,6 +25,17 @@ struct soc_button_info {
 	bool wakeup;
 };
 
+/**
+ * struct soc_device_data - driver data for different device types
+ * @button_info: specifications of buttons, if NULL specification is assumed to
+ *               be present in _DSD
+ * @check: device-specific check (NULL means all will be accepted)
+ */
+struct soc_device_data {
+	struct soc_button_info *button_info;
+	int (*check)(struct device *dev);
+};
+
 /*
  * 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
@@ -310,6 +321,7 @@ static int soc_button_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	const struct acpi_device_id *id;
+	struct soc_device_data *device_data;
 	struct soc_button_info *button_info;
 	struct soc_button_data *priv;
 	struct platform_device *pd;
@@ -320,18 +332,20 @@ static int soc_button_probe(struct platform_device *pdev)
 	if (!id)
 		return -ENODEV;
 
-	if (!id->driver_data) {
+	device_data = (struct soc_device_data *)id->driver_data;
+	if (device_data && device_data->check) {
+		error = device_data->check(dev);
+		if (error)
+			return error;
+	}
+
+	if (device_data && device_data->button_info) {
+		button_info = (struct soc_button_info *)
+				device_data->button_info;
+	} else {
 		button_info = soc_button_get_button_info(dev);
 		if (IS_ERR(button_info))
 			return PTR_ERR(button_info);
-	} else {
-		button_info = (struct soc_button_info *)id->driver_data;
-	}
-
-	error = gpiod_count(dev, NULL);
-	if (error < 0) {
-		dev_dbg(dev, "no GPIO attached, ignoring...\n");
-		return -ENODEV;
 	}
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -357,12 +371,32 @@ static int soc_button_probe(struct platform_device *pdev)
 	if (!priv->children[0] && !priv->children[1])
 		return -ENODEV;
 
-	if (!id->driver_data)
+	if (!device_data || !device_data->button_info)
 		devm_kfree(dev, button_info);
 
 	return 0;
 }
 
+
+static int soc_device_check_generic(struct device *dev)
+{
+	int gpios;
+
+	gpios = gpiod_count(dev, NULL);
+	if (gpios < 0) {
+		dev_dbg(dev, "no GPIO attached, ignoring...\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static struct soc_device_data soc_device_ACPI0011 = {
+	.button_info = NULL,
+	.check = soc_device_check_generic,
+};
+
+
 /*
  * Definition of buttons on the tablet. The ACPI index of each button
  * is defined in section 2.8.7.2 of "Windows ACPI Design Guide for SoC
@@ -377,9 +411,96 @@ static struct soc_button_info soc_button_PNP0C40[] = {
 	{ }
 };
 
+static struct soc_device_data soc_device_PNP0C40 = {
+	.button_info = soc_button_PNP0C40,
+	.check = soc_device_check_generic,
+};
+
+
+/*
+ * Special device check for Surface Book 2 and Surface Pro (2017).
+ * Both, the Surface Pro 4 (surfacepro3_button.c) and the above mentioned
+ * devices use MSHW0040 for power and volume buttons, however the way they
+ * have to be addressed differs. Make sure that we only load this drivers
+ * for the correct devices by checking the OEM Platform Revision provided by
+ * the _DSM method.
+ */
+#define MSHW0040_DSM_REVISION		0x01
+#define MSHW0040_DSM_GET_OMPR		0x02	// get OEM Platform Revision
+static const guid_t MSHW0040_DSM_UUID =
+	GUID_INIT(0x6fd05c69, 0xcde3, 0x49f4, 0x95, 0xed, 0xab, 0x16, 0x65,
+		  0x49, 0x80, 0x35);
+
+#ifdef CONFIG_ACPI
+
+static int soc_device_check_MSHW0040(struct device *dev)
+{
+	acpi_handle handle = ACPI_HANDLE(dev);
+	union acpi_object *result;
+	u64 oem_platform_rev = 0;
+	int gpios;
+
+	// get OEM platform revision
+	result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID,
+					 MSHW0040_DSM_REVISION,
+					 MSHW0040_DSM_GET_OMPR, NULL,
+					 ACPI_TYPE_INTEGER);
+
+	if (result) {
+		oem_platform_rev = result->integer.value;
+		ACPI_FREE(result);
+	}
+
+	if (oem_platform_rev == 0)
+		return -ENODEV;
+
+	dev_dbg(dev, "OEM Platform Revision %llu\n", oem_platform_rev);
+
+	/*
+	 * We are _really_ expecting GPIOs here. If we do not get any, this
+	 * means the GPIO driver has not been loaded yet (which can happen).
+	 * Try again later.
+	 */
+	gpios = gpiod_count(dev, NULL);
+	if (gpios < 0)
+		return -EAGAIN;
+
+	return 0;
+}
+
+#else   /* CONFIG_ACPI */
+
+static int soc_device_check_MSHW0040(struct device *dev)
+{
+    return -ENODEV;
+}
+
+#endif  /* CONFIG_ACPI */
+
+/*
+ * Button infos for Microsoft Surface Book 2 and Surface Pro (2017).
+ * Obtained from DSDT/testing.
+ */
+static struct soc_button_info soc_button_MSHW0040[] = {
+	{ "power", 0, EV_KEY, KEY_POWER, false, true },
+	{ "volume_up", 2, EV_KEY, KEY_VOLUMEUP, true, false },
+	{ "volume_down", 4, EV_KEY, KEY_VOLUMEDOWN, true, false },
+	{ }
+};
+
+static struct soc_device_data soc_device_MSHW0040 = {
+	.button_info = soc_button_MSHW0040,
+	.check = soc_device_check_MSHW0040,
+};
+
+
 static const struct acpi_device_id soc_button_acpi_match[] = {
-	{ "PNP0C40", (unsigned long)soc_button_PNP0C40 },
-	{ "ACPI0011", 0 },
+	{ "PNP0C40", (unsigned long)&soc_device_PNP0C40 },
+	{ "ACPI0011", (unsigned long)&soc_device_ACPI0011 },
+
+	/* Microsoft Surface Devices (5th and 6th generation) */
+	{ "MSHW0040", (unsigned long)&soc_device_MSHW0040 },
+
 	{ }
 };
 
-- 
2.22.0


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] platform: Fix device check for surfacepro3_button
  2019-07-02  0:37 ` [PATCH v2 1/2] platform: Fix device check for surfacepro3_button Maximilian Luz
@ 2019-07-02  1:14   ` Yu Chen
  2019-07-02  1:25     ` Maximilian Luz
  2019-07-18 17:43   ` Andy Shevchenko
  1 sibling, 1 reply; 20+ messages in thread
From: Yu Chen @ 2019-07-02  1:14 UTC (permalink / raw)
  To: Maximilian Luz
  Cc: linux-kernel, linux-input, platform-driver-x86, Dmitry Torokhov,
	Hans de Goede, Darren Hart, Andy Shevchenko, Benjamin Tissoires

Hi,
On Tue, Jul 02, 2019 at 02:37:39AM +0200, Maximilian Luz wrote:
> Do not use the surfacepro3_button driver on newer Microsoft Surface
> models, only use it on the Surface Pro 3 and 4. Newer models (5th, 6th
> and possibly future generations) use the same device as the Surface Pro
> 4 to represent their volume and power buttons (MSHW0040), but their
> acutal implementation is significantly different. This patch ensures
> that the surfacepro3_button driver is only used on the Pro 3 and 4
> models, allowing a different driver to bind on other models.
>
This method overall looks ok to me.
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> ---
>  drivers/platform/x86/surfacepro3_button.c | 38 +++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/platform/x86/surfacepro3_button.c b/drivers/platform/x86/surfacepro3_button.c
> index 47c6d000465a..0e2c7dfafd9f 100644
> --- a/drivers/platform/x86/surfacepro3_button.c
> +++ b/drivers/platform/x86/surfacepro3_button.c
> @@ -20,6 +20,12 @@
>  #define SURFACE_BUTTON_OBJ_NAME		"VGBI"
>  #define SURFACE_BUTTON_DEVICE_NAME	"Surface Pro 3/4 Buttons"
>  
> +#define MSHW0040_DSM_REVISION		0x01
> +#define MSHW0040_DSM_GET_OMPR		0x02	// get OEM Platform Revision
> +static const guid_t MSHW0040_DSM_UUID =
> +	GUID_INIT(0x6fd05c69, 0xcde3, 0x49f4, 0x95, 0xed, 0xab, 0x16, 0x65,
> +		  0x49, 0x80, 0x35);
> +
>  #define SURFACE_BUTTON_NOTIFY_TABLET_MODE	0xc8
>  
>  #define SURFACE_BUTTON_NOTIFY_PRESS_POWER	0xc6
> @@ -142,6 +148,34 @@ static int surface_button_resume(struct device *dev)
>  }
>  #endif
>  
> +/*
> + * Surface Pro 4 and Surface Book 2 / Surface Pro 2017 use the same device
> + * ID (MSHW0040) for the power/volume buttons. Make sure this is the right
> + * device by checking for the _DSM method and OEM Platform Revision.
> + */
> +static int surface_button_check_MSHW0040(struct acpi_device *dev)
> +{
> +	acpi_handle handle = dev->handle;
> +	union acpi_object *result;
> +	u64 oem_platform_rev = 0;
> +
> +	// get OEM platform revision
> +	result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID,
> +					 MSHW0040_DSM_REVISION,
> +					 MSHW0040_DSM_GET_OMPR,
> +					 NULL, ACPI_TYPE_INTEGER);
> +
Does it mean, only 5th, 6th and newer platforms have OEM platform revision?
3rd/4th will get NULL result? Or the opposite?
> +	if (result) {
> +		oem_platform_rev = result->integer.value;
> +		ACPI_FREE(result);
> +	}
> +
> +	dev_dbg(&dev->dev, "OEM Platform Revision %llu\n", oem_platform_rev);
> +
> +	return oem_platform_rev == 0 ? 0 : -ENODEV;
if 3rd/4th do not have this oem rev information while 5th/newer have,
why the latter returns NODEV(it actually has this info)?
> +}
> +
> +
>  static int surface_button_add(struct acpi_device *device)
>  {
>  	struct surface_button *button;
> @@ -154,6 +188,10 @@ static int surface_button_add(struct acpi_device *device)
>  	    strlen(SURFACE_BUTTON_OBJ_NAME)))
>  		return -ENODEV;
>  
> +	error = surface_button_check_MSHW0040(device);
> +	if (error)
> +		return error;
> +
ditto, 3rd/4th get error=0?
>  	button = kzalloc(sizeof(struct surface_button), GFP_KERNEL);
>  	if (!button)
>  		return -ENOMEM;
> -- 
> 2.22.0
> 
Best,
Yu

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] platform: Fix device check for surfacepro3_button
  2019-07-02  1:14   ` Yu Chen
@ 2019-07-02  1:25     ` Maximilian Luz
  2019-07-02  1:33       ` Maximilian Luz
  0 siblings, 1 reply; 20+ messages in thread
From: Maximilian Luz @ 2019-07-02  1:25 UTC (permalink / raw)
  To: Yu Chen
  Cc: linux-kernel, linux-input, platform-driver-x86, Dmitry Torokhov,
	Hans de Goede, Darren Hart, Andy Shevchenko, Benjamin Tissoires

On 7/2/19 3:14 AM, Yu Chen wrote:
> On Tue, Jul 02, 2019 at 02:37:39AM +0200, Maximilian Luz wrote:
>> +/*
>> + * Surface Pro 4 and Surface Book 2 / Surface Pro 2017 use the same device
>> + * ID (MSHW0040) for the power/volume buttons. Make sure this is the right
>> + * device by checking for the _DSM method and OEM Platform Revision.
>> + */
>> +static int surface_button_check_MSHW0040(struct acpi_device *dev)
>> +{
>> +	acpi_handle handle = dev->handle;
>> +	union acpi_object *result;
>> +	u64 oem_platform_rev = 0;
>> +
>> +	// get OEM platform revision
>> +	result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID,
>> +					 MSHW0040_DSM_REVISION,
>> +					 MSHW0040_DSM_GET_OMPR,
>> +					 NULL, ACPI_TYPE_INTEGER);
>> +
> Does it mean, only 5th, 6th and newer platforms have OEM platform revision?
> 3rd/4th will get NULL result? Or the opposite?

Correct, from my testing (with limited sample size) and AML code: 5th
and 6th generation devices have a non-zero OEM platform revision,
whereas 3rd and 4th gen. devices do not have any (i.e. result will be
NULL).

>> +	if (result) {
>> +		oem_platform_rev = result->integer.value;
>> +		ACPI_FREE(result);
>> +	}
>> +
>> +	dev_dbg(&dev->dev, "OEM Platform Revision %llu\n", oem_platform_rev);
>> +
>> +	return oem_platform_rev == 0 ? 0 : -ENODEV;
> if 3rd/4th do not have this oem rev information while 5th/newer have,
> why the latter returns NODEV(it actually has this info)?

Since we always expect a non-zero platform revision (for 5th/6th gen.),
we can initialize it to zero and use that as "unknown"/"not available".
So if it can not be determined, we return NODEV.

>> +}

Cheers,
Maximilian

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] platform: Fix device check for surfacepro3_button
  2019-07-02  1:25     ` Maximilian Luz
@ 2019-07-02  1:33       ` Maximilian Luz
  2019-07-02  1:57         ` Yu Chen
  0 siblings, 1 reply; 20+ messages in thread
From: Maximilian Luz @ 2019-07-02  1:33 UTC (permalink / raw)
  To: Yu Chen
  Cc: linux-kernel, linux-input, platform-driver-x86, Dmitry Torokhov,
	Hans de Goede, Darren Hart, Andy Shevchenko, Benjamin Tissoires

On 7/2/19 3:25 AM, Maximilian Luz wrote:
> On 7/2/19 3:14 AM, Yu Chen wrote:
>> On Tue, Jul 02, 2019 at 02:37:39AM +0200, Maximilian Luz wrote:
>>> +/*
>>> + * Surface Pro 4 and Surface Book 2 / Surface Pro 2017 use the same device
>>> + * ID (MSHW0040) for the power/volume buttons. Make sure this is the right
>>> + * device by checking for the _DSM method and OEM Platform Revision.
>>> + */
>>> +static int surface_button_check_MSHW0040(struct acpi_device *dev)
>>> +{
>>> +    acpi_handle handle = dev->handle;
>>> +    union acpi_object *result;
>>> +    u64 oem_platform_rev = 0;
>>> +
>>> +    // get OEM platform revision
>>> +    result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID,
>>> +                     MSHW0040_DSM_REVISION,
>>> +                     MSHW0040_DSM_GET_OMPR,
>>> +                     NULL, ACPI_TYPE_INTEGER);
>>> +
>> Does it mean, only 5th, 6th and newer platforms have OEM platform revision?
>> 3rd/4th will get NULL result? Or the opposite?
> 
> Correct, from my testing (with limited sample size) and AML code: 5th
> and 6th generation devices have a non-zero OEM platform revision,
> whereas 3rd and 4th gen. devices do not have any (i.e. result will be
> NULL).
> 
>>> +    if (result) {
>>> +        oem_platform_rev = result->integer.value;
>>> +        ACPI_FREE(result);
>>> +    }
>>> +
>>> +    dev_dbg(&dev->dev, "OEM Platform Revision %llu\n", oem_platform_rev);
>>> +
>>> +    return oem_platform_rev == 0 ? 0 : -ENODEV;
>> if 3rd/4th do not have this oem rev information while 5th/newer have,
>> why the latter returns NODEV(it actually has this info)?
> 
> Since we always expect a non-zero platform revision (for 5th/6th gen.),
> we can initialize it to zero and use that as "unknown"/"not available".
> So if it can not be determined, we return NODEV.

Sorry, small mistake here: If it can be determined (i.e. is 5th or 6th
gen.) then we return NODEV. Not the other way around.

Also to clarify on your last question:

On 7/2/19 3:14 AM, Yu Chen wrote:
>>   static int surface_button_add(struct acpi_device *device)
>>   {
>>   	struct surface_button *button;
>> @@ -154,6 +188,10 @@ static int surface_button_add(struct acpi_device *device)
>>   	    strlen(SURFACE_BUTTON_OBJ_NAME)))
>>   		return -ENODEV;
>>   
>> +	error = surface_button_check_MSHW0040(device);
>> +	if (error)
>> +		return error;
>> +
> ditto, 3rd/4th get error=0?

You are correct.

Maximilian

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] platform: Fix device check for surfacepro3_button
  2019-07-02  1:33       ` Maximilian Luz
@ 2019-07-02  1:57         ` Yu Chen
  2019-07-02  2:04           ` Maximilian Luz
  0 siblings, 1 reply; 20+ messages in thread
From: Yu Chen @ 2019-07-02  1:57 UTC (permalink / raw)
  To: Maximilian Luz
  Cc: linux-kernel, linux-input, platform-driver-x86, Dmitry Torokhov,
	Hans de Goede, Darren Hart, Andy Shevchenko, Benjamin Tissoires

On Tue, Jul 02, 2019 at 03:33:20AM +0200, Maximilian Luz wrote:
> On 7/2/19 3:25 AM, Maximilian Luz wrote:
> > On 7/2/19 3:14 AM, Yu Chen wrote:
> > > On Tue, Jul 02, 2019 at 02:37:39AM +0200, Maximilian Luz wrote:
> > > > +/*
> > > > + * Surface Pro 4 and Surface Book 2 / Surface Pro 2017 use the same device
> > > > + * ID (MSHW0040) for the power/volume buttons. Make sure this is the right
> > > > + * device by checking for the _DSM method and OEM Platform Revision.
> > > > + */
> > > > +static int surface_button_check_MSHW0040(struct acpi_device *dev)
> > > > +{
> > > > +    acpi_handle handle = dev->handle;
> > > > +    union acpi_object *result;
> > > > +    u64 oem_platform_rev = 0;
> > > > +
> > > > +    // get OEM platform revision
> > > > +    result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID,
> > > > +                     MSHW0040_DSM_REVISION,
> > > > +                     MSHW0040_DSM_GET_OMPR,
> > > > +                     NULL, ACPI_TYPE_INTEGER);
> > > > +
> > > Does it mean, only 5th, 6th and newer platforms have OEM platform revision?
> > > 3rd/4th will get NULL result? Or the opposite?
> > 
> > Correct, from my testing (with limited sample size) and AML code: 5th
> > and 6th generation devices have a non-zero OEM platform revision,
> > whereas 3rd and 4th gen. devices do not have any (i.e. result will be
> > NULL).
> > 
> > > > +    if (result) {
> > > > +        oem_platform_rev = result->integer.value;
> > > > +        ACPI_FREE(result);
> > > > +    }
> > > > +
> > > > +    dev_dbg(&dev->dev, "OEM Platform Revision %llu\n", oem_platform_rev);
> > > > +
> > > > +    return oem_platform_rev == 0 ? 0 : -ENODEV;
> > > if 3rd/4th do not have this oem rev information while 5th/newer have,
> > > why the latter returns NODEV(it actually has this info)?
> > 
> > Since we always expect a non-zero platform revision (for 5th/6th gen.),
> > we can initialize it to zero and use that as "unknown"/"not available".
> > So if it can not be determined, we return NODEV.
> 
> Sorry, small mistake here: If it can be determined (i.e. is 5th or 6th
> gen.) then we return NODEV. Not the other way around.
>
How about using a boolean, according to the function name, if a mshw0040 revison
is detected then returns true other wise false. Other than that,
Acked-by: Chen Yu <yu.c.chen@intel.com>

Best,
Chenyu
> Also to clarify on your last question:
> 
> On 7/2/19 3:14 AM, Yu Chen wrote:
> > >   static int surface_button_add(struct acpi_device *device)
> > >   {
> > >   	struct surface_button *button;
> > > @@ -154,6 +188,10 @@ static int surface_button_add(struct acpi_device *device)
> > >   	    strlen(SURFACE_BUTTON_OBJ_NAME)))
> > >   		return -ENODEV;
> > > +	error = surface_button_check_MSHW0040(device);
> > > +	if (error)
> > > +		return error;
> > > +
> > ditto, 3rd/4th get error=0?
> 
> You are correct.
> 
> Maximilian

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] platform: Fix device check for surfacepro3_button
  2019-07-02  1:57         ` Yu Chen
@ 2019-07-02  2:04           ` Maximilian Luz
  0 siblings, 0 replies; 20+ messages in thread
From: Maximilian Luz @ 2019-07-02  2:04 UTC (permalink / raw)
  To: Yu Chen
  Cc: linux-kernel, linux-input, platform-driver-x86, Dmitry Torokhov,
	Hans de Goede, Darren Hart, Andy Shevchenko, Benjamin Tissoires

On 7/2/19 3:57 AM, Yu Chen wrote:
> How about using a boolean, according to the function name, if a mshw0040 revison
> is detected then returns true other wise false. Other than that,
> Acked-by: Chen Yu <yu.c.chen@intel.com>

I can change that if you want me to. Just thought this might be a bit
more flexible in case we ever have to adapt the check for future device
generations.

Thanks,
Maximilian

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/2] Support for buttons on newer MS Surface devices
  2019-07-02  0:37 [PATCH 0/2] Support for buttons on newer MS Surface devices Maximilian Luz
  2019-07-02  0:37 ` [PATCH v2 1/2] platform: Fix device check for surfacepro3_button Maximilian Luz
  2019-07-02  0:37 ` [PATCH v2 2/2] input: soc_button_array for newer surface devices Maximilian Luz
@ 2019-07-02 17:13 ` Andy Shevchenko
  2019-07-02 17:26   ` Maximilian Luz
  2019-07-20 15:15 ` Maximilian Luz
  3 siblings, 1 reply; 20+ messages in thread
From: Andy Shevchenko @ 2019-07-02 17:13 UTC (permalink / raw)
  To: Maximilian Luz
  Cc: Linux Kernel Mailing List, linux-input, Platform Driver,
	Dmitry Torokhov, Hans de Goede, Chen Yu, Darren Hart,
	Andy Shevchenko, Benjamin Tissoires

On Tue, Jul 2, 2019 at 3:38 AM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
> This series adds suport for power and volume buttons on 5th and 6th
> generation Microsoft Surface devices. Specifically, it adds support for
> the power-button on the Surface Laptop 1 and Laptop 2, as well as
> support for power- and (on-device) volume-buttons on the Surface Pro 5
> (2017), Pro 6, and Book 2.
>
> These devices use the same MSHW0040 device as on the Surface Pro 4,
> however, whereas the Pro 4 uses an ACPI notify handler, the newer
> devices use GPIO interrupts to signal these events.
>
> The first patch of this series ensures that the surfacepro3_button
> driver, used for MSHW0040 on the Pro 4, does not probe for the newer
> devices. The second patch adapts soc_button_array to implement the
> actual button support.
>
> I think the changes to soc_button_array in the second patch warrant a
> thorough review. I've tried to make things a bit more generic to be able
> to integrate arbitrary ACPI GPIO power-/volume-button devices more
> easily, I'm not sure if there may be reasons against this.
>
> These patches have also been tested on various Surface devices via the
> github.com/jakeday/linux-surface patchset.
>

> Changes since v1:
>   - [PATCH 1/2] platform: Fix device check for surfacepro3_button
>     No changes.
>
>   - [PATCH 2/2] input: soc_button_array for newer surface devices
>     Ensure the patch compiles without CONFIG_ACPI.

I re-pushed to my queue, though if you are going to send a new
version, check my repository for the titles of the patches (you need
to use correct templates for the subsystems).

>
> Maximilian Luz (2):
>   platform: Fix device check for surfacepro3_button
>   input: soc_button_array for newer surface devices
>
>  drivers/input/misc/soc_button_array.c     | 145 ++++++++++++++++++++--
>  drivers/platform/x86/surfacepro3_button.c |  38 ++++++
>  2 files changed, 171 insertions(+), 12 deletions(-)
>
> --
> 2.22.0
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/2] Support for buttons on newer MS Surface devices
  2019-07-02 17:13 ` [PATCH 0/2] Support for buttons on newer MS Surface devices Andy Shevchenko
@ 2019-07-02 17:26   ` Maximilian Luz
  0 siblings, 0 replies; 20+ messages in thread
From: Maximilian Luz @ 2019-07-02 17:26 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linux Kernel Mailing List, linux-input, Platform Driver,
	Dmitry Torokhov, Hans de Goede, Chen Yu, Darren Hart,
	Andy Shevchenko, Benjamin Tissoires

On 7/2/19 7:13 PM, Andy Shevchenko wrote:
> I re-pushed to my queue, though if you are going to send a new
> version, check my repository for the titles of the patches (you need
> to use correct templates for the subsystems).

Got it, sorry for the inconvenience.

Thank you!
Maximilian

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] input: soc_button_array for newer surface devices
  2019-07-02  0:37 ` [PATCH v2 2/2] input: soc_button_array for newer surface devices Maximilian Luz
@ 2019-07-04 15:31   ` Maximilian Luz
  2019-07-16  7:21   ` Dmitry Torokhov
  1 sibling, 0 replies; 20+ messages in thread
From: Maximilian Luz @ 2019-07-04 15:31 UTC (permalink / raw)
  Cc: linux-kernel, linux-input, platform-driver-x86, Dmitry Torokhov,
	Hans de Goede, Chen Yu, Darren Hart, Andy Shevchenko,
	Benjamin Tissoires

On 7/2/19 2:37 AM, Maximilian Luz wrote:
> +static int soc_device_check_MSHW0040(struct device *dev)
> +{
> +	acpi_handle handle = ACPI_HANDLE(dev);
> +	union acpi_object *result;
> +	u64 oem_platform_rev = 0;
> +	int gpios;
> +
> +	// get OEM platform revision
> +	result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID,
> +					 MSHW0040_DSM_REVISION,
> +					 MSHW0040_DSM_GET_OMPR, NULL,
> +					 ACPI_TYPE_INTEGER);
> +
> +	if (result) {
> +		oem_platform_rev = result->integer.value;
> +		ACPI_FREE(result);
> +	}
> +
> +	if (oem_platform_rev == 0)
> +		return -ENODEV;
> +
> +	dev_dbg(dev, "OEM Platform Revision %llu\n", oem_platform_rev);
> +
> +	/*
> +	 * We are _really_ expecting GPIOs here. If we do not get any, this
> +	 * means the GPIO driver has not been loaded yet (which can happen).
> +	 * Try again later.
> +	 */
> +	gpios = gpiod_count(dev, NULL);
> +	if (gpios < 0)
> +		return -EAGAIN;
> +
> +	return 0;
> +}

Just had another look at this: Shouldn't the EAGAIN here be
EPROBE_DEFER?

The reasoning is that we would want to defer probing of the driver if we
can't get any GPIO pins, since we know that MSHW0040 should have some.
It has in the past been reported that the driver didn't load properly
(without this check), since it could happen that the GPIO subsystem
wasn't quite ready yet when probing.

Best,
Maximilian

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] input: soc_button_array for newer surface devices
  2019-07-02  0:37 ` [PATCH v2 2/2] input: soc_button_array for newer surface devices Maximilian Luz
  2019-07-04 15:31   ` Maximilian Luz
@ 2019-07-16  7:21   ` Dmitry Torokhov
  2019-07-16 18:19     ` Maximilian Luz
  1 sibling, 1 reply; 20+ messages in thread
From: Dmitry Torokhov @ 2019-07-16  7:21 UTC (permalink / raw)
  To: Maximilian Luz
  Cc: linux-kernel, linux-input, platform-driver-x86, Hans de Goede,
	Chen Yu, Darren Hart, Andy Shevchenko, Benjamin Tissoires

Hi Maximilian,

On Tue, Jul 02, 2019 at 02:37:40AM +0200, Maximilian Luz wrote:
> Power and volume button support for 5th and 6th genration Microsoft
> Surface devices via soc_button_array.
> 
> Note that these devices use the same MSHW0040 device as on the Surface
> Pro 4, however the implementation is different (GPIOs vs. ACPI
> notifications). Thus some checking is required to ensure we only load
> this driver on the correct devices.

When you are saying that Pro 4 and later models use different
notifications, does this mean that Pro 4 does not define any GPIOs? If
so can we use their presence as indicator whether we should be using
this driver or not. I would like to avoid repeating the ACPI parsing
code that you have in the platform driver.

> +static int soc_device_check_MSHW0040(struct device *dev)
> +{
> +	acpi_handle handle = ACPI_HANDLE(dev);
> +	union acpi_object *result;
> +	u64 oem_platform_rev = 0;
> +	int gpios;
> +
> +	// get OEM platform revision
> +	result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID,
> +					 MSHW0040_DSM_REVISION,
> +					 MSHW0040_DSM_GET_OMPR, NULL,
> +					 ACPI_TYPE_INTEGER);
> +
> +	if (result) {
> +		oem_platform_rev = result->integer.value;
> +		ACPI_FREE(result);
> +	}
> +
> +	if (oem_platform_rev == 0)
> +		return -ENODEV;
> +
> +	dev_dbg(dev, "OEM Platform Revision %llu\n", oem_platform_rev);
> +
> +	/*
> +	 * We are _really_ expecting GPIOs here. If we do not get any, this
> +	 * means the GPIO driver has not been loaded yet (which can happen).
> +	 * Try again later.
> +	 */
> +	gpios = gpiod_count(dev, NULL);
> +	if (gpios < 0)
> +		return -EAGAIN;

I do not believe -EAGAIN has any special meaning in the driver core;
also when the GPIO controller is not ready gpiod_get() will return
-EPROBE_DEFER, which is the prober way if signalling that some resource
is not yet available and probe should be retries at a later time.

Moreover, I do not believe that gpiod_count() needs GPIO controller to
be ready, the count is taken from board firmware or static board file
definition, so if gpiod_count() returns 0 it should be clear indication
that the driver should not be used with the device.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] input: soc_button_array for newer surface devices
  2019-07-16  7:21   ` Dmitry Torokhov
@ 2019-07-16 18:19     ` Maximilian Luz
  2019-07-16 20:18       ` Dmitry Torokhov
  0 siblings, 1 reply; 20+ messages in thread
From: Maximilian Luz @ 2019-07-16 18:19 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: linux-kernel, linux-input, platform-driver-x86, Hans de Goede,
	Chen Yu, Darren Hart, Andy Shevchenko, Benjamin Tissoires


Hi,

On 7/16/19 9:21 AM, Dmitry Torokhov wrote:
> When you are saying that Pro 4 and later models use different
> notifications, does this mean that Pro 4 does not define any GPIOs?

Unfortunately, at least the Surface Book (first generation, buttons are
handled the same way as on the Pro 4) has GPIOs defined in MSHW0040, but
they are for different use. Specifically: They can detect if the
clipboard (screen part of the device, the device basically has two parts
that can be separated: clipboard and base) is being removed. Relying on
the GPIOs was my first idea too, but that has been reported to shut down
the Book 1 when the clipboard is detached.

As far as I know, the OEM platform revision check is the easiest way to
differentiate between those devices.

> I do not believe -EAGAIN has any special meaning in the driver core;

I think I got the -EAGAIN from an outdated LWN article when I first
started working on this, thanks for confirming.

> also when the GPIO controller is not ready gpiod_get() will return
> -EPROBE_DEFER, which is the prober way if signalling that some resource
> is not yet available and probe should be retries at a later time.
>
> Moreover, I do not believe that gpiod_count() needs GPIO controller to
> be ready, the count is taken from board firmware or static board file
> definition, so if gpiod_count() returns 0 it should be clear indication
> that the driver should not be used with the device.

Thank you for this insight, I will update the patch accordingly.

Maximilian

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] input: soc_button_array for newer surface devices
  2019-07-16 18:19     ` Maximilian Luz
@ 2019-07-16 20:18       ` Dmitry Torokhov
  2019-07-17 19:23         ` Maximilian Luz
  0 siblings, 1 reply; 20+ messages in thread
From: Dmitry Torokhov @ 2019-07-16 20:18 UTC (permalink / raw)
  To: Maximilian Luz
  Cc: linux-kernel, linux-input, platform-driver-x86, Hans de Goede,
	Chen Yu, Darren Hart, Andy Shevchenko, Benjamin Tissoires

On Tue, Jul 16, 2019 at 08:19:04PM +0200, Maximilian Luz wrote:
> 
> Hi,
> 
> On 7/16/19 9:21 AM, Dmitry Torokhov wrote:
> > When you are saying that Pro 4 and later models use different
> > notifications, does this mean that Pro 4 does not define any GPIOs?
> 
> Unfortunately, at least the Surface Book (first generation, buttons are
> handled the same way as on the Pro 4) has GPIOs defined in MSHW0040, but
> they are for different use. Specifically: They can detect if the
> clipboard (screen part of the device, the device basically has two parts
> that can be separated: clipboard and base) is being removed. Relying on
> the GPIOs was my first idea too, but that has been reported to shut down
> the Book 1 when the clipboard is detached.
> 
> As far as I know, the OEM platform revision check is the easiest way to
> differentiate between those devices.

OK, fair enough. By the way, I see you are adding some #ifdef
CONFIG_ACPI and stubbing out new functions, but the driver does not
really work without ACPI (acpi_match_device() will fail in this case I
would think and that will cause probe() to abort). So maybe we just add
depends on ACPI to the driver's Kconfig entry?

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] input: soc_button_array for newer surface devices
  2019-07-16 20:18       ` Dmitry Torokhov
@ 2019-07-17 19:23         ` Maximilian Luz
  0 siblings, 0 replies; 20+ messages in thread
From: Maximilian Luz @ 2019-07-17 19:23 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: linux-kernel, linux-input, platform-driver-x86, Hans de Goede,
	Chen Yu, Darren Hart, Andy Shevchenko, Benjamin Tissoires

On 7/16/19 10:18 PM, Dmitry Torokhov wrote:
> OK, fair enough. By the way, I see you are adding some #ifdef
> CONFIG_ACPI and stubbing out new functions, but the driver does not
> really work without ACPI (acpi_match_device() will fail in this case I
> would think and that will cause probe() to abort). So maybe we just add
> depends on ACPI to the driver's Kconfig entry?

Makes sense, I can change that too.

Maximilian

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] platform: Fix device check for surfacepro3_button
  2019-07-02  0:37 ` [PATCH v2 1/2] platform: Fix device check for surfacepro3_button Maximilian Luz
  2019-07-02  1:14   ` Yu Chen
@ 2019-07-18 17:43   ` Andy Shevchenko
  1 sibling, 0 replies; 20+ messages in thread
From: Andy Shevchenko @ 2019-07-18 17:43 UTC (permalink / raw)
  To: Maximilian Luz
  Cc: Linux Kernel Mailing List, linux-input, Platform Driver,
	Dmitry Torokhov, Hans de Goede, Chen Yu, Darren Hart,
	Andy Shevchenko, Benjamin Tissoires

On Tue, Jul 2, 2019 at 3:38 AM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
> Do not use the surfacepro3_button driver on newer Microsoft Surface
> models, only use it on the Surface Pro 3 and 4. Newer models (5th, 6th
> and possibly future generations) use the same device as the Surface Pro
> 4 to represent their volume and power buttons (MSHW0040), but their
> acutal implementation is significantly different. This patch ensures
> that the surfacepro3_button driver is only used on the Pro 3 and 4
> models, allowing a different driver to bind on other models.
>

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Assuming it will go thru Input subsystem.

> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> ---
>  drivers/platform/x86/surfacepro3_button.c | 38 +++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/drivers/platform/x86/surfacepro3_button.c b/drivers/platform/x86/surfacepro3_button.c
> index 47c6d000465a..0e2c7dfafd9f 100644
> --- a/drivers/platform/x86/surfacepro3_button.c
> +++ b/drivers/platform/x86/surfacepro3_button.c
> @@ -20,6 +20,12 @@
>  #define SURFACE_BUTTON_OBJ_NAME                "VGBI"
>  #define SURFACE_BUTTON_DEVICE_NAME     "Surface Pro 3/4 Buttons"
>
> +#define MSHW0040_DSM_REVISION          0x01
> +#define MSHW0040_DSM_GET_OMPR          0x02    // get OEM Platform Revision
> +static const guid_t MSHW0040_DSM_UUID =
> +       GUID_INIT(0x6fd05c69, 0xcde3, 0x49f4, 0x95, 0xed, 0xab, 0x16, 0x65,
> +                 0x49, 0x80, 0x35);
> +
>  #define SURFACE_BUTTON_NOTIFY_TABLET_MODE      0xc8
>
>  #define SURFACE_BUTTON_NOTIFY_PRESS_POWER      0xc6
> @@ -142,6 +148,34 @@ static int surface_button_resume(struct device *dev)
>  }
>  #endif
>
> +/*
> + * Surface Pro 4 and Surface Book 2 / Surface Pro 2017 use the same device
> + * ID (MSHW0040) for the power/volume buttons. Make sure this is the right
> + * device by checking for the _DSM method and OEM Platform Revision.
> + */
> +static int surface_button_check_MSHW0040(struct acpi_device *dev)
> +{
> +       acpi_handle handle = dev->handle;
> +       union acpi_object *result;
> +       u64 oem_platform_rev = 0;
> +
> +       // get OEM platform revision
> +       result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID,
> +                                        MSHW0040_DSM_REVISION,
> +                                        MSHW0040_DSM_GET_OMPR,
> +                                        NULL, ACPI_TYPE_INTEGER);
> +
> +       if (result) {
> +               oem_platform_rev = result->integer.value;
> +               ACPI_FREE(result);
> +       }
> +
> +       dev_dbg(&dev->dev, "OEM Platform Revision %llu\n", oem_platform_rev);
> +
> +       return oem_platform_rev == 0 ? 0 : -ENODEV;
> +}
> +
> +
>  static int surface_button_add(struct acpi_device *device)
>  {
>         struct surface_button *button;
> @@ -154,6 +188,10 @@ static int surface_button_add(struct acpi_device *device)
>             strlen(SURFACE_BUTTON_OBJ_NAME)))
>                 return -ENODEV;
>
> +       error = surface_button_check_MSHW0040(device);
> +       if (error)
> +               return error;
> +
>         button = kzalloc(sizeof(struct surface_button), GFP_KERNEL);
>         if (!button)
>                 return -ENOMEM;
> --
> 2.22.0
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/2] Support for buttons on newer MS Surface devices
  2019-07-02  0:37 [PATCH 0/2] Support for buttons on newer MS Surface devices Maximilian Luz
                   ` (2 preceding siblings ...)
  2019-07-02 17:13 ` [PATCH 0/2] Support for buttons on newer MS Surface devices Andy Shevchenko
@ 2019-07-20 15:15 ` Maximilian Luz
  3 siblings, 0 replies; 20+ messages in thread
From: Maximilian Luz @ 2019-07-20 15:15 UTC (permalink / raw)
  Cc: linux-kernel, linux-input, platform-driver-x86, Dmitry Torokhov,
	Hans de Goede, Chen Yu, Darren Hart, Andy Shevchenko,
	Benjamin Tissoires

I have updated the patches with the requested changes and sent a v3.

Best,
Maximilian

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/2] Support for buttons on newer MS Surface devices
  2019-06-29 14:18 ` Andy Shevchenko
@ 2019-07-02  0:43   ` Maximilian Luz
  0 siblings, 0 replies; 20+ messages in thread
From: Maximilian Luz @ 2019-07-02  0:43 UTC (permalink / raw)
  Cc: Linux Kernel Mailing List, linux-input, Platform Driver,
	Dmitry Torokhov, Hans de Goede, Chen Yu, Darren Hart,
	Andy Shevchenko, Benjamin Tissoires

On 6/29/19 4:18 PM, Andy Shevchenko wrote:
> Pushed to my review and testing queue, thanks!

Sorry for my rookie mistake of not checking that this works without
CONFIG_ACPI. I have updated and re-sent the patches to fix this.

Maximilian

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/2] Support for buttons on newer MS Surface devices
  2019-06-20 11:50 Maximilian Luz
@ 2019-06-29 14:18 ` Andy Shevchenko
  2019-07-02  0:43   ` Maximilian Luz
  0 siblings, 1 reply; 20+ messages in thread
From: Andy Shevchenko @ 2019-06-29 14:18 UTC (permalink / raw)
  To: Maximilian Luz
  Cc: Linux Kernel Mailing List, linux-input, Platform Driver,
	Dmitry Torokhov, Hans de Goede, Chen Yu, Darren Hart,
	Andy Shevchenko, Benjamin Tissoires

On Thu, Jun 20, 2019 at 2:51 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
> This series adds suport for power and volume buttons on 5th and 6th
> generation Microsoft Surface devices. Specifically, it adds support for
> the power-button on the Surface Laptop 1 and Laptop 2, as well as
> support for power- and (on-device) volume-buttons on the Surface Pro 5
> (2017), Pro 6, and Book 2.
>
> These devices use the same MSHW0040 device as on the Surface Pro 4,
> however, whereas the Pro 4 uses an ACPI notify handler, the newer
> devices use GPIO interrupts to signal these events.
>
> The first patch of this series ensures that the surfacepro3_button
> driver, used for MSHW0040 on the Pro 4, does not probe for the newer
> devices. The second patch adapts soc_button_array to implement the
> actual button support.
>
> I think the changes to soc_button_array in the second patch warrant a
> thorough review. I've tried to make things a bit more generic to be able
> to integrate arbitrary ACPI GPIO power-/volume-button devices more
> easily, I'm not sure if there may be reasons against this.
>
> These patches have also been tested on various Surface devices via the
> github.com/jakeday/linux-surface patchset.
>

Pushed to my review and testing queue, thanks!

> Maximilian Luz (2):
>   platform: Fix device check for surfacepro3_button
>   input: soc_button_array for newer surface devices
>
>  drivers/input/misc/soc_button_array.c     | 134 ++++++++++++++++++++--
>  drivers/platform/x86/surfacepro3_button.c |  38 ++++++
>  2 files changed, 160 insertions(+), 12 deletions(-)
>
> --
> 2.22.0
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 0/2] Support for buttons on newer MS Surface devices
@ 2019-06-20 11:50 Maximilian Luz
  2019-06-29 14:18 ` Andy Shevchenko
  0 siblings, 1 reply; 20+ messages in thread
From: Maximilian Luz @ 2019-06-20 11:50 UTC (permalink / raw)
  Cc: linux-kernel, linux-input, platform-driver-x86, Dmitry Torokhov,
	Hans de Goede, Chen Yu, Darren Hart, Andy Shevchenko,
	Benjamin Tissoires, Maximilian Luz

This series adds suport for power and volume buttons on 5th and 6th
generation Microsoft Surface devices. Specifically, it adds support for
the power-button on the Surface Laptop 1 and Laptop 2, as well as
support for power- and (on-device) volume-buttons on the Surface Pro 5
(2017), Pro 6, and Book 2.

These devices use the same MSHW0040 device as on the Surface Pro 4,
however, whereas the Pro 4 uses an ACPI notify handler, the newer
devices use GPIO interrupts to signal these events.

The first patch of this series ensures that the surfacepro3_button
driver, used for MSHW0040 on the Pro 4, does not probe for the newer
devices. The second patch adapts soc_button_array to implement the
actual button support.

I think the changes to soc_button_array in the second patch warrant a
thorough review. I've tried to make things a bit more generic to be able
to integrate arbitrary ACPI GPIO power-/volume-button devices more
easily, I'm not sure if there may be reasons against this.

These patches have also been tested on various Surface devices via the
github.com/jakeday/linux-surface patchset.

Maximilian Luz (2):
  platform: Fix device check for surfacepro3_button
  input: soc_button_array for newer surface devices

 drivers/input/misc/soc_button_array.c     | 134 ++++++++++++++++++++--
 drivers/platform/x86/surfacepro3_button.c |  38 ++++++
 2 files changed, 160 insertions(+), 12 deletions(-)

-- 
2.22.0


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2019-07-20 15:15 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-02  0:37 [PATCH 0/2] Support for buttons on newer MS Surface devices Maximilian Luz
2019-07-02  0:37 ` [PATCH v2 1/2] platform: Fix device check for surfacepro3_button Maximilian Luz
2019-07-02  1:14   ` Yu Chen
2019-07-02  1:25     ` Maximilian Luz
2019-07-02  1:33       ` Maximilian Luz
2019-07-02  1:57         ` Yu Chen
2019-07-02  2:04           ` Maximilian Luz
2019-07-18 17:43   ` Andy Shevchenko
2019-07-02  0:37 ` [PATCH v2 2/2] input: soc_button_array for newer surface devices Maximilian Luz
2019-07-04 15:31   ` Maximilian Luz
2019-07-16  7:21   ` Dmitry Torokhov
2019-07-16 18:19     ` Maximilian Luz
2019-07-16 20:18       ` Dmitry Torokhov
2019-07-17 19:23         ` Maximilian Luz
2019-07-02 17:13 ` [PATCH 0/2] Support for buttons on newer MS Surface devices Andy Shevchenko
2019-07-02 17:26   ` Maximilian Luz
2019-07-20 15:15 ` Maximilian Luz
  -- strict thread matches above, loose matches on Subject: below --
2019-06-20 11:50 Maximilian Luz
2019-06-29 14:18 ` Andy Shevchenko
2019-07-02  0:43   ` Maximilian Luz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).