[PATCH V4 1/5] dt-bindings: fsl: scu: add scu key binding

[PATCH V4 1/5] dt-bindings: fsl: scu: add scu key binding

[Anson Huang]

NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as
system controller, the system controller is in charge of system
power, clock and scu key event etc. management, Linux kernel has
to communicate with system controller via MU (message unit) IPC
to get scu key event, add binding doc for i.MX system controller
key driver.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No changes.
---
 .../devicetree/bindings/arm/freescale/fsl,scu.txt          | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
index c149fad..5eab7d0 100644
--- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
@@ -157,6 +157,15 @@ Required properties:
 Optional properties:
 - timeout-sec: contains the watchdog timeout in seconds.
 
+SCU key bindings based on SCU Message Protocol
+------------------------------------------------------------
+
+Required properties:
+- compatible: should be:
+              "fsl,imx8qxp-sc-key"
+              followed by "fsl,imx-sc-key";
+- linux,keycodes: See Documentation/devicetree/bindings/input/keys.txt
+
 Example (imx8qxp):
 -------------
 aliases {
@@ -220,6 +229,11 @@ firmware {
 			compatible = "fsl,imx8qxp-sc-rtc";
 		};
 
+		scu_key: scu-key {
+			compatible = "fsl,imx8qxp-sc-key", "fsl,imx-sc-key";
+			linux,keycode = <KEY_POWER>;
+		};
+
 		watchdog {
 			compatible = "fsl,imx8qxp-sc-wdt", "fsl,imx-sc-wdt";
 			timeout-sec = <60>;
-- 
2.7.4

[PATCH V4 2/5] input: keyboard: imx_sc: Add i.MX system controller key support

[Anson Huang]

i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller
inside, the system controller is in charge of controlling power,
clock and scu key etc..

Adds i.MX system controller key driver support, Linux kernel has
to communicate with system controller via MU (message unit) IPC
to get scu key's status.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Changes since V3:
	- switch the debounce and repeat interval time for delay work schdule;
	- add .remove to handle group irq and notify etc..
---
 drivers/input/keyboard/Kconfig      |   7 ++
 drivers/input/keyboard/Makefile     |   1 +
 drivers/input/keyboard/imx_sc_key.c | 190 ++++++++++++++++++++++++++++++++++++
 3 files changed, 198 insertions(+)
 create mode 100644 drivers/input/keyboard/imx_sc_key.c

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index 8911bc2..00f8428 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -469,6 +469,13 @@ config KEYBOARD_IMX
 	  To compile this driver as a module, choose M here: the
 	  module will be called imx_keypad.
 
+config KEYBOARD_IMX_SC_KEY
+	tristate "IMX SCU Key Driver"
+	depends on IMX_SCU
+	help
+	  This is the system controller key driver for NXP i.MX SoCs with
+	  system controller inside.
+
 config KEYBOARD_NEWTON
 	tristate "Newton keyboard"
 	select SERIO
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 9510325..f5b1752 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_KEYBOARD_HIL)		+= hil_kbd.o
 obj-$(CONFIG_KEYBOARD_HIL_OLD)		+= hilkbd.o
 obj-$(CONFIG_KEYBOARD_IPAQ_MICRO)	+= ipaq-micro-keys.o
 obj-$(CONFIG_KEYBOARD_IMX)		+= imx_keypad.o
+obj-$(CONFIG_KEYBOARD_IMX_SC_KEY)	+= imx_sc_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_sc_key.c b/drivers/input/keyboard/imx_sc_key.c
new file mode 100644
index 0000000..59c68fa
--- /dev/null
+++ b/drivers/input/keyboard/imx_sc_key.c
@@ -0,0 +1,190 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/firmware/imx/sci.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+#define DEBOUNCE_TIME	100
+#define REPEAT_INTERVAL	60
+
+#define SC_IRQ_BUTTON		1
+#define SC_IRQ_GROUP_WAKE	3
+#define IMX_SC_MISC_FUNC_GET_BUTTON_STATUS	18
+
+struct imx_key_drv_data {
+	int keycode;
+	bool keystate;  /* 1: pressed, 0: release */
+	bool delay_check;
+	struct delayed_work check_work;
+	struct input_dev *input;
+	struct imx_sc_ipc *key_ipc_handle;
+	struct notifier_block key_notifier;
+};
+
+struct imx_sc_msg_key {
+	struct imx_sc_rpc_msg hdr;
+	u8 state;
+};
+
+static int imx_sc_key_notify(struct notifier_block *nb,
+			     unsigned long event, void *group)
+{
+	struct imx_key_drv_data *priv =
+				 container_of(nb,
+					      struct imx_key_drv_data,
+					      key_notifier);
+
+	if ((event & SC_IRQ_BUTTON) && (*(u8 *)group == SC_IRQ_GROUP_WAKE)
+	    && !priv->delay_check) {
+		priv->delay_check = 1;
+		schedule_delayed_work(&priv->check_work,
+				      msecs_to_jiffies(DEBOUNCE_TIME));
+	}
+
+	return 0;
+}
+
+static void imx_sc_check_for_events(struct work_struct *work)
+{
+	struct imx_key_drv_data *priv =
+				 container_of(work,
+					      struct imx_key_drv_data,
+					      check_work.work);
+	struct input_dev *input = priv->input;
+	struct imx_sc_msg_key msg;
+	struct imx_sc_rpc_msg *hdr = &msg.hdr;
+	bool state;
+	int ret;
+
+	hdr->ver = IMX_SC_RPC_VERSION;
+	hdr->svc = IMX_SC_RPC_SVC_MISC;
+	hdr->func = IMX_SC_MISC_FUNC_GET_BUTTON_STATUS;
+	hdr->size = 1;
+
+	ret = imx_scu_call_rpc(priv->key_ipc_handle, &msg, true);
+	if (ret) {
+		dev_err(&input->dev, "read imx sc key failed, ret %d\n", ret);
+		return;
+	}
+
+	state = (bool)msg.state;
+
+	if (!state && !priv->keystate)
+		state = true;
+
+	if (state ^ priv->keystate) {
+		pm_wakeup_event(input->dev.parent, 0);
+		priv->keystate = state;
+		input_event(input, EV_KEY, priv->keycode, state);
+		input_sync(input);
+		if (!state)
+			priv->delay_check = 0;
+		pm_relax(priv->input->dev.parent);
+	}
+
+	if (state)
+		schedule_delayed_work(&priv->check_work,
+				      msecs_to_jiffies(REPEAT_INTERVAL));
+}
+
+static int imx_sc_key_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	static struct imx_key_drv_data *priv;
+	struct input_dev *input;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	ret = imx_scu_get_handle(&priv->key_ipc_handle);
+	if (ret)
+		return ret;
+
+	if (of_property_read_u32(np, "linux,keycode", &priv->keycode)) {
+		dev_err(&pdev->dev, "missing KEY_POWER in DT\n");
+		return -EINVAL;
+	}
+
+	INIT_DELAYED_WORK(&priv->check_work, imx_sc_check_for_events);
+
+	input = devm_input_allocate_device(&pdev->dev);
+	if (!input) {
+		dev_err(&pdev->dev, "failed to allocate the input device\n");
+		return -ENOMEM;
+	}
+
+	input->name = pdev->name;
+	input->phys = "imx-sc-key/input0";
+	input->id.bustype = BUS_HOST;
+
+	input_set_capability(input, EV_KEY, priv->keycode);
+
+	ret = input_register_device(input);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register input device\n");
+		return ret;
+	}
+
+	priv->input = input;
+	platform_set_drvdata(pdev, priv);
+
+	ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WAKE, SC_IRQ_BUTTON, true);
+	if (ret) {
+		dev_warn(&pdev->dev, "enable scu group irq failed\n");
+		return ret;
+	}
+
+	priv->key_notifier.notifier_call = imx_sc_key_notify;
+	ret = imx_scu_irq_register_notifier(&priv->key_notifier);
+	if (ret) {
+		imx_scu_irq_group_enable(SC_IRQ_GROUP_WAKE, SC_IRQ_BUTTON, false);
+		dev_warn(&pdev->dev, "register scu notifier failed\n");
+	}
+
+	return ret;
+}
+
+static int imx_sc_key_remove(struct platform_device *pdev)
+{
+	struct imx_key_drv_data *priv = platform_get_drvdata(pdev);
+
+	imx_scu_irq_group_enable(SC_IRQ_GROUP_WAKE, SC_IRQ_BUTTON, false);
+	imx_scu_irq_unregister_notifier(&priv->key_notifier);
+	cancel_delayed_work_sync(&priv->check_work);
+
+	return 0;
+}
+
+static const struct of_device_id imx_sc_key_ids[] = {
+	{ .compatible = "fsl,imx-sc-key" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx_sc_key_ids);
+
+static struct platform_driver imx_sc_key_driver = {
+	.driver = {
+		.name = "imx-sc-key",
+		.of_match_table = imx_sc_key_ids,
+	},
+	.probe = imx_sc_key_probe,
+	.remove = imx_sc_key_remove,
+};
+module_platform_driver(imx_sc_key_driver);
+
+MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
+MODULE_DESCRIPTION("i.MX System Controller Key Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCH V4 3/5] arm64: dts: imx8qxp: Add scu key node

[Anson Huang]

Add scu key node for i.MX8QXP, disabled by default as it
depends on board design.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No changes.
---
 arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
index 1133b41..71e38c1 100644
--- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
@@ -8,6 +8,7 @@
 #include <dt-bindings/clock/imx8-clock.h>
 #include <dt-bindings/firmware/imx/rsrc.h>
 #include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/input.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/pinctrl/pads-imx8qxp.h>
 
@@ -174,6 +175,12 @@
 			#power-domain-cells = <1>;
 		};
 
+		scu_key: scu-key {
+			compatible = "fsl,imx8qxp-sc-key", "fsl,imx-sc-key";
+			linux,keycode = <KEY_POWER>;
+			status = "disabled";
+		};
+
 		rtc: rtc {
 			compatible = "fsl,imx8qxp-sc-rtc";
 		};
-- 
2.7.4

[PATCH V4 4/5] arm64: dts: imx8qxp-mek: Enable scu key

[Anson Huang]

Enable scu key for i.MX8QXP MEK board.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No changes.
---
 arch/arm64/boot/dts/freescale/imx8qxp-mek.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts b/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts
index 1946805..88dd9132 100644
--- a/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts
+++ b/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts
@@ -234,3 +234,7 @@
 &adma_dsp {
 	status = "okay";
 };
+
+&scu_key {
+	status = "okay";
+};
-- 
2.7.4

[PATCH V4 5/5] arm64: defconfig: Enable CONFIG_KEYBOARD_IMX_SC_KEY as module

[Anson Huang]

Select CONFIG_KEYBOARD_IMX_SC_KEY as module by default to
support i.MX8QXP scu key driver.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
No changes.
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 0a70e10..9c83014 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -314,6 +314,7 @@ CONFIG_INPUT_EVDEV=y
 CONFIG_KEYBOARD_ADC=m
 CONFIG_KEYBOARD_GPIO=y
 CONFIG_KEYBOARD_SNVS_PWRKEY=m
+CONFIG_KEYBOARD_IMX_SC_KEY=m
 CONFIG_KEYBOARD_CROS_EC=y
 CONFIG_INPUT_TOUCHSCREEN=y
 CONFIG_TOUCHSCREEN_ATMEL_MXT=m
-- 
2.7.4

Re: [PATCH V4 2/5] input: keyboard: imx_sc: Add i.MX system controller key support

[Dmitry Torokhov]

On Mon, Sep 16, 2019 at 10:52:50AM +0800, Anson Huang wrote:
> i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller
> inside, the system controller is in charge of controlling power,
> clock and scu key etc..
> 
> Adds i.MX system controller key driver support, Linux kernel has
> to communicate with system controller via MU (message unit) IPC
> to get scu key's status.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> Changes since V3:
> 	- switch the debounce and repeat interval time for delay work schdule;
> 	- add .remove to handle group irq and notify etc..
> ---
>  drivers/input/keyboard/Kconfig      |   7 ++
>  drivers/input/keyboard/Makefile     |   1 +
>  drivers/input/keyboard/imx_sc_key.c | 190 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 198 insertions(+)
>  create mode 100644 drivers/input/keyboard/imx_sc_key.c
> 
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 8911bc2..00f8428 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -469,6 +469,13 @@ config KEYBOARD_IMX
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called imx_keypad.
>  
> +config KEYBOARD_IMX_SC_KEY
> +	tristate "IMX SCU Key Driver"
> +	depends on IMX_SCU
> +	help
> +	  This is the system controller key driver for NXP i.MX SoCs with
> +	  system controller inside.
> +
>  config KEYBOARD_NEWTON
>  	tristate "Newton keyboard"
>  	select SERIO
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 9510325..f5b1752 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_KEYBOARD_HIL)		+= hil_kbd.o
>  obj-$(CONFIG_KEYBOARD_HIL_OLD)		+= hilkbd.o
>  obj-$(CONFIG_KEYBOARD_IPAQ_MICRO)	+= ipaq-micro-keys.o
>  obj-$(CONFIG_KEYBOARD_IMX)		+= imx_keypad.o
> +obj-$(CONFIG_KEYBOARD_IMX_SC_KEY)	+= imx_sc_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_sc_key.c b/drivers/input/keyboard/imx_sc_key.c
> new file mode 100644
> index 0000000..59c68fa
> --- /dev/null
> +++ b/drivers/input/keyboard/imx_sc_key.c
> @@ -0,0 +1,190 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2019 NXP.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/firmware/imx/sci.h>
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/jiffies.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +
> +#define DEBOUNCE_TIME	100
> +#define REPEAT_INTERVAL	60
> +
> +#define SC_IRQ_BUTTON		1
> +#define SC_IRQ_GROUP_WAKE	3
> +#define IMX_SC_MISC_FUNC_GET_BUTTON_STATUS	18
> +
> +struct imx_key_drv_data {
> +	int keycode;
> +	bool keystate;  /* 1: pressed, 0: release */
> +	bool delay_check;
> +	struct delayed_work check_work;
> +	struct input_dev *input;
> +	struct imx_sc_ipc *key_ipc_handle;
> +	struct notifier_block key_notifier;
> +};
> +
> +struct imx_sc_msg_key {
> +	struct imx_sc_rpc_msg hdr;
> +	u8 state;
> +};
> +
> +static int imx_sc_key_notify(struct notifier_block *nb,
> +			     unsigned long event, void *group)
> +{
> +	struct imx_key_drv_data *priv =
> +				 container_of(nb,
> +					      struct imx_key_drv_data,
> +					      key_notifier);
> +
> +	if ((event & SC_IRQ_BUTTON) && (*(u8 *)group == SC_IRQ_GROUP_WAKE)
> +	    && !priv->delay_check) {
> +		priv->delay_check = 1;
> +		schedule_delayed_work(&priv->check_work,
> +				      msecs_to_jiffies(DEBOUNCE_TIME));

If I am reading this right, you are trying to avoid scheduling the work
again if it is already scheduled. You do not need to do that, as
schedule_delayed_work() will take care of that (if you want to make sure
the work is re-scheduled with updated expiration, you need to use
mod_delayed_work).

> +	}
> +
> +	return 0;
> +}
> +
> +static void imx_sc_check_for_events(struct work_struct *work)
> +{
> +	struct imx_key_drv_data *priv =
> +				 container_of(work,
> +					      struct imx_key_drv_data,
> +					      check_work.work);
> +	struct input_dev *input = priv->input;
> +	struct imx_sc_msg_key msg;
> +	struct imx_sc_rpc_msg *hdr = &msg.hdr;
> +	bool state;
> +	int ret;
> +
> +	hdr->ver = IMX_SC_RPC_VERSION;
> +	hdr->svc = IMX_SC_RPC_SVC_MISC;
> +	hdr->func = IMX_SC_MISC_FUNC_GET_BUTTON_STATUS;
> +	hdr->size = 1;
> +
> +	ret = imx_scu_call_rpc(priv->key_ipc_handle, &msg, true);
> +	if (ret) {
> +		dev_err(&input->dev, "read imx sc key failed, ret %d\n", ret);
> +		return;
> +	}
> +
> +	state = (bool)msg.state;
> +
> +	if (!state && !priv->keystate)
> +		state = true;

This needs an explanation please.

> +
> +	if (state ^ priv->keystate) {
> +		pm_wakeup_event(input->dev.parent, 0);

I'd expect this happening in imx_sc_key_notify() so that the device
would have a change to run this work.

> +		priv->keystate = state;
> +		input_event(input, EV_KEY, priv->keycode, state);
> +		input_sync(input);
> +		if (!state)
> +			priv->delay_check = 0;
> +		pm_relax(priv->input->dev.parent);

Are you sure you want to call pm_relax() unconditionally, and not when
state == false (i.e. button released)?

> +	}
> +
> +	if (state)
> +		schedule_delayed_work(&priv->check_work,
> +				      msecs_to_jiffies(REPEAT_INTERVAL));
> +}
> +
> +static int imx_sc_key_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	static struct imx_key_drv_data *priv;
> +	struct input_dev *input;
> +	int ret;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	ret = imx_scu_get_handle(&priv->key_ipc_handle);
> +	if (ret)
> +		return ret;
> +
> +	if (of_property_read_u32(np, "linux,keycode", &priv->keycode)) {
> +		dev_err(&pdev->dev, "missing KEY_POWER in DT\n");
> +		return -EINVAL;
> +	}
> +
> +	INIT_DELAYED_WORK(&priv->check_work, imx_sc_check_for_events);
> +
> +	input = devm_input_allocate_device(&pdev->dev);
> +	if (!input) {
> +		dev_err(&pdev->dev, "failed to allocate the input device\n");
> +		return -ENOMEM;
> +	}
> +
> +	input->name = pdev->name;
> +	input->phys = "imx-sc-key/input0";
> +	input->id.bustype = BUS_HOST;
> +
> +	input_set_capability(input, EV_KEY, priv->keycode);
> +
> +	ret = input_register_device(input);
> +	if (ret) {

Could you please rename this (and elsewhere) from 'ret' to 'error'?

> +		dev_err(&pdev->dev, "failed to register input device\n");
> +		return ret;
> +	}
> +
> +	priv->input = input;
> +	platform_set_drvdata(pdev, priv);
> +
> +	ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WAKE, SC_IRQ_BUTTON, true);
> +	if (ret) {
> +		dev_warn(&pdev->dev, "enable scu group irq failed\n");
> +		return ret;
> +	}
> +
> +	priv->key_notifier.notifier_call = imx_sc_key_notify;
> +	ret = imx_scu_irq_register_notifier(&priv->key_notifier);
> +	if (ret) {
> +		imx_scu_irq_group_enable(SC_IRQ_GROUP_WAKE, SC_IRQ_BUTTON, false);
> +		dev_warn(&pdev->dev, "register scu notifier failed\n");

		return error;
> +	}
> +
> +	return ret;

	return 0;
