[PATCH v2 0/2] Add driver for power button on Dell Wyse 3020

[PATCH v2 0/2] Add driver for power button on Dell Wyse 3020

[Lubomir Rintel]

please consider applying the patches chained to this message. It's a
rather simple driver for a power button on Dell Ariel board along with
the Device Tree binding document.

Compared to previous version, endianness issues are fixed and I
connected a Reviewed-by for the binding doc that I failed to before.
Change logs are present in individual patches.

Note that this set is versioned "v2", but I've sent the previous version
(both unversioned) out twice before, because I've forgotten about
the previous submission before. Sorry if it caused a confusion.

Thank you
Lubo

[PATCH v2 1/2] dt-bindings: input: Add Dell Wyse 3020 Power Button binding

[Lubomir Rintel]

Add binding document for the Dell Wyse 3020 a.k.a. "Ariel" Power Button.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Reviewed-by: Rob Herring <robh@kernel.org>

---
Changes since v1:
- Collect Rob's R-b

 .../bindings/input/ariel-pwrbutton.yaml       | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/ariel-pwrbutton.yaml

diff --git a/Documentation/devicetree/bindings/input/ariel-pwrbutton.yaml b/Documentation/devicetree/bindings/input/ariel-pwrbutton.yaml
new file mode 100644
index 0000000000000..e022d36c48d23
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/ariel-pwrbutton.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/ariel-pwrbutton.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Dell Wyse 3020 a.k.a. "Ariel" Power Button
+
+maintainers:
+  - Lubomir Rintel <lkundrak@v3.sk>
+
+description: |
+  The ENE Embedded Controller on the Ariel board has an interface to the
+  SPI bus that is capable of sending keyboard and mouse data. A single
+  power button is attached to it. This binding describes this
+  configuration.
+
+allOf:
+  - $ref: input.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: dell,wyse-ariel-ec-input
+      - const: ene,kb3930-input
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        power-button@0 {
+            compatible = "dell,wyse-ariel-ec-input", "ene,kb3930-input";
+            reg = <0>;
+            interrupt-parent = <&gpio>;
+            interrupts = <60 IRQ_TYPE_EDGE_RISING>;
+            spi-max-frequency = <33000000>;
+        };
+    };
-- 
2.28.0

Re: [PATCH v2 1/2] dt-bindings: input: Add Dell Wyse 3020 Power Button binding

[Rob Herring]

On Sun, 29 Nov 2020 15:21:44 +0100, Lubomir Rintel wrote:
> Add binding document for the Dell Wyse 3020 a.k.a. "Ariel" Power Button.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Reviewed-by: Rob Herring <robh@kernel.org>
> 
> ---
> Changes since v1:
> - Collect Rob's R-b
> 
>  .../bindings/input/ariel-pwrbutton.yaml       | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/ariel-pwrbutton.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/input/ariel-pwrbutton.yaml: 'oneOf' conditional failed, one must be fixed:
	'unevaluatedProperties' is a required property
	'additionalProperties' is a required property
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/input/ariel-pwrbutton.yaml: ignoring, error in schema: 
warning: no schema found in file: ./Documentation/devicetree/bindings/input/ariel-pwrbutton.yaml


See https://patchwork.ozlabs.org/patch/1407831

The base for the patch is generally the last rc1. Any dependencies
should be noted.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

[PATCH v2 2/2] Input: add driver for power button on Dell Wyse 3020

[Lubomir Rintel]

This adds support for the power button attached to the Embedded Controller
on a Dell Wyse 3020 "Ariel" board.

The Embedded Controller's SPI interface is actually capable sending and
receiving the PS/2 keyboard and mouse protocol data, which looks like
a good fit for a serio driver. Howerver, I don't know of any machines where
this is actually used.

My board only has a single power button and no way to connect an actual
keyboard or a mouse. Using the atkbd driver with serio would be an overkill
and would be inconvenient for the userspace. Therefore this driver
registers an input device that is only capable of reporting the power
button presses and releases.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

