RE: [PATCH V2 2/5] input: keyboard: imx_sc: Add i.MX system controller power key support
From: Anson Huang
Date: Tue Sep 03 2019 - 03:35:19 EST
Hi, Oleksij
> On 03.09.19 08:48, Anson Huang wrote:
> > Hi, Oleksij
> >
> >> On 03.09.19 16:03, 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 power key etc..
> >>>
> >>> Adds i.MX system controller power key driver support, Linux kernel
> >>> has to communicate with system controller via MU (message unit) IPC
> >>> to get power key's status.
> >>>
> >>> Signed-off-by: Anson Huang <Anson.Huang@xxxxxxx>
> >>> ---
> >>> Changes since V1:
> >>> - remove "wakeup-source" property operation, scu power key uses
> >> generic scu irq,
> >>> no need to have this property for device wakeup operation.
> >>> ---
> >>> drivers/input/keyboard/Kconfig | 7 ++
> >>> drivers/input/keyboard/Makefile | 1 +
> >>> drivers/input/keyboard/imx_sc_pwrkey.c | 169
> >> +++++++++++++++++++++++++++++++++
> >>> 3 files changed, 177 insertions(+)
> >>> create mode 100644 drivers/input/keyboard/imx_sc_pwrkey.c
> >>>
> >>> diff --git a/drivers/input/keyboard/Kconfig
> >>> b/drivers/input/keyboard/Kconfig index 2e6d288..3aaeb9c 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_PWRKEY
> >>> + tristate "IMX SCU Power Key Driver"
> >>> + depends on IMX_SCU
> >>> + help
> >>> + This is the system controller powerkey driver for NXP i.MX SoCs with
> >>> + system controller inside.
> >>
> >> The KEY is configurable over devicetree, why is it called PWRKEY? It
> >> looks for me as generic SCU key handler.
> >
> > We always use it as power key, NOT a generic key, as it has HW
> > function designed for power key purpose.
>
> gpio-key driver is mostly used for power or reboot key. And it is still called
> gpio-key driver. If it is used for power key only, why is it configurable? I can
> configure it as KEY_ENTER or some thing different. This driver has not
> KEY_POWER specific any thing.
Understood, I am making the V3 with all "power" removed, just using the "key".
>
> >
> >>
> >>> config KEYBOARD_NEWTON
> >>> tristate "Newton keyboard"
> >>> select SERIO
> >>> diff --git a/drivers/input/keyboard/Makefile
> >>> b/drivers/input/keyboard/Makefile index 9510325..9ea5585 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_PWRKEY) += imx_sc_pwrkey.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_pwrkey.c
> >>> b/drivers/input/keyboard/imx_sc_pwrkey.c
> >>> new file mode 100644
> >>> index 0000000..53aa9a4
> >>> --- /dev/null
> >>> +++ b/drivers/input/keyboard/imx_sc_pwrkey.c
> >>> @@ -0,0 +1,169 @@
> >>> +// 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_pwrkey_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_msg_pwrkey {
> >>> + struct imx_sc_rpc_msg hdr;
> >>> + u8 state;
> >>> +};
> >>> +static struct imx_pwrkey_drv_data *pdata;
> >>
> >> Why is it global struct? It seems to be flexible configurable over devicetree.
> >> So I would assume it should be able to handle more then one button.
> >> Please remove global variables, make it allocatable per OF node.
> >
> > There is ONLY one button available for SC key, but yes, I think I can
> > make the structure private and get all necessary data from the structure
> using container_of.
>
> And we will never need more then 640 kB RAM ;)
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.wi
> kiquote.org%2Fwiki%2FTalk%3ABill_Gates&data=02%7C01%7Canson.hu
> ang%40nxp.com%7C4d4d7458087747e0d8f008d7304057e9%7C686ea1d3bc2
> b4c6fa92cd99c5c301635%7C0%7C0%7C637030925236150243&sdata=w
> %2FGXBaHfnBdLwjTxjbzWSPeIw3ExL%2Fs9IMOgF1onL6A%3D&reserved
> =0
>
> >
> >>
> >> Please use different name "pdata" is usually used as platform data.
> >> Please, use "priv".
> >
> > OK.
> >
> >>
> >>> +static struct imx_sc_ipc *pwrkey_ipc_handle;
> >>
> >> same as before, no global variables.
> >
> > Will move it into private platform data structure.
> >
> >>
> >>> +
> >>> +static int imx_sc_pwrkey_notify(struct notifier_block *nb,
> >>> + unsigned long event, void *group) {
> >>> + if ((event & SC_IRQ_BUTTON) && (*(u8 *)group ==
> >> SC_IRQ_GROUP_WAKE)
> >>> + && !pdata->delay_check) {
> >>> + pdata->delay_check = 1;
> >>> + schedule_delayed_work(&pdata->check_work,
> >>> + msecs_to_jiffies(REPEAT_INTERVAL));
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static void imx_sc_check_for_events(struct work_struct *work) {
> >>> + struct input_dev *input = pdata->input;
> >>> + struct imx_sc_msg_pwrkey msg;
> >>> + struct imx_sc_rpc_msg *hdr = &msg.hdr;
> >>> + bool state;
> >>> +
> >>> + 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;
> >>> +
> >>> + /*
> >>> + * Current SCU firmware does NOT have return value for
> >>> + * this API, that means it is always successful.
> >>> + */
> >>
> >> It is not true for the kernel part:
> >> https://elixir.
> >>
> bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Ffirmware%2Fimx%2F
> >> imx-
> >>
> scu.c%23L157&data=02%7C01%7Canson.huang%40nxp.com%7C7a5ed3
> >>
> ef3b2541e61be808d7303810a9%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
> >>
> 0%7C0%7C637030889669489141&sdata=d3uw6x6WCPeavJu3QYf9o9cxx
> >> Rx4mJar04fQFLF9EhE%3D&reserved=0
> >>
> >> imx_scu_call_rpc() may fail in different ways and provide proper error
> value.
> >> Please use it.
> >
> > There are about 3 APIs are special, this API is one of them, this API
> > has no return value from SCU FW API, but it has response data from it,
> > so that means if we set the response to false, the stack will be free
> > and mailbox will have NULL pointer issue when response data passed
> > from SCU FW. If we set the response to true, as the SCU FW has no
> > return value, the return value will be the msg->func which will be
> > already failed, that is why we have to skip the return value check. This is
> one restriction/bug of SCU FW, we will notify SCU FW owner to fix/improve.
>
> Ok, I see. imx_scu_call_rpc() can return kernel side errors, for example from
> imx-scu.c framework EINVAL or ETIMEDOUT or what ever error mbox
> framework may also provide.
> Aaaannnndd... it can extract an error from SCU package and return it over
> same way as other errors.
>
> And current SCU version has some bugs, so it is providing wrong error value.
> Soo... as usual the NXP has decided to make the linux kernel a bit more
> worse to make the SCU firmware happy? Is it what you trying to describe?
> Really ?! :D
>
> Please. Fix the SCU first. The provide fixed kernel patch.
Understood, I will notify SCU owner to fix it, meanwhile it does NOT block this driver,
I will add return value check in this driver.
Thanks,
Anson