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@xxxxx>

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@xxxxx>");
> +MODULE_DESCRIPTION("Dell Wyse 3020 Power Button Input Driver");
> +MODULE_LICENSE("Dual BSD/GPL");
> -- 
> 2.28.0
> 

Thanks.

-- 
Dmitry