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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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&amp;data=02%7C01%7Canson.hu
> ang%40nxp.com%7C4d4d7458087747e0d8f008d7304057e9%7C686ea1d3bc2
> b4c6fa92cd99c5c301635%7C0%7C0%7C637030925236150243&amp;sdata=w
> %2FGXBaHfnBdLwjTxjbzWSPeIw3ExL%2Fs9IMOgF1onL6A%3D&amp;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&amp;data=02%7C01%7Canson.huang%40nxp.com%7C7a5ed3
> >>
> ef3b2541e61be808d7303810a9%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
> >>
> 0%7C0%7C637030889669489141&amp;sdata=d3uw6x6WCPeavJu3QYf9o9cxx
> >> Rx4mJar04fQFLF9EhE%3D&amp;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




[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux