input: keyboad: imx: add snvs power key driver

From: Robin Gong <[email protected]>

add snvs power key driver.
It work in imx chips after i.mx6sx

Signed-off-by Robin Gong <[email protected]>

Signed-off-by Frank Li <[email protected]>

---
Change from V1 to V2
- Remove SOC_IMX6SX depend

- 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

drivers/input/keyboard/Kconfig +9 -0

index 106fbac..6c1c7de 100644
@@ -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

drivers/input/keyboard/Makefile +1 -0

index df28d55..1d416dd 100644
@@ -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

drivers/input/keyboard/snvs_pwrkey.c new file +53 -0

new file mode 100644
index 0000000..e4c2de3
@@ -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) {

drivers/input/keyboard/snvs_pwrkey.c new file +4 -0

+	}
+}
+
+static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)

drivers/input/keyboard/snvs_pwrkey.c new file +12 -0

+
+	/* 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);

drivers/input/keyboard/snvs_pwrkey.c new file +19 -0

+
+	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);
+

drivers/input/keyboard/snvs_pwrkey.c new file +22 -0

+
+	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");

drivers/input/keyboard/snvs_pwrkey.c new file +29 -0

+
+	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 */ }

drivers/input/keyboard/snvs_pwrkey.c new file +11 -0

+		.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");

Signature · 1 line

1.9.1

Hi Frank,

Quoted 10 lines · Frank.Li at freescale.com

From: Robin Gong <[email protected]>

add snvs power key driver.
It work in imx chips after i.mx6sx

Signed-off-by Robin Gong <[email protected]>

Signed-off-by Frank Li <[email protected]>

---
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

Quoted 109 lines

- 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

drivers/input/keyboard/Kconfig +9 -0

index 106fbac..6c1c7de 100644
@@ -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

drivers/input/keyboard/Makefile +1 -0

index df28d55..1d416dd 100644
@@ -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

drivers/input/keyboard/snvs_pwrkey.c new file +53 -0

new file mode 100644
index 0000000..e4c2de3
@@ -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) {

drivers/input/keyboard/snvs_pwrkey.c line 63

Quoted 10 lines

drivers/input/keyboard/snvs_pwrkey.c new file +10 -0

+             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.

Quoted 4 lines

drivers/input/keyboard/snvs_pwrkey.c new file +4 -0

+     }
+}
+
+static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)

drivers/input/keyboard/snvs_pwrkey.c line 77

Quoted 10 lines

drivers/input/keyboard/snvs_pwrkey.c new file +10 -0

+{
+     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.

Quoted 12 lines

drivers/input/keyboard/snvs_pwrkey.c new file +12 -0

+
+     /* 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);

drivers/input/keyboard/snvs_pwrkey.c line 99

Quoted 10 lines

drivers/input/keyboard/snvs_pwrkey.c new file +10 -0

+}
+
+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.

drivers/input/keyboard/snvs_pwrkey.c line 108

Quoted 9 lines

drivers/input/keyboard/snvs_pwrkey.c new file +9 -0

+
+     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.

drivers/input/keyboard/snvs_pwrkey.c line 111

Quoted 3 lines

drivers/input/keyboard/snvs_pwrkey.c new file +3 -0

+     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);

drivers/input/keyboard/snvs_pwrkey.c line 118

Quoted 7 lines

drivers/input/keyboard/snvs_pwrkey.c new file +7 -0

+
+     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).

Quoted 19 lines

drivers/input/keyboard/snvs_pwrkey.c new file +19 -0

+
+     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);
+

drivers/input/keyboard/snvs_pwrkey.c line 147

Quoted 10 lines

drivers/input/keyboard/snvs_pwrkey.c new file +10 -0

+     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.

Quoted 22 lines

drivers/input/keyboard/snvs_pwrkey.c new file +22 -0

+
+     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");

drivers/input/keyboard/snvs_pwrkey.c line 179

Quoted 10 lines

drivers/input/keyboard/snvs_pwrkey.c new file +10 -0

+             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.

drivers/input/keyboard/snvs_pwrkey.c line 188

Quoted 9 lines

drivers/input/keyboard/snvs_pwrkey.c new file +9 -0

+
+     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.

drivers/input/keyboard/snvs_pwrkey.c line 189

Quoted 1 line

drivers/input/keyboard/snvs_pwrkey.c new file +1 -0

+     iounmap(pdata->ioaddr);

If you switch to devm_ioremap_resource you can remove imx_snvs_pwrkey_remove() altogether.

Quoted 29 lines

drivers/input/keyboard/snvs_pwrkey.c new file +29 -0

+
+     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 */ }

drivers/input/keyboard/snvs_pwrkey.c line 228

Quoted 10 lines

drivers/input/keyboard/snvs_pwrkey.c new file +10 -0

+};
+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.

Quoted 12 lines

drivers/input/keyboard/snvs_pwrkey.c new file +11 -1

+             .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");
--

Quoted 2 lines

1.9.1

Thanks.

--
Dmitry

On Wed, May 13, 2015 at 11:40 AM, Dmitry Torokhov

Quoted 8 lines · <[email protected]> wrote:

Hi Frank,

drivers/input/keyboard/snvs_pwrkey.c line 111

Quoted 3 lines

drivers/input/keyboard/snvs_pwrkey.c new file +3 -0

