[PATCH v2 1/3] input: keyboad: imx: add snvs power key driver

[PATCH v2 1/3] input: keyboad: imx: add snvs power key driver

[Frank.Li]

From: Robin Gong <b38343@freescale.com>

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

Signed-off-by: Robin Gong <b38343@freescale.com>
Signed-off-by: Frank Li <Frank.Li@freescale.com>
---
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

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));
+	}
+}
+
+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));
+
+	/* 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;
+
+	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;
+	pdata->ioaddr = of_iomap(np, 0);
+	if (IS_ERR(pdata->ioaddr))
+		return PTR_ERR(pdata->ioaddr);
+
+	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);
+
+	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);
+
+	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");
+
+	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);
+	iounmap(pdata->ioaddr);
+
+	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,
+		.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

[PATCH v2 2/3] document: devicetree: input: imx: i.mx snvs power device tree bindings

[Frank.Li]

From: Frank Li <Frank.Li@freescale.com>

The snvs-pwrkey is designed to enable POWER key function which controlled
by SNVS ONOFF. the driver can report the status of POWER key and wakeup
system if pressed after system suspend.

Signed-off-by: Frank Li <Frank.Li@freescale.com>
Signed-off-by: Robin Gong <b38343@freescale.com>
---
 .../devicetree/bindings/input/snvs-pwrkey.txt      | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/snvs-pwrkey.txt

diff --git a/Documentation/devicetree/bindings/input/snvs-pwrkey.txt b/Documentation/devicetree/bindings/input/snvs-pwrkey.txt
new file mode 100644
index 0000000..9b3b58d
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/snvs-pwrkey.txt
@@ -0,0 +1,26 @@
+* Freescale i.MX SNVS powerkey device tree bindings
+
+The snvs-pwrkey is designed to enable POWER key function which controlled
+by SNVS ONOFF, the driver can report the status of POWER key and wakeup
+system if pressed after system suspend.
+
+Required SoC Specific Properties:
+- compatible: Should be "fsl,imx6sx-snvs-pwrkey".
+
+- reg: Physical base address of the SNVS and length of memory mapped
+  region.
+
+- interrupts: The SNVS interrupt number to the CPU(s).
+
+- linux,keycode: Keycode to emit, KEY_POWER by default.
+
+- linux,wakeup: Button can wake-up the system
+
+Example:
+snvs-pwrkey@0x020cc000 {
+	compatible = "fsl,imx6sx-snvs-pwrkey";
+	reg = <0x020cc000 0x4000>;
+	interrupts = <0 4 0x4>;
+	linux,keycode = <116>; /* KEY_POWER */
+	linux,wakeup;
+};
-- 
1.9.1

[PATCH v2 3/3] arm: dts: imx6sx: enable snvs power key

[Frank.Li]

From: Frank Li <Frank.Li@freescale.com>

enable snvs ONOFF power key support

Signed-off-by: Frank Li <Frank.Li@freescale.com>
---
 arch/arm/boot/dts/imx6sx.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
index 708175d..f531d18 100644
--- a/arch/arm/boot/dts/imx6sx.dtsi
+++ b/arch/arm/boot/dts/imx6sx.dtsi
@@ -680,6 +680,14 @@
 				};
 			};
 
+			snvs-pwrkey@0x020cc000 {
+				compatible = "fsl,imx6sx-snvs-pwrkey";
+				reg = <0x020cc000 0x4000>;
+				interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
+				linux,keycode = <116>; /* KEY_POWER */
+				linux,wakeup;
+			};
+
 			epit1: epit@020d0000 {
 				reg = <0x020d0000 0x4000>;
 				interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
-- 
1.9.1

Re: [PATCH v2 1/3] input: keyboad: imx: add snvs power key driver

[Dmitry Torokhov]

Hi Frank,

On Wed, May 13, 2015 at 10:47:40PM +0800, Frank.Li@freescale.com wrote:
> From: Robin Gong <b38343@freescale.com>
> 
> add snvs power key driver.
> It work in imx chips after i.mx6sx
> 
> Signed-off-by: Robin Gong <b38343@freescale.com>
> Signed-off-by: Frank Li <Frank.Li@freescale.com>
> ---
> 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);

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

Re: [PATCH v2 1/3] input: keyboad: imx: add snvs power key driver

[Zhi Li]

On Wed, May 13, 2015 at 11:40 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Frank,
>
> On Wed, May 13, 2015 at 10:47:40PM +0800, Frank.Li@freescale.com wrote:
>> From: Robin Gong <b38343@freescale.com>
>>
>> add snvs power key driver.
>> It work in imx chips after i.mx6sx
>>
>> Signed-off-by: Robin Gong <b38343@freescale.com>
>> Signed-off-by: Frank Li <Frank.Li@freescale.com>
>> ---
>> 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

Re: [PATCH v2 1/3] input: keyboad: imx: add snvs power key driver