---
Changes since v1:
- Do away bitfields in order to be endian independent

 drivers/input/misc/Kconfig           |  11 ++
 drivers/input/misc/Makefile          |   1 +
 drivers/input/misc/ariel-pwrbutton.c | 165 +++++++++++++++++++++++++++
 3 files changed, 177 insertions(+)
 create mode 100644 drivers/input/misc/ariel-pwrbutton.c

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 362e8a01980cd..e7bb572e15182 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -73,6 +73,17 @@ config INPUT_AD714X_SPI
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad714x-spi.
 
+config INPUT_ARIEL_PWRBUTTON
+	tristate "Dell Wyse 3020 Power Button Driver"
+	depends on SPI
+	depends on MACH_MMP3_DT || COMPILE_TEST
+	help
+	  Say Y to enable support for reporting power button status on
+	  on Dell Wyse 3020 ("Ariel") thin client.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called ariel-pwrbutton.
+
 config INPUT_ARIZONA_HAPTICS
 	tristate "Arizona haptics support"
 	depends on MFD_ARIZONA && SND_SOC
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index a48e5f2d859d4..062cea9f181c9 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_INPUT_ADXL34X)		+= adxl34x.o
 obj-$(CONFIG_INPUT_ADXL34X_I2C)		+= adxl34x-i2c.o
 obj-$(CONFIG_INPUT_ADXL34X_SPI)		+= adxl34x-spi.o
 obj-$(CONFIG_INPUT_APANEL)		+= apanel.o
+obj-$(CONFIG_INPUT_ARIEL_PWRBUTTON)	+= ariel-pwrbutton.o
 obj-$(CONFIG_INPUT_ARIZONA_HAPTICS)	+= arizona-haptics.o
 obj-$(CONFIG_INPUT_ATI_REMOTE2)		+= ati_remote2.o
 obj-$(CONFIG_INPUT_ATLAS_BTNS)		+= atlas_btns.o
