| Message ID | 1568602373-14164-1-git-send-email-Anson.Huang@nxp.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [V4,1/5] dt-bindings: fsl: scu: add scu key binding | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
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.
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.
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
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
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>;
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(+)