[Joshua Clayton]

Hi Frank.

On Wednesday, May 13, 2015 01:27:39 PM Zhi Li wrote:
> On Wed, May 13, 2015 at 11:40 AM, Dmitry Torokhov
> 
> <dmitry.torokhov@gmail.com> wrote:
> > Hi Frank,
> > 

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

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

Re: [PATCH v2 1/3] input: keyboad: imx: add snvs power key driver

[Zhi Li]

On Wed, May 13, 2015 at 2:10 PM, Joshua Clayton
<stillcompiling@gmail.com> wrote:
>
> Hi Frank.
>
> On Wednesday, May 13, 2015 01:27:39 PM Zhi Li wrote:
>> On Wed, May 13, 2015 at 11:40 AM, Dmitry Torokhov
>>
>> <dmitry.torokhov@gmail.com> wrote:
>> > Hi Frank,
>> >
>
>> >
>> >> +     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
>>
>
> 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.

>
> --
> ~Joshua Clayton

Re: [PATCH v2 1/3] input: keyboad: imx: add snvs power key driver

[Dmitry Torokhov]

On Wed, May 13, 2015 at 02:27:46PM -0500, Zhi Li wrote:
> On Wed, May 13, 2015 at 2:10 PM, Joshua Clayton
> <stillcompiling@gmail.com> wrote:
> >
> > Hi Frank.
> >
> > On Wednesday, May 13, 2015 01:27:39 PM Zhi Li wrote:
> >> On Wed, May 13, 2015 at 11:40 AM, Dmitry Torokhov
> >>
> >> <dmitry.torokhov@gmail.com> wrote:
> >> > Hi Frank,
> >> >
> >
> >> >
> >> >> +     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
> >>
> >
> > 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.

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.

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

Re: [PATCH v2 1/3] input: keyboad: imx: add snvs power key driver

[Zhi Li]

On Wed, May 13, 2015 at 2:44 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, May 13, 2015 at 02:27:46PM -0500, Zhi Li wrote:
>> On Wed, May 13, 2015 at 2:10 PM, Joshua Clayton
>> <stillcompiling@gmail.com> wrote:
>> >
>> > Hi Frank.
>> >
>> > On Wednesday, May 13, 2015 01:27:39 PM Zhi Li wrote:
>> >> On Wed, May 13, 2015 at 11:40 AM, Dmitry Torokhov
>> >>
>> >> <dmitry.torokhov@gmail.com> wrote:
>> >> > Hi Frank,
>> >> >
>> >
>> >> >
>> >> >> +     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
>> >>
>> >
>> > 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.
>
> 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.

>
>>
>> PowerKey use difference irq number with RTC part.
>
> Yes and so they can run simultaneously and step on each other's feet,
> right?

Yes.

>
> Thanks.
>
> --
> Dmitry

Re: [PATCH v2 1/3] input: keyboad: imx: add snvs power key driver

[Dmitry Torokhov]

On Wed, May 13, 2015 at 02:57:00PM -0500, Zhi Li wrote:
> On Wed, May 13, 2015 at 2:44 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Wed, May 13, 2015 at 02:27:46PM -0500, Zhi Li wrote:
> >> On Wed, May 13, 2015 at 2:10 PM, Joshua Clayton
> >> <stillcompiling@gmail.com> wrote:
> >> >
> >> > Hi Frank.
> >> >
> >> > On Wednesday, May 13, 2015 01:27:39 PM Zhi Li wrote:
> >> >> On Wed, May 13, 2015 at 11:40 AM, Dmitry Torokhov
> >> >>
> >> >> <dmitry.torokhov@gmail.com> wrote:
> >> >> > Hi Frank,
> >> >> >
> >> >
> >> >> >
> >> >> >> +     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
> >> >>
> >> >
> >> > 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.
> >
> > 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.

Makes sense.

> 
> >
> >>
> >> PowerKey use difference irq number with RTC part.
> >
> > Yes and so they can run simultaneously and step on each other's feet,
> > right?
> 
> Yes.
> 
> >
> > Thanks.
> >
> > --
> > Dmitry

-- 
Dmitry

Re: [PATCH v2 1/3] input: keyboad: imx: add snvs power key driver

[Stefan Wahren]

Hi Frank,

looks like a typo in your subject line. keyboard?

> Frank.Li@freescale.com hat am 13. Mai 2015 um 16:47 geschrieben:
>
> [...]
> 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 @@
> +/*

a small driver description here would be nice.

> + * 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
> + */
> +
> [...]
> +static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> +{
> [...]
> +
> + pdata->wakeup = !!of_get_property(np, "linux,wakeup", NULL);

of_property_read_bool() ?

> +
> [...]
> +
> + ret = input_register_device(input);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to register input device\n");
> + input_free_device(input);

input_free_device() shouldn't be necessary since you are using
devm_input_allocate_device().

Thanks

Best regards
Stefan