diff --git a/drivers/input/misc/ariel-pwrbutton.c b/drivers/input/misc/ariel-pwrbutton.c
new file mode 100644
index 0000000000000..502bc6a65f657
--- /dev/null
+++ b/drivers/input/misc/ariel-pwrbutton.c
@@ -0,0 +1,165 @@
+// SPDX-License-Identifier: BSD-2-Clause OR GPL-2.0-or-later
+/*
+ * Dell Wyse 3020 a.k.a. "Ariel" Power Button Driver
+ *
+ * Copyright (C) 2020 Lubomir Rintel
+ */
+
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/interrupt.h>
+#include <linux/input.h>
+
+#define RESP_COUNTER(response)	(response.header & 0x3)
+#define RESP_SIZE(response)	((response.header >> 2) & 0x3)
+#define RESP_TYPE(response)	((response.header >> 4) & 0xf)
+
+struct ec_input_response {
+	u8 reserved;
+	u8 header;
+	u8 data[3];
+} __packed;
+
+struct ariel_pwrbutton {
+	struct spi_device *client;
+	struct input_dev *input;
+	u8 msg_counter:2;
+};
+
+static int ec_input_read(struct ariel_pwrbutton *priv,
+		      struct ec_input_response *response)
+{
+	u8 read_request[] = { 0x00, 0x5a, 0xa5, 0x00, 0x00 };
+	struct spi_device *spi = priv->client;
+	struct spi_transfer t = {
+		.tx_buf = read_request,
+		.rx_buf = response,
+		.len = sizeof(read_request),
+	};
+
+	compiletime_assert(sizeof(read_request) == sizeof(*response),
+			   "SPI xfer request/response size mismatch");
+
+	return spi_sync_transfer(spi, &t, 1);
+}
+
+static irqreturn_t ec_input_interrupt(int irq, void *dev_id)
+{
+	struct ariel_pwrbutton *priv = dev_id;
+	struct spi_device *spi = priv->client;
+	struct ec_input_response response;
+	int i;
+
+	if (ec_input_read(priv, &response) < 0) {
+		dev_err(&spi->dev, "EC read failed.\n");
+		return IRQ_HANDLED;
+	}
+
+	if (priv->msg_counter == RESP_COUNTER(response)) {
+		dev_warn(&spi->dev, "No new data to read?\n");
+		return IRQ_HANDLED;
+	}
+
+	priv->msg_counter = RESP_COUNTER(response);
+
+	if (RESP_TYPE(response) != 0x3 && RESP_TYPE(response) != 0xc) {
+		dev_dbg(&spi->dev, "Ignoring message that's not kbd data\n");
+		return IRQ_HANDLED;
+	}
+
+	for (i = 0; i < RESP_SIZE(response); i++) {
+		dev_err(&spi->dev, "scan code %02x\n", response.data[i]);
+		switch (response.data[i]) {
+		case 0x74:
+			input_report_key(priv->input, KEY_POWER, 1);
+			input_sync(priv->input);
+			break;
+		case 0xf4:
+			input_report_key(priv->input, KEY_POWER, 0);
+			input_sync(priv->input);
+			break;
+		default:
+			dev_dbg(&spi->dev, "Unknown scan code: %02x\n",
+				response.data[i]);
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int ariel_pwrbutton_probe(struct spi_device *spi)
+{
+	struct ec_input_response response;
+	struct ariel_pwrbutton *priv;
+	int ret;
+
+	if (!spi->irq) {
+		dev_err(&spi->dev, "Missing IRQ.\n");
+		return -ENXIO;
+	}
+
+	priv = devm_kzalloc(&spi->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->client = spi;
+	spi_set_drvdata(spi, priv);
+
+	priv->input = devm_input_allocate_device(&spi->dev);
+	if (!priv->input)
+		return -ENOMEM;
+	priv->input->name = "Power Button";
+	priv->input->dev.parent = &spi->dev;
+	input_set_capability(priv->input, EV_KEY, KEY_POWER);
+	ret = input_register_device(priv->input);
+	if (ret) {
+		dev_err(&spi->dev, "error registering input device: %d\n", ret);
+		return ret;
+	}
+
+	ret = ec_input_read(priv, &response);
+	if (ret < 0) {
+		dev_err(&spi->dev, "EC read failed: %d\n", ret);
+		return ret;
+	}
+	priv->msg_counter = RESP_COUNTER(response);
+
+	ret = devm_request_threaded_irq(&spi->dev, spi->irq, NULL,
+					ec_input_interrupt,
+					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+					"Ariel EC Input", priv);
+
+	if (ret) {
+		dev_err(&spi->dev, "Failed to request IRQ %d: %d\n",
+			spi->irq, ret);
+		return ret;
+	}
+
+	dev_info(&spi->dev, "Dell Wyse 3020 Power Button\n");
+	return 0;
+}
+
+static const struct of_device_id ariel_pwrbutton_of_match[] = {
+	{ .compatible = "dell,wyse-ariel-ec-input" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ariel_pwrbutton_of_match);
+
+static const struct spi_device_id ariel_pwrbutton_id_table[] = {
+	{ "wyse-ariel-ec-input", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, ariel_pwrbutton_id_table);
+
+static struct spi_driver ariel_pwrbutton_driver = {
+	.driver = {
+		.name = "dell-wyse-ariel-ec-input",
+		.of_match_table = ariel_pwrbutton_of_match,
+	},
+	.probe = ariel_pwrbutton_probe,
+};
+module_spi_driver(ariel_pwrbutton_driver);
+
+MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>");
+MODULE_DESCRIPTION("Dell Wyse 3020 Power Button Input Driver");
+MODULE_LICENSE("Dual BSD/GPL");
-- 
2.28.0

Re: [PATCH v2 2/2] Input: add driver for power button on Dell Wyse 3020

[Dmitry Torokhov]

Hi Lubomir,

On Sun, Nov 29, 2020 at 03:21:45PM +0100, Lubomir Rintel wrote:
> This adds support for the power button attached to the Embedded Controller
> on a Dell Wyse 3020 "Ariel" board.
> 
> The Embedded Controller's SPI interface is actually capable sending and
> receiving the PS/2 keyboard and mouse protocol data, which looks like
> a good fit for a serio driver. Howerver, I don't know of any machines where
> this is actually used.
> 
> My board only has a single power button and no way to connect an actual
> keyboard or a mouse. Using the atkbd driver with serio would be an overkill
> and would be inconvenient for the userspace. Therefore this driver
> registers an input device that is only capable of reporting the power
> button presses and releases.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

Just a few small comments.

> 
> ---
> Changes since v1:
> - Do away bitfields in order to be endian independent
> 
>  drivers/input/misc/Kconfig           |  11 ++
>  drivers/input/misc/Makefile          |   1 +
>  drivers/input/misc/ariel-pwrbutton.c | 165 +++++++++++++++++++++++++++
>  3 files changed, 177 insertions(+)
>  create mode 100644 drivers/input/misc/ariel-pwrbutton.c
> 
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 362e8a01980cd..e7bb572e15182 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -73,6 +73,17 @@ config INPUT_AD714X_SPI
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad714x-spi.
>  
> +config INPUT_ARIEL_PWRBUTTON
> +	tristate "Dell Wyse 3020 Power Button Driver"
> +	depends on SPI
> +	depends on MACH_MMP3_DT || COMPILE_TEST
> +	help
> +	  Say Y to enable support for reporting power button status on
> +	  on Dell Wyse 3020 ("Ariel") thin client.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called ariel-pwrbutton.
> +
>  config INPUT_ARIZONA_HAPTICS
>  	tristate "Arizona haptics support"
>  	depends on MFD_ARIZONA && SND_SOC
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index a48e5f2d859d4..062cea9f181c9 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_INPUT_ADXL34X)		+= adxl34x.o
>  obj-$(CONFIG_INPUT_ADXL34X_I2C)		+= adxl34x-i2c.o
>  obj-$(CONFIG_INPUT_ADXL34X_SPI)		+= adxl34x-spi.o
>  obj-$(CONFIG_INPUT_APANEL)		+= apanel.o
> +obj-$(CONFIG_INPUT_ARIEL_PWRBUTTON)	+= ariel-pwrbutton.o
>  obj-$(CONFIG_INPUT_ARIZONA_HAPTICS)	+= arizona-haptics.o
>  obj-$(CONFIG_INPUT_ATI_REMOTE2)		+= ati_remote2.o
>  obj-$(CONFIG_INPUT_ATLAS_BTNS)		+= atlas_btns.o
> diff --git a/drivers/input/misc/ariel-pwrbutton.c b/drivers/input/misc/ariel-pwrbutton.c
> new file mode 100644
> index 0000000000000..502bc6a65f657
> --- /dev/null
> +++ b/drivers/input/misc/ariel-pwrbutton.c
> @@ -0,0 +1,165 @@
> +// SPDX-License-Identifier: BSD-2-Clause OR GPL-2.0-or-later
> +/*
> + * Dell Wyse 3020 a.k.a. "Ariel" Power Button Driver
> + *
> + * Copyright (C) 2020 Lubomir Rintel
> + */
> +
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/interrupt.h>
> +#include <linux/input.h>

This is very light on includes. Please check what other includes might
be needed (devm_kzalloc? GFP_KERNEL? of_device_id?).

> +
> +#define RESP_COUNTER(response)	(response.header & 0x3)
> +#define RESP_SIZE(response)	((response.header >> 2) & 0x3)
> +#define RESP_TYPE(response)	((response.header >> 4) & 0xf)
> +
> +struct ec_input_response {
> +	u8 reserved;
> +	u8 header;
> +	u8 data[3];
> +} __packed;
> +
> +struct ariel_pwrbutton {
> +	struct spi_device *client;
> +	struct input_dev *input;
> +	u8 msg_counter:2;

I do not think there is any benefit of declaring it as bitfield vs
simply a byte.

> +};
> +
> +static int ec_input_read(struct ariel_pwrbutton *priv,
> +		      struct ec_input_response *response)
> +{
> +	u8 read_request[] = { 0x00, 0x5a, 0xa5, 0x00, 0x00 };
> +	struct spi_device *spi = priv->client;
> +	struct spi_transfer t = {
> +		.tx_buf = read_request,
> +		.rx_buf = response,
> +		.len = sizeof(read_request),
> +	};
> +
> +	compiletime_assert(sizeof(read_request) == sizeof(*response),
> +			   "SPI xfer request/response size mismatch");
> +
> +	return spi_sync_transfer(spi, &t, 1);
> +}
> +
> +static irqreturn_t ec_input_interrupt(int irq, void *dev_id)
> +{
> +	struct ariel_pwrbutton *priv = dev_id;
> +	struct spi_device *spi = priv->client;
> +	struct ec_input_response response;
> +	int i;
> +
> +	if (ec_input_read(priv, &response) < 0) {
> +		dev_err(&spi->dev, "EC read failed.\n");

Might be useful to print error code too.

> +		return IRQ_HANDLED;

Prefer t have single exit in interrupt handlers, so "goto out".

> +	}
> +
> +	if (priv->msg_counter == RESP_COUNTER(response)) {
> +		dev_warn(&spi->dev, "No new data to read?\n");
> +		return IRQ_HANDLED;
> +	}
> +
> +	priv->msg_counter = RESP_COUNTER(response);
> +
> +	if (RESP_TYPE(response) != 0x3 && RESP_TYPE(response) != 0xc) {
> +		dev_dbg(&spi->dev, "Ignoring message that's not kbd data\n");
> +		return IRQ_HANDLED;
> +	}
> +
> +	for (i = 0; i < RESP_SIZE(response); i++) {
> +		dev_err(&spi->dev, "scan code %02x\n", response.data[i]);

dev_dbg() at most.

> +		switch (response.data[i]) {
> +		case 0x74:
> +			input_report_key(priv->input, KEY_POWER, 1);
> +			input_sync(priv->input);
> +			break;
> +		case 0xf4:
> +			input_report_key(priv->input, KEY_POWER, 0);
> +			input_sync(priv->input);
> +			break;
> +		default:
> +			dev_dbg(&spi->dev, "Unknown scan code: %02x\n",
> +				response.data[i]);
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ariel_pwrbutton_probe(struct spi_device *spi)
> +{
> +	struct ec_input_response response;
> +	struct ariel_pwrbutton *priv;
> +	int ret;
> +
> +	if (!spi->irq) {
> +		dev_err(&spi->dev, "Missing IRQ.\n");
> +		return -ENXIO;

-EINVAL?

> +	}
> +
> +	priv = devm_kzalloc(&spi->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->client = spi;
> +	spi_set_drvdata(spi, priv);
> +
> +	priv->input = devm_input_allocate_device(&spi->dev);
> +	if (!priv->input)
> +		return -ENOMEM;
> +	priv->input->name = "Power Button";
> +	priv->input->dev.parent = &spi->dev;
> +	input_set_capability(priv->input, EV_KEY, KEY_POWER);
> +	ret = input_register_device(priv->input);

Can we please call this variable "error"?

> +	if (ret) {
> +		dev_err(&spi->dev, "error registering input device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = ec_input_read(priv, &response);
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "EC read failed: %d\n", ret);
> +		return ret;
> +	}
> +	priv->msg_counter = RESP_COUNTER(response);
> +
> +	ret = devm_request_threaded_irq(&spi->dev, spi->irq, NULL,
> +					ec_input_interrupt,
> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,

Let's rely on the platform (ACPI/DT) to specify the interrupt trigger.

> +					"Ariel EC Input", priv);
> +
> +	if (ret) {
> +		dev_err(&spi->dev, "Failed to request IRQ %d: %d\n",
> +			spi->irq, ret);
> +		return ret;
> +	}
> +
> +	dev_info(&spi->dev, "Dell Wyse 3020 Power Button\n");

Input core already prints when a new input device is created, so this is
not needed.

> +	return 0;
> +}
> +
> +static const struct of_device_id ariel_pwrbutton_of_match[] = {
> +	{ .compatible = "dell,wyse-ariel-ec-input" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ariel_pwrbutton_of_match);
> +
> +static const struct spi_device_id ariel_pwrbutton_id_table[] = {
> +	{ "wyse-ariel-ec-input", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, ariel_pwrbutton_id_table);
> +
> +static struct spi_driver ariel_pwrbutton_driver = {
> +	.driver = {
> +		.name = "dell-wyse-ariel-ec-input",
> +		.of_match_table = ariel_pwrbutton_of_match,
> +	},
> +	.probe = ariel_pwrbutton_probe,
> +};
> +module_spi_driver(ariel_pwrbutton_driver);
> +
> +MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>");
> +MODULE_DESCRIPTION("Dell Wyse 3020 Power Button Input Driver");
> +MODULE_LICENSE("Dual BSD/GPL");
> -- 
> 2.28.0
> 

Thanks.

-- 
Dmitry