+     pdata->ioaddr = of_iomap(np, 0);
+     if (IS_ERR(pdata->ioaddr))
+             return PTR_ERR(pdata->ioaddr);

Quoted 6 lines

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

Hi Frank.

Quoted 22 lines · Zhi Li

On Wed, May 13, 2015 at 11:40 AM, Dmitry Torokhov

Quoted 8 lines · <[email protected]> wrote:

Hi Frank,

drivers/input/keyboard/snvs_pwrkey.c line 111

Quoted 3 lines

drivers/input/keyboard/snvs_pwrkey.c new file +3 -0

+     pdata->ioaddr = of_iomap(np, 0);
+     if (IS_ERR(pdata->ioaddr))
+             return PTR_ERR(pdata->ioaddr);

Quoted 6 lines

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.

Quoted 7 lines

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

The fact that you cannot use of_iomap() seems a clear warning of unsafe access.
Since it is a shared resource, which you are writing to as well as reading, perhaps you should set it up for safe access using the regmap API like the GPR registers.
/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h

--
~Joshua Clayton

Quoted 34 lines · Joshua Clayton

Hi Frank.

Quoted 22 lines · Zhi Li

On Wed, May 13, 2015 at 11:40 AM, Dmitry Torokhov

Quoted 8 lines · <[email protected]> wrote:

Hi Frank,

drivers/input/keyboard/snvs_pwrkey.c line 111

Quoted 3 lines

drivers/input/keyboard/snvs_pwrkey.c new file +3 -0

+     pdata->ioaddr = of_iomap(np, 0);
+     if (IS_ERR(pdata->ioaddr))
+             return PTR_ERR(pdata->ioaddr);

Quoted 6 lines

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.

Quoted 7 lines

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

Quoted 6 lines

The fact that you cannot use of_iomap() seems a clear warning of unsafe access.
Since it is a shared resource, which you are writing to as well as reading, perhaps you should set it up for safe access using the regmap API like the GPR registers.
/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h

I read driver again clearfully. It is safed shared with rtc driver directly. Because pwr key driver just read a irq status register and other one register, which used only in this driver.

Just w1c (write 1 clear) register. it is atomic access by hardware.

PowerKey use difference irq number with RTC part.

Quoted 47 lines · Zhi Li

Quoted 34 lines · Joshua Clayton

Hi Frank.

Quoted 22 lines · Zhi Li

On Wed, May 13, 2015 at 11:40 AM, Dmitry Torokhov

Quoted 8 lines · <[email protected]> wrote:

Hi Frank,

drivers/input/keyboard/snvs_pwrkey.c line 111

Quoted 3 lines

drivers/input/keyboard/snvs_pwrkey.c new file +3 -0

+     pdata->ioaddr = of_iomap(np, 0);
+     if (IS_ERR(pdata->ioaddr))
+             return PTR_ERR(pdata->ioaddr);

Quoted 6 lines

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.

Quoted 7 lines

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

Quoted 6 lines

The fact that you cannot use of_iomap() seems a clear warning of unsafe access.
Since it is a shared resource, which you are writing to as well as reading, perhaps you should set it up for safe access using the regmap API like the GPR registers.
/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h

I read driver again clearfully. It is safed shared with rtc driver directly. Because pwr key driver just read a irq status register and other one register, which used only in this driver.

Quoted 1 line

Just w1c (write 1 clear) register. it is atomic access by hardware.

It does not really matter, driver has to claim resources it is using to make sure other parties are not accessing its registers. If registers are shared you need to have a 3rd party mediator.

Quoted 3 lines

PowerKey use difference irq number with RTC part.

Yes and so they can run simultaneously and step on each other's feet, right?

Thanks.

--
Dmitry

Quoted 48 lines · Dmitry Torokhov

Quoted 47 lines · Zhi Li

Quoted 34 lines · Joshua Clayton

Hi Frank.

Quoted 22 lines · Zhi Li

On Wed, May 13, 2015 at 11:40 AM, Dmitry Torokhov

Quoted 8 lines · <[email protected]> wrote:

Hi Frank,

Review

Quoted 3 lines

(quoted diff) +3 -0

+     pdata->ioaddr = of_iomap(np, 0);
+     if (IS_ERR(pdata->ioaddr))
+             return PTR_ERR(pdata->ioaddr);

Quoted 6 lines

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.

Quoted 7 lines

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

Quoted 6 lines

The fact that you cannot use of_iomap() seems a clear warning of unsafe access.
Since it is a shared resource, which you are writing to as well as reading, perhaps you should set it up for safe access using the regmap API like the GPR registers.
/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h

I read driver again clearfully. It is safed shared with rtc driver directly. Because pwr key driver just read a irq status register and other one register, which used only in this driver.

Quoted 5 lines

Quoted 1 line

Just w1c (write 1 clear) register. it is atomic access by hardware.

It does not really matter, driver has to claim resources it is using to make sure other parties are not accessing its registers. If registers are shared you need to have a 3rd party mediator.

I can change to use syscon.
but other two driver(rtc and power off) need change also.

Quoted 7 lines

Quoted 4 lines

PowerKey use difference irq number with RTC part.

Yes and so they can run simultaneously and step on each other's feet, right?

Yes.

Quoted 5 lines

Thanks.

--
Dmitry

drivers/input/keyboard/snvs_pwrkey.c new file +6 -0

+ * 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:
+ *