Re: [PATCH v2 4/7] Input: add driver for the input part of qnap-mcu devices

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Heiko,

On Sun, Jul 28, 2024 at 11:17:48PM +0200, Heiko Stuebner wrote:
> The MCU controls the power-button and beeper, so expose them as input
> device. There is of course no interrupt line, so the status of the
> power-button needs to be polled. To generate an event the power-button
> also needs to be held for 1-2 seconds, so the polling interval does
> not need to be overly fast.
> 
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  MAINTAINERS                         |   1 +
>  drivers/input/misc/Kconfig          |  12 +++
>  drivers/input/misc/Makefile         |   1 +
>  drivers/input/misc/qnap-mcu-input.c | 156 ++++++++++++++++++++++++++++
>  4 files changed, 170 insertions(+)
>  create mode 100644 drivers/input/misc/qnap-mcu-input.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f690b55730111..58574f278bfed 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18345,6 +18345,7 @@ F:	drivers/media/tuners/qm1d1c0042*
>  QNAP MCU DRIVER
>  M:	Heiko Stuebner <heiko@sntech.de>
>  S:	Maintained
> +F:	drivers/input/misc/qnap-mcu-input.c
>  F:	drivers/leds/leds-qnap-mcu.c
>  F:	drivers/mfd/qnap-mcu.c
>  F:	include/linux/qnap-mcu.h
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 6ba984d7f0b18..4ab8fe8301635 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -900,6 +900,18 @@ config INPUT_HISI_POWERKEY
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called hisi_powerkey.
>  
> +config INPUT_QNAP_MCU
> +	tristate "Input Support for QNAP MCU controllers"
> +	depends on MFD_QNAP_MCU
> +	help
> +	  This option enables support for input elements available on
> +	  embedded controllers used in QNAP NAS devices.
> +
> +	  This includes a polled power-button as well as a beeper.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called qnap-mcu-input.
> +
>  config INPUT_RAVE_SP_PWRBUTTON
>  	tristate "RAVE SP Power button Driver"
>  	depends on RAVE_SP_CORE
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 04296a4abe8e8..05f5d0072b08f 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -66,6 +66,7 @@ obj-$(CONFIG_INPUT_PMIC8XXX_PWRKEY)	+= pmic8xxx-pwrkey.o
>  obj-$(CONFIG_INPUT_POWERMATE)		+= powermate.o
>  obj-$(CONFIG_INPUT_PWM_BEEPER)		+= pwm-beeper.o
>  obj-$(CONFIG_INPUT_PWM_VIBRA)		+= pwm-vibra.o
> +obj-$(CONFIG_INPUT_QNAP_MCU)		+= qnap-mcu-input.o
>  obj-$(CONFIG_INPUT_RAVE_SP_PWRBUTTON)	+= rave-sp-pwrbutton.o
>  obj-$(CONFIG_INPUT_RB532_BUTTON)	+= rb532_button.o
>  obj-$(CONFIG_INPUT_REGULATOR_HAPTIC)	+= regulator-haptic.o
> diff --git a/drivers/input/misc/qnap-mcu-input.c b/drivers/input/misc/qnap-mcu-input.c
> new file mode 100644
> index 0000000000000..9bac7ea2c6b80
> --- /dev/null
> +++ b/drivers/input/misc/qnap-mcu-input.c
> @@ -0,0 +1,156 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Driver for input events on QNAP-MCUs
> + *
> + * Copyright (C) 2024 Heiko Stuebner <heiko@sntech.de>
> + */
> +
> +#include <linux/input.h>
> +#include <linux/mfd/qnap-mcu.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <uapi/linux/input-event-codes.h>
> +
> +/*
> + * The power-key needs to be pressed for a while to create an event,
> + * so there is no use for overly frequent polling.
> + */
> +#define POLL_INTERVAL		500
> +
> +struct qnap_mcu_input_dev {
> +	struct input_dev *input;
> +	struct qnap_mcu *mcu;
> +	struct device *dev;
> +
> +	struct work_struct beep_work;
> +	int beep_type;
> +};
> +
> +static void qnap_mcu_input_poll(struct input_dev *input)
> +{
> +	struct qnap_mcu_input_dev *idev = input_get_drvdata(input);
> +	u8 cmd[] = {
> +		[0] = 0x40, /* @ */
> +		[1] = 0x43, /* C */
> +		[2] = 0x56  /* V */
> +	};
> +	u8 reply[4];
> +	int state, ret;
> +
> +	/* poll the power button */
> +	ret = qnap_mcu_exec(idev->mcu, cmd, sizeof(cmd), reply, sizeof(reply));
> +	if (ret)
> +		return;
> +
> +	/* First bytes must mirror the sent command */
> +	if (memcmp(cmd, reply, sizeof(cmd))) {
> +		dev_err(idev->dev, "malformed data received\n");
> +		return;
> +	}
> +
> +	state = reply[3] - 0x30;
> +	input_event(input, EV_KEY, KEY_POWER, state);
> +	input_sync(input);
> +}
> +
> +static void qnap_mcu_input_beeper_work(struct work_struct *work)
> +{
> +	struct qnap_mcu_input_dev *idev =
> +		container_of(work, struct qnap_mcu_input_dev, beep_work);
> +	u8 cmd[] = {

Can this be const?

> +		[0] = 0x40, /* @ */
> +		[1] = 0x43, /* C */
> +		[2] = (idev->beep_type == SND_TONE) ? 0x33 : 0x32
> +	};
> +
> +	qnap_mcu_exec_with_ack(idev->mcu, cmd, sizeof(cmd));
> +}
> +
> +static int qnap_mcu_input_event(struct input_dev *input, unsigned int type,
> +				unsigned int code, int value)
> +{
> +	struct qnap_mcu_input_dev *idev = input_get_drvdata(input);
> +
> +	if (type != EV_SND || (code != SND_BELL && code != SND_TONE))
> +		return -EOPNOTSUPP;
> +
> +	if (value < 0)
> +		return -EINVAL;
> +
> +	/* beep runtime is determined by the MCU */
> +	if (value == 0)
> +		return 0;
> +
> +	/* Schedule work to actually turn the beeper on */
> +	idev->beep_type = code;
> +	schedule_work(&idev->beep_work);

I do not see this being canceled anywhere. You should define ->close()
method for the input device and cancel the work there.

> +
> +	return 0;
> +}
> +
> +static int qnap_mcu_input_probe(struct platform_device *pdev)
> +{
> +	struct qnap_mcu *mcu = dev_get_drvdata(pdev->dev.parent);
> +	struct qnap_mcu_input_dev *idev;
> +	struct device *dev = &pdev->dev;
> +	struct input_dev *input;
> +	int ret;
> +
> +	idev = devm_kzalloc(dev, sizeof(*idev), GFP_KERNEL);
> +	if (!idev)
> +		return -ENOMEM;
> +
> +	input = devm_input_allocate_device(dev);
> +	if (!input)
> +		return dev_err_probe(dev, -ENOMEM, "no memory for input device\n");
> +
> +	idev->input = input;
> +	idev->dev = dev;
> +	idev->mcu = mcu;
> +
> +	input_set_drvdata(input, idev);
> +
> +	input->name		= "qnap-mcu";
> +	input->phys		= "qnap-mcu-input/input0";
> +	input->id.bustype	= BUS_HOST;
> +	input->id.vendor	= 0x0001;
> +	input->id.product	= 0x0001;
> +	input->id.version	= 0x0100;
> +	input->event		= qnap_mcu_input_event;
> +
> +	input_set_capability(input, EV_KEY, KEY_POWER);
> +	input_set_capability(input, EV_SND, SND_BELL);
> +	input_set_capability(input, EV_SND, SND_TONE);
> +
> +	INIT_WORK(&idev->beep_work, qnap_mcu_input_beeper_work);
> +
> +	ret = input_setup_polling(input, qnap_mcu_input_poll);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "unable to set up polling\n");
> +
> +	input_set_poll_interval(input, POLL_INTERVAL);
> +
> +	ret = input_register_device(input);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "unable to register input device\n");
> +
> +	qnap_mcu_input_poll(input);
> +	input_sync(input);

Why do you need this here? Either the device will be opened by now (and
will be polled) or there are no listeners and events will be dropped on
the floor...

Thanks.

-- 
Dmitry