Messages in this thread |  | | Subject | Re: [PATCH V2 2/5] input: keyboard: imx_sc: Add i.MX system controller power key support | From | Oleksij Rempel <> | Date | Tue, 3 Sep 2019 09:43:25 +0200 |
| |
On 03.09.19 09:35, Anson Huang wrote: > 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@nxp.com> >>>>> --- >>>>> 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.
It is great! Thank you!
Kind regards, Oleksij Rempel
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
|  |