> +}
> +
> +static int imx_sc_key_remove(struct platform_device *pdev)
> +{
> +	struct imx_key_drv_data *priv = platform_get_drvdata(pdev);
> +
> +	imx_scu_irq_group_enable(SC_IRQ_GROUP_WAKE, SC_IRQ_BUTTON, false);
> +	imx_scu_irq_unregister_notifier(&priv->key_notifier);
> +	cancel_delayed_work_sync(&priv->check_work);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id imx_sc_key_ids[] = {
> +	{ .compatible = "fsl,imx-sc-key" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx_sc_key_ids);
> +
> +static struct platform_driver imx_sc_key_driver = {
> +	.driver = {
> +		.name = "imx-sc-key",
> +		.of_match_table = imx_sc_key_ids,
> +	},
> +	.probe = imx_sc_key_probe,
> +	.remove = imx_sc_key_remove,
> +};
> +module_platform_driver(imx_sc_key_driver);
> +
> +MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
> +MODULE_DESCRIPTION("i.MX System Controller Key Driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.7.4
> 

Thanks.

-- 
Dmitry

RE: [PATCH V4 2/5] input: keyboard: imx_sc: Add i.MX system controller key support

[Anson Huang]

Hi, Dmitry

> On Mon, Sep 16, 2019 at 10:52:50AM +0800, Anson Huang wrote:
> > i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller
> > inside, the system controller is in charge of controlling power, clock
> > and scu key etc..
> >
> > Adds i.MX system controller key driver support, Linux kernel has to
> > communicate with system controller via MU (message unit) IPC to get
> > scu key's status.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > Changes since V3:
> > 	- switch the debounce and repeat interval time for delay work
> schdule;
> > 	- add .remove to handle group irq and notify etc..
> > ---
> >  drivers/input/keyboard/Kconfig      |   7 ++
> >  drivers/input/keyboard/Makefile     |   1 +
> >  drivers/input/keyboard/imx_sc_key.c | 190
> > ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 198 insertions(+)
> >  create mode 100644 drivers/input/keyboard/imx_sc_key.c
> >
> > diff --git a/drivers/input/keyboard/Kconfig
> > b/drivers/input/keyboard/Kconfig index 8911bc2..00f8428 100644
> > --- a/drivers/input/keyboard/Kconfig
> > +++ b/drivers/input/keyboard/Kconfig
> > @@ -469,6 +469,13 @@ config KEYBOARD_IMX
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called imx_keypad.
> >
> > +config KEYBOARD_IMX_SC_KEY
> > +	tristate "IMX SCU Key Driver"
> > +	depends on IMX_SCU
> > +	help
> > +	  This is the system controller key driver for NXP i.MX SoCs with
> > +	  system controller inside.
> > +
> >  config KEYBOARD_NEWTON
> >  	tristate "Newton keyboard"
> >  	select SERIO
> > diff --git a/drivers/input/keyboard/Makefile
> > b/drivers/input/keyboard/Makefile index 9510325..f5b1752 100644
> > --- a/drivers/input/keyboard/Makefile
> > +++ b/drivers/input/keyboard/Makefile
> > @@ -29,6 +29,7 @@ obj-$(CONFIG_KEYBOARD_HIL)		+= hil_kbd.o
> >  obj-$(CONFIG_KEYBOARD_HIL_OLD)		+= hilkbd.o
> >  obj-$(CONFIG_KEYBOARD_IPAQ_MICRO)	+= ipaq-micro-keys.o
> >  obj-$(CONFIG_KEYBOARD_IMX)		+= imx_keypad.o
> > +obj-$(CONFIG_KEYBOARD_IMX_SC_KEY)	+= imx_sc_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_sc_key.c
> > b/drivers/input/keyboard/imx_sc_key.c
> > new file mode 100644
> > index 0000000..59c68fa
> > --- /dev/null
> > +++ b/drivers/input/keyboard/imx_sc_key.c
> > @@ -0,0 +1,190 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2019 NXP.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/firmware/imx/sci.h>
> > +#include <linux/init.h>
> > +#include <linux/input.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define DEBOUNCE_TIME	100
> > +#define REPEAT_INTERVAL	60
> > +
> > +#define SC_IRQ_BUTTON		1
> > +#define SC_IRQ_GROUP_WAKE	3
> > +#define IMX_SC_MISC_FUNC_GET_BUTTON_STATUS	18
> > +
> > +struct imx_key_drv_data {
> > +	int keycode;
> > +	bool keystate;  /* 1: pressed, 0: release */
> > +	bool delay_check;
> > +	struct delayed_work check_work;
> > +	struct input_dev *input;
> > +	struct imx_sc_ipc *key_ipc_handle;
> > +	struct notifier_block key_notifier;
> > +};
> > +
> > +struct imx_sc_msg_key {
> > +	struct imx_sc_rpc_msg hdr;
> > +	u8 state;
> > +};
> > +
> > +static int imx_sc_key_notify(struct notifier_block *nb,
> > +			     unsigned long event, void *group) {
> > +	struct imx_key_drv_data *priv =
> > +				 container_of(nb,
> > +					      struct imx_key_drv_data,
> > +					      key_notifier);
> > +
> > +	if ((event & SC_IRQ_BUTTON) && (*(u8 *)group ==
> SC_IRQ_GROUP_WAKE)
> > +	    && !priv->delay_check) {
> > +		priv->delay_check = 1;
> > +		schedule_delayed_work(&priv->check_work,
> > +				      msecs_to_jiffies(DEBOUNCE_TIME));
> 
> If I am reading this right, you are trying to avoid scheduling the work again if
> it is already scheduled. You do not need to do that, as
> schedule_delayed_work() will take care of that (if you want to make sure the
> work is re-scheduled with updated expiration, you need to use
> mod_delayed_work).

The original code here is to skip the event of button release, try to ONLY handle the press interrupt, the release
event is handled by busy loop check, but I double check the SCU FW's behavior, it does NOT send out interrupt
for button release event at all, so it is NOT needed any more, I will remove the priv->delay_check completely in this driver.

> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void imx_sc_check_for_events(struct work_struct *work) {
> > +	struct imx_key_drv_data *priv =
> > +				 container_of(work,
> > +					      struct imx_key_drv_data,
> > +					      check_work.work);
> > +	struct input_dev *input = priv->input;
> > +	struct imx_sc_msg_key msg;
> > +	struct imx_sc_rpc_msg *hdr = &msg.hdr;
> > +	bool state;
> > +	int ret;
> > +
> > +	hdr->ver = IMX_SC_RPC_VERSION;
> > +	hdr->svc = IMX_SC_RPC_SVC_MISC;
> > +	hdr->func = IMX_SC_MISC_FUNC_GET_BUTTON_STATUS;
> > +	hdr->size = 1;
> > +
> > +	ret = imx_scu_call_rpc(priv->key_ipc_handle, &msg, true);
> > +	if (ret) {
> > +		dev_err(&input->dev, "read imx sc key failed, ret %d\n", ret);
> > +		return;
> > +	}
> > +
> > +	state = (bool)msg.state;
> > +
> > +	if (!state && !priv->keystate)
> > +		state = true;
> 
> This needs an explanation please.

This is to handle the quick press of button, e.g., when button is pressed and released very quickly,
when the delay work is scheduled, the button state read from SCU FW is actually a release state (0),
the press state is (1), so the quick press/release will be ignored.

However, after double check and test, I think this should be handled by debounce time, if the button is pressed/release
very quickly, the event should be ignored, I will remove it and reduce the debounce time to 30mS, previous 100mS
is too long, using 30mS as debounce time, I did NOT see similar issue no matter how quick I press/release the button.

> 
> > +
> > +	if (state ^ priv->keystate) {
> > +		pm_wakeup_event(input->dev.parent, 0);
> 
> I'd expect this happening in imx_sc_key_notify() so that the device would
> have a change to run this work.

Agreed, will move it to the imx_sc_key_notify().

> 
> > +		priv->keystate = state;
> > +		input_event(input, EV_KEY, priv->keycode, state);
> > +		input_sync(input);
> > +		if (!state)
> > +			priv->delay_check = 0;
> > +		pm_relax(priv->input->dev.parent);
> 
> Are you sure you want to call pm_relax() unconditionally, and not when state
> == false (i.e. button released)?

Agreed, will make sure ONLY call it when button is released which indicates the event
is finished.

> 
> > +	}
> > +
> > +	if (state)
> > +		schedule_delayed_work(&priv->check_work,
> > +				      msecs_to_jiffies(REPEAT_INTERVAL));
> > +}
> > +
> > +static int imx_sc_key_probe(struct platform_device *pdev) {
> > +	struct device_node *np = pdev->dev.of_node;
> > +	static struct imx_key_drv_data *priv;
> > +	struct input_dev *input;
> > +	int ret;
> > +
> > +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	ret = imx_scu_get_handle(&priv->key_ipc_handle);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (of_property_read_u32(np, "linux,keycode", &priv->keycode)) {
> > +		dev_err(&pdev->dev, "missing KEY_POWER in DT\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	INIT_DELAYED_WORK(&priv->check_work,
> imx_sc_check_for_events);
> > +
> > +	input = devm_input_allocate_device(&pdev->dev);
> > +	if (!input) {
> > +		dev_err(&pdev->dev, "failed to allocate the input device\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	input->name = pdev->name;
> > +	input->phys = "imx-sc-key/input0";
> > +	input->id.bustype = BUS_HOST;
> > +
> > +	input_set_capability(input, EV_KEY, priv->keycode);
> > +
> > +	ret = input_register_device(input);
> > +	if (ret) {
> 
> Could you please rename this (and elsewhere) from 'ret' to 'error'?

OK.

> 
> > +		dev_err(&pdev->dev, "failed to register input device\n");
> > +		return ret;
> > +	}
> > +
> > +	priv->input = input;
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WAKE,
> SC_IRQ_BUTTON, true);
> > +	if (ret) {
> > +		dev_warn(&pdev->dev, "enable scu group irq failed\n");
> > +		return ret;
> > +	}
> > +
> > +	priv->key_notifier.notifier_call = imx_sc_key_notify;
> > +	ret = imx_scu_irq_register_notifier(&priv->key_notifier);
> > +	if (ret) {
> > +		imx_scu_irq_group_enable(SC_IRQ_GROUP_WAKE,
> SC_IRQ_BUTTON, false);
> > +		dev_warn(&pdev->dev, "register scu notifier failed\n");
> 
> 		return error;
> > +	}
> > +
> > +	return ret;
> 
> 	return 0;

OK.

Thanks,
Anson.

Re: [PATCH V4 2/5] input: keyboard: imx_sc: Add i.MX system controller key support

[Pavel Machek]

Hi!

> > > +	ret = imx_scu_call_rpc(priv->key_ipc_handle, &msg, true);
> > > +	if (ret) {
> > > +		dev_err(&input->dev, "read imx sc key failed, ret %d\n", ret);
> > > +		return;
> > > +	}
> > > +
> > > +	state = (bool)msg.state;
> > > +
> > > +	if (!state && !priv->keystate)
> > > +		state = true;
> > 
> > This needs an explanation please.
> 
> This is to handle the quick press of button, e.g., when button is pressed and released very quickly,
> when the delay work is scheduled, the button state read from SCU FW is actually a release state (0),
> the press state is (1), so the quick press/release will be ignored.
> 
> However, after double check and test, I think this should be handled by debounce time, if the button is pressed/release
> very quickly, the event should be ignored, I will remove it and reduce the debounce time to 30mS, previous 100mS
> is too long, using 30mS as debounce time, I did NOT see similar issue no matter how quick I press/release the button.

Are you sure this is expected behaviour?

AFAIK microswitches can bounce when the button is pressed and released, but will not generate
glitches when the button was not pressed, so even short presses are real and should be
propagated...

Best regards,
										Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

RE: [PATCH V4 2/5] input: keyboard: imx_sc: Add i.MX system controller key support

[Anson Huang]

Hi, Pavel

> Subject: Re: [PATCH V4 2/5] input: keyboard: imx_sc: Add i.MX system
> controller key support
> 
> Hi!
> 
> > > > +	ret = imx_scu_call_rpc(priv->key_ipc_handle, &msg, true);
> > > > +	if (ret) {
> > > > +		dev_err(&input->dev, "read imx sc key failed, ret %d\n", ret);
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	state = (bool)msg.state;
> > > > +
> > > > +	if (!state && !priv->keystate)
> > > > +		state = true;
> > >
> > > This needs an explanation please.
> >
> > This is to handle the quick press of button, e.g., when button is
> > pressed and released very quickly, when the delay work is scheduled,
> > the button state read from SCU FW is actually a release state (0), the press
> state is (1), so the quick press/release will be ignored.
> >
> > However, after double check and test, I think this should be handled
> > by debounce time, if the button is pressed/release very quickly, the
> > event should be ignored, I will remove it and reduce the debounce time to
> 30mS, previous 100mS is too long, using 30mS as debounce time, I did NOT
> see similar issue no matter how quick I press/release the button.
> 
> Are you sure this is expected behaviour?
> 
> AFAIK microswitches can bounce when the button is pressed and released,
> but will not generate glitches when the button was not pressed, so even
> short presses are real and should be propagated...

Latest version of patch handle the quick press as below:

- When the button is pressed, the delayed work will be scheduled;
- In the delayed work, the button state will be read, once the state is different as previous
  state, it will be propagated;
- If the state is a press state, another delayed work will be scheduled for handling release
  event.

So there could be a small window of DEBOUNCE_TIME (30mS) between button press and the delayed
work scheduled for reading the button state, if the button is released in less than 30ms, then the
state read could be a release state which is same as previous state and it will be ignored.

Do you think in this case, we should also propagated the press event? If yes, then what about the
release event? Although from what I tested, no matter how quick I press the button, the driver
can always receive both the press and release event, as the 30mS is very short.

Thanks,
Anson