Re: [PATCH v5 7/7] input: keyboard: support i.MX95 BBM module

Cristian Marussi
2024-07-11T14:02:01Z

On Fri, Jun 21, 2024 at 03:04:42PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> The BBM module provides BUTTON feature. To i.MX95, this module
> is managed by System Manager and exported using System Management
> Control Interface(SCMI). Linux could use i.MX SCMI BBM Extension
> protocol to use BUTTON feature.

Hi Peng,

one remarks down below.

>
> This driver is to use SCMI interface to enable pwrkey.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> drivers/input/keyboard/Kconfig | 11 ++
> drivers/input/keyboard/Makefile | 1 +
> drivers/input/keyboard/imx-sm-bbm-key.c | 225 ++++++++++++++++++++++++++++++++
> 3 files changed, 237 insertions(+)
>
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 1d0c5f4c0f99..1c3fef7d34af 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -466,6 +466,17 @@ config KEYBOARD_IMX
> To compile this driver as a module, choose M here: the
> module will be called imx_keypad.
>
> +config KEYBOARD_IMX_BBM_SCMI
> + tristate "IMX BBM SCMI Key Driver"
> + depends on IMX_SCMI_BBM_EXT || COMPILE_TEST
> + default y if ARCH_MXC
> + help
> + This is the BBM key driver for NXP i.MX SoCs managed through
> + SCMI protocol.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called scmi-imx-bbm-key.
> +
> config KEYBOARD_IMX_SC_KEY
> tristate "IMX SCU Key Driver"
> depends on IMX_SCU
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index aecef00c5d09..624c90adde89 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -31,6 +31,7 @@ obj-$(CONFIG_KEYBOARD_IPAQ_MICRO) += ipaq-micro-keys.o
> obj-$(CONFIG_KEYBOARD_IQS62X) += iqs62x-keys.o
> obj-$(CONFIG_KEYBOARD_IMX) += imx_keypad.o
> obj-$(CONFIG_KEYBOARD_IMX_SC_KEY) += imx_sc_key.o
> +obj-$(CONFIG_KEYBOARD_IMX_BBM_SCMI) += imx-sm-bbm-key.o
> obj-$(CONFIG_KEYBOARD_HP6XX) += jornada680_kbd.o
> obj-$(CONFIG_KEYBOARD_HP7XX) += jornada720_kbd.o
> obj-$(CONFIG_KEYBOARD_LKKBD) += lkkbd.o
> diff --git a/drivers/input/keyboard/imx-sm-bbm-key.c b/drivers/input/keyboard/imx-sm-bbm-key.c
> new file mode 100644
> index 000000000000..907dad383b8f
> --- /dev/null
> +++ b/drivers/input/keyboard/imx-sm-bbm-key.c
> @@ -0,0 +1,225 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2024 NXP.
> + */
> +
> +#include <linux/input.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/rtc.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/scmi_imx_protocol.h>
> +#include <linux/suspend.h>
> +
> +#define DEBOUNCE_TIME 30
> +#define REPEAT_INTERVAL 60
> +
> +struct scmi_imx_bbm {
> + struct scmi_protocol_handle *ph;
> + const struct scmi_imx_bbm_proto_ops *ops;
> + struct notifier_block nb;
> + int keycode;
> + int keystate; /* 1:pressed */
> + bool suspended;
> + struct delayed_work check_work;
> + struct input_dev *input;
> +};
> +
> +static void scmi_imx_bbm_pwrkey_check_for_events(struct work_struct *work)
> +{
> + struct scmi_imx_bbm *bbnsm = container_of(to_delayed_work(work),
> + struct scmi_imx_bbm, check_work);
> + struct scmi_protocol_handle *ph = bbnsm->ph;
> + struct input_dev *input = bbnsm->input;
> + u32 state = 0;
> + int ret;
> +
> + ret = bbnsm->ops->button_get(ph, &state);
> + if (ret) {
> + pr_err("%s: %d\n", __func__, ret);
> + return;
> + }
> +
> + pr_debug("%s: state: %d, keystate %d\n", __func__, state, bbnsm->keystate);
> +
> + /* only report new event if status changed */
> + if (state ^ bbnsm->keystate) {
> + bbnsm->keystate = state;
> + input_event(input, EV_KEY, bbnsm->keycode, state);
> + input_sync(input);
> + pm_relax(bbnsm->input->dev.parent);
> + pr_debug("EV_KEY: %x\n", bbnsm->keycode);
> + }
> +
> + /* repeat check if pressed long */
> + if (state)
> + schedule_delayed_work(&bbnsm->check_work, msecs_to_jiffies(REPEAT_INTERVAL));
> +}
> +
> +static int scmi_imx_bbm_pwrkey_event(struct scmi_imx_bbm *bbnsm)
> +{
> + struct input_dev *input = bbnsm->input;
> +
> + pm_wakeup_event(input->dev.parent, 0);
> +
> + schedule_delayed_work(&bbnsm->check_work, msecs_to_jiffies(DEBOUNCE_TIME));
> +
> + /*
> + * Directly report key event after resume to make no key press
> + * event is missed.
> + */
> + if (READ_ONCE(bbnsm->suspended)) {
> + bbnsm->keystate = 1;
> + input_event(input, EV_KEY, bbnsm->keycode, 1);
> + input_sync(input);
> + WRITE_ONCE(bbnsm->suspended, false);
> + }
> +
> + return 0;
> +}
> +
> +static void scmi_imx_bbm_pwrkey_act(void *pdata)
> +{
> + struct scmi_imx_bbm *bbnsm = pdata;
> +
> + cancel_delayed_work_sync(&bbnsm->check_work);
> +}
> +
> +static int scmi_imx_bbm_key_notifier(struct notifier_block *nb, unsigned long event, void *data)
> +{
> + struct scmi_imx_bbm *bbnsm = container_of(nb, struct scmi_imx_bbm, nb);
> + struct scmi_imx_bbm_notif_report *r = data;
> +
> + if (r->is_button) {
> + pr_debug("BBM Button Power key pressed\n");
> + scmi_imx_bbm_pwrkey_event(bbnsm);
> + } else {
> + /* Should never reach here */
> + pr_err("Unexpected BBM event: %s\n", __func__);
> + }
> +
> + return 0;
> +}
> +
> +static int scmi_imx_bbm_pwrkey_init(struct scmi_device *sdev)
> +{
> + const struct scmi_handle *handle = sdev->handle;
> + struct device *dev = &sdev->dev;
> + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev);
> + struct input_dev *input;
> + int ret;
> +
> + if (device_property_read_u32(dev, "linux,code", &bbnsm->keycode)) {
> + bbnsm->keycode = KEY_POWER;
> + dev_warn(dev, "key code is not specified, using default KEY_POWER\n");
> + }
> +
> + INIT_DELAYED_WORK(&bbnsm->check_work, scmi_imx_bbm_pwrkey_check_for_events);

To stay on the safe side I would issue a cancel_delayed_work_sync()
somewhere along the remove() path of this driver...just to be sure
there is not an actively scheduled deleayed work queued while we are
shutting down.....I maybe overly paranoic...but seems a safe thing to do O_o

Thanks,
Cristian