On Wed, May 13, 2015 at 11:40 AM, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote: > Hi Frank, > > On Wed, May 13, 2015 at 10:47:40PM +0800, Frank.Li@xxxxxxxxxxxxx wrote: >> From: Robin Gong <b38343@xxxxxxxxxxxxx> >> >> add snvs power key driver. >> It work in imx chips after i.mx6sx >> >> Signed-off-by: Robin Gong <b38343@xxxxxxxxxxxxx> >> Signed-off-by: Frank Li <Frank.Li@xxxxxxxxxxxxx> >> --- >> Change from V1 to V2 >> - Remove SOC_IMX6SX depend > > No, I did not mean to remove SOC_IMX6SX completely. I was wondering if > we should also add "depends on OF". Even if we don't add OF dpeendency I > think we should have > > depends on SOC_IMX6SX || COMPILE_TEST > >> - Add "TO compile the deiver as module.. " >> - Change license 2015 >> - BIT(x) >> - use pdev->dev.of_node >> - iounmap at remove >> - use linux,keycode >> - use linux,wakeup >> - Remove irq >=0 check at devm_regust_irq >> - Remove IRQF_NO_SUSPEND >> - Register irq after allocate device to avoid race condition >> - add devm_add_action to remove del_timer_sync >> >> drivers/input/keyboard/Kconfig | 9 ++ >> drivers/input/keyboard/Makefile | 1 + >> drivers/input/keyboard/snvs_pwrkey.c | 239 +++++++++++++++++++++++++++++++++++ >> 3 files changed, 249 insertions(+) >> create mode 100644 drivers/input/keyboard/snvs_pwrkey.c >> >> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig >> index 106fbac..6c1c7de 100644 >> --- a/drivers/input/keyboard/Kconfig >> +++ b/drivers/input/keyboard/Kconfig >> @@ -400,6 +400,15 @@ config KEYBOARD_MPR121 >> To compile this driver as a module, choose M here: the >> module will be called mpr121_touchkey. >> >> +config KEYBOARD_SNVS_PWRKEY >> + tristate "IMX SNVS Power Key Driver" >> + help >> + This is the snvs powerkey driver for the Freescale i.MX application >> + processors that are newer than i.MX6 SX. >> + >> + To compile this driver as a module, choose M here; the >> + module will be called snvs_pwrkey. >> + >> config KEYBOARD_IMX >> tristate "IMX keypad support" >> depends on ARCH_MXC >> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile >> index df28d55..1d416dd 100644 >> --- a/drivers/input/keyboard/Makefile >> +++ b/drivers/input/keyboard/Makefile >> @@ -51,6 +51,7 @@ obj-$(CONFIG_KEYBOARD_QT1070) += qt1070.o >> obj-$(CONFIG_KEYBOARD_QT2160) += qt2160.o >> obj-$(CONFIG_KEYBOARD_SAMSUNG) += samsung-keypad.o >> obj-$(CONFIG_KEYBOARD_SH_KEYSC) += sh_keysc.o >> +obj-$(CONFIG_KEYBOARD_SNVS_PWRKEY) += snvs_pwrkey.o >> obj-$(CONFIG_KEYBOARD_SPEAR) += spear-keyboard.o >> obj-$(CONFIG_KEYBOARD_STMPE) += stmpe-keypad.o >> obj-$(CONFIG_KEYBOARD_STOWAWAY) += stowaway.o >> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c >> new file mode 100644 >> index 0000000..e4c2de3 >> --- /dev/null >> +++ b/drivers/input/keyboard/snvs_pwrkey.c >> @@ -0,0 +1,239 @@ >> +/* >> + * Copyright (C) 2015 Freescale Semiconductor, Inc. All Rights Reserved. >> + * >> + * The code contained herein is licensed under the GNU General Public >> + * License. You may obtain a copy of the GNU General Public License >> + * Version 2 or later at the following locations: >> + * >> + * http://www.opensource.org/licenses/gpl-license.html >> + * http://www.gnu.org/copyleft/gpl.html >> + */ >> + >> +#include <linux/device.h> >> +#include <linux/err.h> >> +#include <linux/init.h> >> +#include <linux/input.h> >> +#include <linux/interrupt.h> >> +#include <linux/io.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 SNVS_LPSR_REG 0x4C /* LP Status Register */ >> +#define SNVS_LPCR_REG 0x38 /* LP Control Register */ >> +#define SNVS_HPSR_REG 0x14 >> +#define SNVS_HPSR_BTN BIT(6) >> +#define SNVS_LPSR_SPO BIT(18) >> +#define SNVS_LPCR_DEP_EN BIT(5) >> + >> +struct pwrkey_drv_data { >> + void __iomem *ioaddr; >> + int irq; >> + int keycode; >> + int keystate; /* 1:pressed */ >> + int wakeup; >> + struct timer_list check_timer; >> + struct input_dev *input; >> +}; >> + >> +static void imx_imx_snvs_check_for_events(unsigned long data) >> +{ >> + struct pwrkey_drv_data *pdata = (struct pwrkey_drv_data *) data; >> + struct input_dev *input = pdata->input; >> + void __iomem *ioaddr = pdata->ioaddr; >> + u32 state; >> + >> + state = ((readl_relaxed(ioaddr + SNVS_HPSR_REG) & SNVS_HPSR_BTN) ? >> + 1 : 0); >> + >> + /* only report new event if status changed */ >> + if (state ^ pdata->keystate) { >> + pdata->keystate = state; >> + input_event(input, EV_KEY, pdata->keycode, state); >> + input_sync(input); >> + pm_relax(pdata->input->dev.parent); >> + } >> + >> + /* repeat check if pressed long */ >> + if (state) { >> + mod_timer(&pdata->check_timer, >> + jiffies + msecs_to_jiffies(60)); > > #define for 60 please. > >> + } >> +} >> + >> +static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id) >> +{ >> + struct platform_device *pdev = dev_id; >> + struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev); >> + void __iomem *ioaddr = pdata->ioaddr; >> + u32 lp_status; >> + >> + pm_wakeup_event(pdata->input->dev.parent, 0); >> + lp_status = readl_relaxed(ioaddr + SNVS_LPSR_REG); >> + if (lp_status & SNVS_LPSR_SPO) >> + mod_timer(&pdata->check_timer, jiffies + msecs_to_jiffies(2)); > > Hmm, 2 msec is kind of low for debouncing I think. In any case please > add a #define for it. > >> + >> + /* clear SPO status */ >> + writel_relaxed(lp_status, ioaddr + SNVS_LPSR_REG); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static void imx_snvs_pwrkey_act(void *pdata) >> +{ >> + struct pwrkey_drv_data *pd = pdata; >> + >> + del_timer_sync(&pd->check_timer); >> +} >> + >> +static int imx_snvs_pwrkey_probe(struct platform_device *pdev) >> +{ >> + struct pwrkey_drv_data *pdata = NULL; >> + struct input_dev *input = NULL; >> + struct device_node *np; >> + void __iomem *ioaddr; >> + u32 val; >> + int ret = 0; > > Presonal preference - can we please call this variable "error"? And you > do not need to initialize it. > >> + >> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); >> + if (!pdata) >> + return -ENOMEM; >> + >> + /* Get SNVS register Page */ >> + np = pdev->dev.of_node; >> + if (!np) >> + return -ENODEV; > > We should check it before trying to allocate memory. > >> + pdata->ioaddr = of_iomap(np, 0); >> + if (IS_ERR(pdata->ioaddr)) >> + return PTR_ERR(pdata->ioaddr); > > Umm, you are still leaking it on error path. Can you try doing: > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > pdata->ioaddr = devm_ioremap_resource(&pdev->dev, res); > if (IS_ERR(pdata->ioaddr)) > return PTR_ERR(pdata->ioaddr); > Thank you for comments. but I don't use of_iomap here because the resource is shared with the other device. SNVS included a rtc, a power off, ON/OFF key. If I use platform_get_resource and devm_ioremap_resource, rtc driver fail to probe. best regards Frank Li >> + >> + if (of_property_read_u32(np, "linux,keycode", &pdata->keycode)) { >> + pdata->keycode = KEY_POWER; >> + dev_warn(&pdev->dev, "KEY_POWER without setting in dts\n"); >> + } >> + >> + pdata->wakeup = !!of_get_property(np, "linux,wakeup", NULL); > > Just "wakeup", unlike "keycode" it is not linux-specific property, you > want wakeup on all OSes (or you don't). > >> + >> + pdata->irq = platform_get_irq(pdev, 0); >> + if (pdata->irq < 0) { >> + dev_err(&pdev->dev, "no irq defined in platform data\n"); >> + return -EINVAL; >> + } >> + >> + ioaddr = pdata->ioaddr; >> + val = readl_relaxed(ioaddr + SNVS_LPCR_REG); >> + val |= SNVS_LPCR_DEP_EN, >> + writel_relaxed(val, ioaddr + SNVS_LPCR_REG); >> + /* clear the unexpected interrupt before driver ready */ >> + val = readl_relaxed(ioaddr + SNVS_LPSR_REG); >> + if (val & SNVS_LPSR_SPO) >> + writel_relaxed(val | SNVS_LPSR_SPO, ioaddr + SNVS_LPSR_REG); >> + >> + setup_timer(&pdata->check_timer, >> + imx_imx_snvs_check_for_events, (unsigned long) pdata); >> + >> + 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 = "snvs-pwrkey/input0"; >> + input->id.bustype = BUS_HOST; >> + input->evbit[0] = BIT_MASK(EV_KEY); > > No need to explicitly set EV_KEY bit, in newer kernels > input_set_capability() does this for you. > >> + >> + input_set_capability(input, EV_KEY, pdata->keycode); >> + >> + /* input customer action to cancel release timer */ >> + ret = devm_add_action(&pdev->dev, imx_snvs_pwrkey_act, pdata); >> + if (ret) { >> + dev_err(&pdev->dev, "failed to register remove action\n"); >> + return ret; >> + } >> + >> + ret = devm_request_irq(&pdev->dev, pdata->irq, >> + imx_snvs_pwrkey_interrupt, >> + 0, pdev->name, pdev); >> + >> + if (ret) { >> + dev_err(&pdev->dev, "interrupt not available.\n"); >> + return ret; >> + } >> + >> + ret = input_register_device(input); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "failed to register input device\n"); >> + input_free_device(input); >> + return ret; >> + } >> + >> + pdata->input = input; >> + platform_set_drvdata(pdev, pdata); >> + >> + device_init_wakeup(&pdev->dev, pdata->wakeup); >> + >> + dev_info(&pdev->dev, "i.MX snvs powerkey probed\n"); > > Drop this, input core will print message for the device. > >> + >> + return 0; >> +} >> + >> +static int imx_snvs_pwrkey_remove(struct platform_device *pdev) >> +{ >> + struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev); >> + >> + input_unregister_device(pdata->input); > > Not needed with devm-managed input device. > >> + iounmap(pdata->ioaddr); > > If you switch to devm_ioremap_resource you can remove > imx_snvs_pwrkey_remove() altogether. > >> + >> + return 0; >> +} >> + >> +static int imx_snvs_pwrkey_suspend(struct device *dev) >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev); >> + >> + if (device_may_wakeup(&pdev->dev)) >> + enable_irq_wake(pdata->irq); >> + >> + return 0; >> +} >> + >> +static int imx_snvs_pwrkey_resume(struct device *dev) >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev); >> + >> + if (device_may_wakeup(&pdev->dev)) >> + disable_irq_wake(pdata->irq); >> + >> + return 0; >> +} >> + >> +static const struct of_device_id imx_snvs_pwrkey_ids[] = { >> + { .compatible = "fsl,imx6sx-snvs-pwrkey" }, >> + { /* sentinel */ } >> +}; >> +MODULE_DEVICE_TABLE(of, imx_snvs_pwrkey_ids); >> + >> +static SIMPLE_DEV_PM_OPS(imx_snvs_pwrkey_pm_ops, imx_snvs_pwrkey_suspend, >> + imx_snvs_pwrkey_resume); >> + >> +static struct platform_driver imx_snvs_pwrkey_driver = { >> + .driver = { >> + .name = "snvs_pwrkey", >> + .owner = THIS_MODULE, > > No need to set the owner, we do this automatically. > >> + .pm = &imx_snvs_pwrkey_pm_ops, >> + .of_match_table = imx_snvs_pwrkey_ids, >> + }, >> + .probe = imx_snvs_pwrkey_probe, >> + .remove = imx_snvs_pwrkey_remove, >> +}; >> +module_platform_driver(imx_snvs_pwrkey_driver); >> + >> +MODULE_AUTHOR("Freescale Semiconductor"); >> +MODULE_DESCRIPTION("i.MX snvs power key Driver"); >> +MODULE_LICENSE("GPL"); >> -- >> 1.9.1 >> > > Thanks. > > -- > Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
