[RFC][PATCH 0/5] Hi655x powerkey support for HiKey

[RFC][PATCH 0/5] Hi655x powerkey support for HiKey

[John Stultz]

This patchset enables the pmic powerkey to function on HiKey.

I wanted to submit it for some initial review. Feedback would
be greatly appreciated!

thanks
-john

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
Cc: Wei Xu <xuwei5@hisilicon.com>
Cc: Guodong Xu <guodong.xu@linaro.org>

Guodong Xu (1):
  arm64: dts: Add powerkey info to pmic for hi6220-hikey

John Stultz (3):
  dts: bindings: Add binding documentation for hisilicon,hi6552-powerkey
  hi655x-pmic: Make hi655x pmic logic probe child nodes in the dt
  hi655x-pmic: Fixup issue with un-acked interrupts

Jorge Ramirez-Ortiz (1):
  drivers: input: powerkey for HISI 65xx SoC

 .../bindings/input/hisilicon,hi6552-powerkey.txt   |  40 ++++
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts     |   7 +
 drivers/input/misc/Kconfig                         |   8 +
 drivers/input/misc/Makefile                        |   1 +
 drivers/input/misc/hisi_powerkey.c                 | 228 +++++++++++++++++++++
 drivers/mfd/hi655x-pmic.c                          |   9 +
 6 files changed, 293 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/hisilicon,hi6552-powerkey.txt
 create mode 100644 drivers/input/misc/hisi_powerkey.c

-- 
1.9.1

[RFC][PATCH 1/5] dts: bindings: Add binding documentation for hisilicon,hi6552-powerkey

[John Stultz]

Adds binding documentation for hi6552 pmic powerkey button.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
Cc: Wei Xu <xuwei5@hisilicon.com>
Cc: Guodong Xu <guodong.xu@linaro.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 .../bindings/input/hisilicon,hi6552-powerkey.txt   | 40 ++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/hisilicon,hi6552-powerkey.txt

diff --git a/Documentation/devicetree/bindings/input/hisilicon,hi6552-powerkey.txt b/Documentation/devicetree/bindings/input/hisilicon,hi6552-powerkey.txt
new file mode 100644
index 0000000..7f09124
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/hisilicon,hi6552-powerkey.txt
@@ -0,0 +1,40 @@
+HiSilicon hi6552 PMIC Power Key
+
+PROPERTIES
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be one of:
+		    "hisilicon,hi6552-powerkey"
+
+- interrupt-parent
+	Usage: (required if interrupt property is defined)
+	Value type: <phandle>
+	Definition: A single <phandle> value that points to the interrupt
+		    parent to which the child domain is being mapped.
+
+- interrupts:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: The first interrupt specifies the key release interrupt
+		    and the second interrupt specifies the key press interrupt,
+		    the third defines the timed key-hold interrupt.
+		    The format of the specifier is defined by the binding
+		    document describing the node's interrupt parent.
+
+- interrupt-names:
+	Usage: required
+	Value type: "down", "up", "hold 4s"
+	Definition: String names for the press, release and timed hold
+		    interrupts.
+
+
+EXAMPLE
+
+	powerkey:powerkey@b1{
+		compatible = "hisilicon,hi6552-powerkey";
+		interrupt-parent = <&pmic>;
+		interrupts = <6 0>, <5 0>, <4 0>;
+		interrupt-names = "down", "up", "hold 4s";
+	};
-- 
1.9.1

[RFC][PATCH 2/5] hi655x-pmic: Make hi655x pmic logic probe child nodes in the dt

[John Stultz]

In trying to wire up the powerkey driver, I found I
needed to add this to get the pmic logic to probe
child nodes in the dt data.

With this patch, child nodes get properly probed.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
Cc: Wei Xu <xuwei5@hisilicon.com>
Cc: Guodong Xu <guodong.xu@linaro.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/mfd/hi655x-pmic.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mfd/hi655x-pmic.c b/drivers/mfd/hi655x-pmic.c
index 05ddc78..3511035 100644
--- a/drivers/mfd/hi655x-pmic.c
+++ b/drivers/mfd/hi655x-pmic.c
@@ -39,6 +39,11 @@ static const struct regmap_irq hi655x_irqs[] = {
 	{ .reg_offset = 0, .mask = RESERVE_INT },
 };
 
+static const struct of_device_id of_hi655x_pmic_child_match_tbl[] = {
+	{ .compatible = "hisilicon,hi6552-powerkey", },
+	{ /* end */ }
+};
+
 static const struct regmap_irq_chip hi655x_irq_chip = {
 	.name = "hi655x-pmic",
 	.irqs = hi655x_irqs,
@@ -122,6 +127,9 @@ static int hi655x_pmic_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pmic);
 
+	/* populate sub nodes */
+	of_platform_populate(np, of_hi655x_pmic_child_match_tbl, NULL, dev);
+
 	ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO, hi655x_pmic_devs,
 			      ARRAY_SIZE(hi655x_pmic_devs), NULL, 0, NULL);
 	if (ret) {
-- 
1.9.1

Re: [RFC][PATCH 2/5] hi655x-pmic: Make hi655x pmic logic probe child nodes in the dt

[Lee Jones]

Subject should indicate "mfd".

`git log --oneline -- drivers/mfd`

> In trying to wire up the powerkey driver, I found I
> needed to add this to get the pmic logic to probe

PMIC

> child nodes in the dt data.

DT.

Please use full buffer width.  Wrapping at 50 chars is not necessary.

> With this patch, child nodes get properly probed.
> 
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> Cc: Wei Xu <xuwei5@hisilicon.com>
> Cc: Guodong Xu <guodong.xu@linaro.org>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/mfd/hi655x-pmic.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/mfd/hi655x-pmic.c b/drivers/mfd/hi655x-pmic.c
> index 05ddc78..3511035 100644
> --- a/drivers/mfd/hi655x-pmic.c
> +++ b/drivers/mfd/hi655x-pmic.c
> @@ -39,6 +39,11 @@ static const struct regmap_irq hi655x_irqs[] = {
>  	{ .reg_offset = 0, .mask = RESERVE_INT },
>  };
>  
> +static const struct of_device_id of_hi655x_pmic_child_match_tbl[] = {
> +	{ .compatible = "hisilicon,hi6552-powerkey", },
> +	{ /* end */ }
> +};
> +
>  static const struct regmap_irq_chip hi655x_irq_chip = {
>  	.name = "hi655x-pmic",
>  	.irqs = hi655x_irqs,
> @@ -122,6 +127,9 @@ static int hi655x_pmic_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, pmic);
>  
> +	/* populate sub nodes */
> +	of_platform_populate(np, of_hi655x_pmic_child_match_tbl, NULL, dev);
> +

Oh, holey poop, no!

Please don't mix DT and MFD registration like this.

Drivers should contain either of_platform_populate() or
mfd_add_devices(), but never both.

>  	ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO, hi655x_pmic_devs,
>  			      ARRAY_SIZE(hi655x_pmic_devs), NULL, 0, NULL);
>  	if (ret) {

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RFC][PATCH 2/5] hi655x-pmic: Make hi655x pmic logic probe child nodes in the dt

[John Stultz]

On Wed, Jun 8, 2016 at 7:31 AM, Lee Jones <lee.jones@linaro.org> wrote:
>
> Please use full buffer width.  Wrapping at 50 chars is not necessary.

But then how would you be able to review patches on your phone?! :)


>> --- a/drivers/mfd/hi655x-pmic.c
>> +++ b/drivers/mfd/hi655x-pmic.c
>> @@ -39,6 +39,11 @@ static const struct regmap_irq hi655x_irqs[] = {
>>       { .reg_offset = 0, .mask = RESERVE_INT },
>>  };
>>
>> +static const struct of_device_id of_hi655x_pmic_child_match_tbl[] = {
>> +     { .compatible = "hisilicon,hi6552-powerkey", },
>> +     { /* end */ }
>> +};
>> +
>>  static const struct regmap_irq_chip hi655x_irq_chip = {
>>       .name = "hi655x-pmic",
>>       .irqs = hi655x_irqs,
>> @@ -122,6 +127,9 @@ static int hi655x_pmic_probe(struct platform_device *pdev)
>>
>>       platform_set_drvdata(pdev, pmic);
>>
>> +     /* populate sub nodes */
>> +     of_platform_populate(np, of_hi655x_pmic_child_match_tbl, NULL, dev);
>> +
>
> Oh, holey poop, no!
>
> Please don't mix DT and MFD registration like this.
>
> Drivers should contain either of_platform_populate() or
> mfd_add_devices(), but never both.
>
>>       ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO, hi655x_pmic_devs,
>>                             ARRAY_SIZE(hi655x_pmic_devs), NULL, 0, NULL);
>>       if (ret) {


So apologies for my ignorance here. I'm not quite sure I see what the
right thing to do. I tried adding a "hi65xx-powerkey" entry in the
hi655x_pmic_devs, but then mfd_add_devices doesn't seem to provide any
of the irq info from the DT.

The only way I can think to make it work w/o calling to populate
sub-nodes is to pull the powerkey subnode out of the pmic node. It
seems to work, but I'm not sure if it properly describes the hardware
then.  Sorry again for being a bit clueless here.

Thanks for the review!
-john

Re: [RFC][PATCH 2/5] hi655x-pmic: Make hi655x pmic logic probe child nodes in the dt

[Lee Jones]

On Wed, 08 Jun 2016, John Stultz wrote:

> On Wed, Jun 8, 2016 at 7:31 AM, Lee Jones <lee.jones@linaro.org> wrote:
> >
> > Please use full buffer width.  Wrapping at 50 chars is not necessary.
> 
> But then how would you be able to review patches on your phone?! :)

Sideways! ;)

> >> --- a/drivers/mfd/hi655x-pmic.c
> >> +++ b/drivers/mfd/hi655x-pmic.c
> >> @@ -39,6 +39,11 @@ static const struct regmap_irq hi655x_irqs[] = {
> >>       { .reg_offset = 0, .mask = RESERVE_INT },
> >>  };
> >>
> >> +static const struct of_device_id of_hi655x_pmic_child_match_tbl[] = {
> >> +     { .compatible = "hisilicon,hi6552-powerkey", },
> >> +     { /* end */ }
> >> +};
> >> +
> >>  static const struct regmap_irq_chip hi655x_irq_chip = {
> >>       .name = "hi655x-pmic",
> >>       .irqs = hi655x_irqs,
> >> @@ -122,6 +127,9 @@ static int hi655x_pmic_probe(struct platform_device *pdev)
> >>
> >>       platform_set_drvdata(pdev, pmic);
> >>
> >> +     /* populate sub nodes */
> >> +     of_platform_populate(np, of_hi655x_pmic_child_match_tbl, NULL, dev);
> >> +
> >
> > Oh, holey poop, no!
> >
> > Please don't mix DT and MFD registration like this.
> >
> > Drivers should contain either of_platform_populate() or
> > mfd_add_devices(), but never both.
> >
> >>       ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO, hi655x_pmic_devs,
> >>                             ARRAY_SIZE(hi655x_pmic_devs), NULL, 0, NULL);
> >>       if (ret) {
> 
> 
> So apologies for my ignorance here. I'm not quite sure I see what the
> right thing to do. I tried adding a "hi65xx-powerkey" entry in the
> hi655x_pmic_devs, but then mfd_add_devices doesn't seem to provide any
> of the irq info from the DT.
> 
> The only way I can think to make it work w/o calling to populate
> sub-nodes is to pull the powerkey subnode out of the pmic node. It
> seems to work, but I'm not sure if it properly describes the hardware
> then.  Sorry again for being a bit clueless here.

Since we use mfd_add_devices() already, you need to create a cell in
this file for the power-key driver and omit it from DT altogether.  If
your priority is to have each sub-device represented in DT, then you
need to stop using mfd_add_devices() and have of_platform_populate()
do your bidding.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[RFC][PATCH 3/5] hi655x-pmic: Fixup issue with un-acked interrupts

[John Stultz]

While trying to get the powerkey to function, I found
when pressing the key, I would get infinitely repeating
interrupts.

After digging around a bit, it seems we didn't set the
ack_base value for the regmap irqchip logic, so nothing
was acking the interrupt.

This patch adds the ack_base, which seems to make things
work.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
Cc: Wei Xu <xuwei5@hisilicon.com>
Cc: Guodong Xu <guodong.xu@linaro.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/mfd/hi655x-pmic.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/hi655x-pmic.c b/drivers/mfd/hi655x-pmic.c
index 3511035..4a3fdbc 100644
--- a/drivers/mfd/hi655x-pmic.c
+++ b/drivers/mfd/hi655x-pmic.c
@@ -50,6 +50,7 @@ static const struct regmap_irq_chip hi655x_irq_chip = {
 	.num_regs = 1,
 	.num_irqs = ARRAY_SIZE(hi655x_irqs),
 	.status_base = HI655X_IRQ_STAT_BASE,
+	.ack_base = HI655X_IRQ_STAT_BASE,
 	.mask_base = HI655X_IRQ_MASK_BASE,
 };
 
-- 
1.9.1

Re: [RFC][PATCH 3/5] hi655x-pmic: Fixup issue with un-acked interrupts

[Lee Jones]

On Wed, 01 Jun 2016, John Stultz wrote:

> While trying to get the powerkey to function, I found
> when pressing the key, I would get infinitely repeating
> interrupts.
> 
> After digging around a bit, it seems we didn't set the
> ack_base value for the regmap irqchip logic, so nothing
> was acking the interrupt.
> 
> This patch adds the ack_base, which seems to make things
> work.
> 
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> Cc: Wei Xu <xuwei5@hisilicon.com>
> Cc: Guodong Xu <guodong.xu@linaro.org>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/mfd/hi655x-pmic.c | 1 +
>  1 file changed, 1 insertion(+)

Seems reasonable.

Acked-by: Lee Jones <lee.jones@linaro.org>

> diff --git a/drivers/mfd/hi655x-pmic.c b/drivers/mfd/hi655x-pmic.c
> index 3511035..4a3fdbc 100644
> --- a/drivers/mfd/hi655x-pmic.c
> +++ b/drivers/mfd/hi655x-pmic.c
> @@ -50,6 +50,7 @@ static const struct regmap_irq_chip hi655x_irq_chip = {
>  	.num_regs = 1,
>  	.num_irqs = ARRAY_SIZE(hi655x_irqs),
>  	.status_base = HI655X_IRQ_STAT_BASE,
> +	.ack_base = HI655X_IRQ_STAT_BASE,
>  	.mask_base = HI655X_IRQ_MASK_BASE,
>  };
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[RFC][PATCH 4/5] drivers: input: powerkey for HISI 65xx SoC

[John Stultz]

From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>

This driver provides a input driver for the power button on the
HiSi 65xx SoC for boards like HiKey.

This driver was originally by Zhiliang Xue <xuezhiliang@huawei.com>
then basically rewritten by Jorge, but preserving the original
module author credits.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
Cc: Wei Xu <xuwei5@hisilicon.com>
Cc: Guodong Xu <guodong.xu@linaro.org>
Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
[jstultz: Reworked commit message, folded in other fixes/cleanups
from Jorge, and made a few small fixes and cleanups of my own]
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/input/misc/Kconfig         |   8 ++
 drivers/input/misc/Makefile        |   1 +
 drivers/input/misc/hisi_powerkey.c | 228 +++++++++++++++++++++++++++++++++++++
 3 files changed, 237 insertions(+)
 create mode 100644 drivers/input/misc/hisi_powerkey.c

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 1f2337a..2e57bbd 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -796,4 +796,12 @@ config INPUT_DRV2667_HAPTICS
 	  To compile this driver as a module, choose M here: the
 	  module will be called drv2667-haptics.
 
+config HISI_POWERKEY
+	tristate "Hisilicon PMIC ONKEY support"
+	help
+	  Say Y to enable support for PMIC ONKEY.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called hisi_powerkey.
+
 endif
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 0357a08..f264777 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -75,3 +75,4 @@ obj-$(CONFIG_INPUT_WM831X_ON)		+= wm831x-on.o
 obj-$(CONFIG_INPUT_XEN_KBDDEV_FRONTEND)	+= xen-kbdfront.o
 obj-$(CONFIG_INPUT_YEALINK)		+= yealink.o
 obj-$(CONFIG_INPUT_IDEAPAD_SLIDEBAR)	+= ideapad_slidebar.o
+obj-$(CONFIG_HISI_POWERKEY)		+= hisi_powerkey.o
diff --git a/drivers/input/misc/hisi_powerkey.c b/drivers/input/misc/hisi_powerkey.c
new file mode 100644
index 0000000..3a35a75
--- /dev/null
+++ b/drivers/input/misc/hisi_powerkey.c
@@ -0,0 +1,228 @@
+/*
+ * hisi_powerkey.c - Hisilicon MIC powerkey driver
+ *
+ * Copyright (C) 2013 Hisilicon Ltd.
+ * Copyright (C) 2015, 2016 Linaro Ltd.
+ *
+ * This file is subject to the terms and conditions of the GNU General
+ * Public License. See the file "COPYING" in the main directory of this
+ * archive for more details.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/reboot.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/input.h>
+#include <linux/slab.h>
+
+/* the above held interrupt will trigger after 4 seconds */
+#define MAX_HELD_TIME	(4 * MSEC_PER_SEC)
+
+
+typedef irqreturn_t (*hi65xx_irq_handler) (int irq, void *data);
+enum { id_pressed, id_released, id_held, id_last };
+static irqreturn_t hi65xx_pkey_irq_handler(int irq, void *q);
+
+/*
+ * power key irq information
+ */
+static struct hi65xx_pkey_irq_info {
+	hi65xx_irq_handler handler;
+	const char *const name;
+	int irq;
+} irq_info[id_last] = {
+	[id_pressed] = {
+		.handler = hi65xx_pkey_irq_handler,
+		.name = "down",
+		.irq = -1,
+	},
+	[id_released] = {
+		.handler = hi65xx_pkey_irq_handler,
+		.name = "up",
+		.irq = -1,
+	},
+	[id_held] = {
+		.handler = hi65xx_pkey_irq_handler,
+		.name = "hold 4s",
+		.irq = -1,
+	},
+};
+
+/*
+ * power key events
+ */
+static struct key_report_pairs {
+	int code;
+	int value;
+} pkey_report[id_last] = {
+	[id_released] = {
+		.code = KEY_POWER,
+		.value = 0
+	},
+	[id_pressed] = {
+		.code = KEY_POWER,
+		.value = 1
+	},
+	[id_held] = {
+		.code = KEY_RESTART,
+		.value = 0
+	},
+};
+
+struct hi65xx_priv {
+	struct input_dev *input;
+	struct wakeup_source wlock;
+};
+
+static inline void report_key(struct input_dev *dev, int id_action)
+{
+	/*
+	 * track the state of the key held event since only ON/OFF values are
+	 * allowed on EV_KEY types: KEY_RESTART will always toggle its value to
+	 * guarantee that the event is passed to handlers (dispossition update).
+	 */
+	if (id_action == id_held)
+		pkey_report[id_held].value ^= 1;
+
+	dev_dbg(dev->dev.parent, "received - code %d, value %d\n",
+		pkey_report[id_action].code,
+		pkey_report[id_action].value);
+
+	input_report_key(dev, pkey_report[id_action].code,
+		pkey_report[id_action].value);
+}
+
+static irqreturn_t hi65xx_pkey_irq_handler(int irq, void *q)
+{
+	struct hi65xx_priv *p = q;
+	int i, action = -1;
+
+	for (i = 0; i < id_last; i++)
+		if (irq == irq_info[i].irq)
+			action = i;
+
+	if (action == -1)
+		return IRQ_NONE;
+
+	__pm_wakeup_event(&p->wlock, MAX_HELD_TIME);
+
+	report_key(p->input, action);
+	input_sync(p->input);
+
+	return IRQ_HANDLED;
+}
+
+static int hi65xx_powerkey_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct hi65xx_priv *priv;
+	int irq, i, ret;
+
+	if (pdev == NULL) {
+		dev_err(dev, "parameter error\n");
+		return -EINVAL;
+	}
+
+	priv = devm_kzalloc(dev, sizeof(struct hi65xx_priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->input = input_allocate_device();
+	if (!priv->input) {
+		dev_err(&pdev->dev, "failed to allocate input device\n");
+		return -ENOENT;
+	}
+
+	priv->input->evbit[0] = BIT_MASK(EV_KEY);
+	priv->input->dev.parent = &pdev->dev;
+	priv->input->phys = "hisi_on/input0";
+	priv->input->name = "HISI 65xx PowerOn Key";
+
+	for (i = 0; i < ARRAY_SIZE(pkey_report); i++)
+		input_set_capability(priv->input, EV_KEY, pkey_report[i].code);
+
+	for (i = 0; i < ARRAY_SIZE(irq_info); i++) {
+
+		irq = platform_get_irq_byname(pdev, irq_info[i].name);
+		if (irq < 0) {
+			dev_err(dev, "couldn't get irq %s\n", irq_info[i].name);
+			ret = irq;
+			goto err_irq;
+		}
+
+		ret = devm_request_threaded_irq(dev, irq, NULL,
+					irq_info[i].handler, IRQF_ONESHOT,
+					irq_info[i].name, priv);
+		if (ret < 0) {
+			dev_err(dev, "couldn't get irq %s\n", irq_info[i].name);
+			goto err_irq;
+		}
+
+		irq_info[i].irq = irq;
+	}
+
+	wakeup_source_init(&priv->wlock, "hisi-powerkey");
+
+	ret = input_register_device(priv->input);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register input device: %d\n",
+			ret);
+		ret = -ENOENT;
+		goto err_register;
+	}
+
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+
+err_register:
+	wakeup_source_trash(&priv->wlock);
+err_irq:
+	input_free_device(priv->input);
+
+	return ret;
+}
+
+static int hi65xx_powerkey_remove(struct platform_device *pdev)
+{
+	struct hi65xx_priv *priv = platform_get_drvdata(pdev);
+
+	wakeup_source_trash(&priv->wlock);
+	input_unregister_device(priv->input);
+
+	return 0;
+}
+
+static const struct of_device_id hi65xx_powerkey_of_match[] = {
+	{ .compatible = "hisilicon,hi6552-powerkey", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, hi65xx_powerkey_of_match);
+
+static struct platform_driver hi65xx_powerkey_driver = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = "hi65xx-powerkey",
+		.of_match_table = hi65xx_powerkey_of_match,
+	},
+	.probe = hi65xx_powerkey_probe,
+	.remove  = hi65xx_powerkey_remove,
+};
+
+module_platform_driver(hi65xx_powerkey_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Zhiliang Xue <xuezhiliang@huawei.com");
+MODULE_DESCRIPTION("Hisi PMIC Power key driver");
+MODULE_LICENSE("GPL v2");
+
+
-- 
1.9.1

Re: [RFC][PATCH 4/5] drivers: input: powerkey for HISI 65xx SoC

[Dmitry Torokhov]

Hi John,

On Wed, Jun 01, 2016 at 02:27:39PM -0700, John Stultz wrote:
> From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> 
> This driver provides a input driver for the power button on the
> HiSi 65xx SoC for boards like HiKey.
> 
> This driver was originally by Zhiliang Xue <xuezhiliang@huawei.com>
> then basically rewritten by Jorge, but preserving the original
> module author credits.
> 
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> Cc: Wei Xu <xuwei5@hisilicon.com>
> Cc: Guodong Xu <guodong.xu@linaro.org>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> [jstultz: Reworked commit message, folded in other fixes/cleanups
> from Jorge, and made a few small fixes and cleanups of my own]
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/input/misc/Kconfig         |   8 ++
>  drivers/input/misc/Makefile        |   1 +
>  drivers/input/misc/hisi_powerkey.c | 228 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 237 insertions(+)
>  create mode 100644 drivers/input/misc/hisi_powerkey.c
> 
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 1f2337a..2e57bbd 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -796,4 +796,12 @@ config INPUT_DRV2667_HAPTICS
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called drv2667-haptics.
>  
> +config HISI_POWERKEY
> +	tristate "Hisilicon PMIC ONKEY support"

Any depends on? Something MACH_XX || COMPILE_TEST?

> +	help
> +	  Say Y to enable support for PMIC ONKEY.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called hisi_powerkey.
> +
>  endif
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 0357a08..f264777 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -75,3 +75,4 @@ obj-$(CONFIG_INPUT_WM831X_ON)		+= wm831x-on.o
>  obj-$(CONFIG_INPUT_XEN_KBDDEV_FRONTEND)	+= xen-kbdfront.o
>  obj-$(CONFIG_INPUT_YEALINK)		+= yealink.o
>  obj-$(CONFIG_INPUT_IDEAPAD_SLIDEBAR)	+= ideapad_slidebar.o
> +obj-$(CONFIG_HISI_POWERKEY)		+= hisi_powerkey.o
> diff --git a/drivers/input/misc/hisi_powerkey.c b/drivers/input/misc/hisi_powerkey.c
> new file mode 100644
> index 0000000..3a35a75
> --- /dev/null
> +++ b/drivers/input/misc/hisi_powerkey.c
> @@ -0,0 +1,228 @@
> +/*
> + * hisi_powerkey.c - Hisilicon MIC powerkey driver

Please drop filename - it may change in tree but I bet nobody will
remember to update it here.

> + *
> + * Copyright (C) 2013 Hisilicon Ltd.
> + * Copyright (C) 2015, 2016 Linaro Ltd.
> + *
> + * This file is subject to the terms and conditions of the GNU General
> + * Public License. See the file "COPYING" in the main directory of this
> + * archive for more details.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/reboot.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_irq.h>
> +#include <linux/input.h>
> +#include <linux/slab.h>
> +
> +/* the above held interrupt will trigger after 4 seconds */
> +#define MAX_HELD_TIME	(4 * MSEC_PER_SEC)
> +
> +
> +typedef irqreturn_t (*hi65xx_irq_handler) (int irq, void *data);
> +enum { id_pressed, id_released, id_held, id_last };
> +static irqreturn_t hi65xx_pkey_irq_handler(int irq, void *q);
> +
> +/*
> + * power key irq information
> + */
> +static struct hi65xx_pkey_irq_info {
> +	hi65xx_irq_handler handler;

They all seem to be using the same handler, drop?

> +	const char *const name;
> +	int irq;
> +} irq_info[id_last] = {
> +	[id_pressed] = {
> +		.handler = hi65xx_pkey_irq_handler,
> +		.name = "down",
> +		.irq = -1,
> +	},
> +	[id_released] = {
> +		.handler = hi65xx_pkey_irq_handler,
> +		.name = "up",
> +		.irq = -1,
> +	},
> +	[id_held] = {
> +		.handler = hi65xx_pkey_irq_handler,
> +		.name = "hold 4s",
> +		.irq = -1,
> +	},
> +};
> +
> +/*
> + * power key events
> + */
> +static struct key_report_pairs {
> +	int code;
> +	int value;
> +} pkey_report[id_last] = {
> +	[id_released] = {
> +		.code = KEY_POWER,
> +		.value = 0
> +	},
> +	[id_pressed] = {
> +		.code = KEY_POWER,
> +		.value = 1
> +	},
> +	[id_held] = {
> +		.code = KEY_RESTART,
> +		.value = 0
> +	},
> +};
> +
> +struct hi65xx_priv {
> +	struct input_dev *input;
> +	struct wakeup_source wlock;

Why do you need custom wakeup source?

> +};
> +
> +static inline void report_key(struct input_dev *dev, int id_action)

No "inline" in C files - let compiler decide.

Pass id_action as enum instead of int.

> +{
> +	/*
> +	 * track the state of the key held event since only ON/OFF values are

Start sentence with a capital letter.

> +	 * allowed on EV_KEY types: KEY_RESTART will always toggle its value to
> +	 * guarantee that the event is passed to handlers (dispossition update).

s/dissposition/disposition.

> +	 */
> +	if (id_action == id_held)
> +		pkey_report[id_held].value ^= 1;

You can fetch the state from input device, no need to modify
module-global. In fact, I do not quite like that we modify both
pkey_report and irq_info. I'd like to have them const static and move
mutable data into hi65xx_priv.

> +
> +	dev_dbg(dev->dev.parent, "received - code %d, value %d\n",
> +		pkey_report[id_action].code,
> +		pkey_report[id_action].value);
> +
> +	input_report_key(dev, pkey_report[id_action].code,
> +		pkey_report[id_action].value);
> +}
> +
> +static irqreturn_t hi65xx_pkey_irq_handler(int irq, void *q)
> +{
> +	struct hi65xx_priv *p = q;
> +	int i, action = -1;

Make action an enum, initialize to "last".

> +
> +	for (i = 0; i < id_last; i++)
> +		if (irq == irq_info[i].irq)
> +			action = i;
> +
> +	if (action == -1)

Compare against "last" here.

> +		return IRQ_NONE;
> +
> +	__pm_wakeup_event(&p->wlock, MAX_HELD_TIME);

Why not pm_wakeup_event?

> +
> +	report_key(p->input, action);
> +	input_sync(p->input);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int hi65xx_powerkey_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct hi65xx_priv *priv;
> +	int irq, i, ret;
> +
> +	if (pdev == NULL) {
> +		dev_err(dev, "parameter error\n");
> +		return -EINVAL;
> +	}

Can't happen.

> +
> +	priv = devm_kzalloc(dev, sizeof(struct hi65xx_priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->input = input_allocate_device();

devm_input_allocate_devivce(&pdev->dev);

> +	if (!priv->input) {
> +		dev_err(&pdev->dev, "failed to allocate input device\n");
> +		return -ENOENT;

-ENOMEM

> +	}
> +
> +	priv->input->evbit[0] = BIT_MASK(EV_KEY);

Not needed - input_set_capability() will do this for us.

> +	priv->input->dev.parent = &pdev->dev;

Drop if switching to devm.

> +	priv->input->phys = "hisi_on/input0";
> +	priv->input->name = "HISI 65xx PowerOn Key";
> +
> +	for (i = 0; i < ARRAY_SIZE(pkey_report); i++)
> +		input_set_capability(priv->input, EV_KEY, pkey_report[i].code);
> +
> +	for (i = 0; i < ARRAY_SIZE(irq_info); i++) {
> +
> +		irq = platform_get_irq_byname(pdev, irq_info[i].name);
> +		if (irq < 0) {
> +			dev_err(dev, "couldn't get irq %s\n", irq_info[i].name);
> +			ret = irq;
> +			goto err_irq;
> +		}
> +
> +		ret = devm_request_threaded_irq(dev, irq, NULL,
> +					irq_info[i].handler, IRQF_ONESHOT,
> +					irq_info[i].name, priv);

Why threaded irq? Seems wasteful to have 3 threads for this.

> +		if (ret < 0) {
> +			dev_err(dev, "couldn't get irq %s\n", irq_info[i].name);
> +			goto err_irq;
> +		}
> +
> +		irq_info[i].irq = irq;
> +	}
> +
> +	wakeup_source_init(&priv->wlock, "hisi-powerkey");

Why not the standard device_init_wakeup()?

> +
> +	ret = input_register_device(priv->input);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register input device: %d\n",
> +			ret);
> +		ret = -ENOENT;

Why do you discard perfectly good error code from
input_register_device()?

> +		goto err_register;
> +	}
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;
> +
> +err_register:
> +	wakeup_source_trash(&priv->wlock);
> +err_irq:
> +	input_free_device(priv->input);

Drop if switching to devm.

> +
> +	return ret;
> +}
> +
> +static int hi65xx_powerkey_remove(struct platform_device *pdev)
> +{
> +	struct hi65xx_priv *priv = platform_get_drvdata(pdev);
> +
> +	wakeup_source_trash(&priv->wlock);
> +	input_unregister_device(priv->input);

Drop if moving to devm.

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id hi65xx_powerkey_of_match[] = {
> +	{ .compatible = "hisilicon,hi6552-powerkey", },
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, hi65xx_powerkey_of_match);
> +
> +static struct platform_driver hi65xx_powerkey_driver = {
> +	.driver = {
> +		.owner = THIS_MODULE,

No need to set owner.

> +		.name = "hi65xx-powerkey",
> +		.of_match_table = hi65xx_powerkey_of_match,

of_match_ptr()?

> +	},
> +	.probe = hi65xx_powerkey_probe,
> +	.remove  = hi65xx_powerkey_remove,
> +};
> +
> +module_platform_driver(hi65xx_powerkey_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Zhiliang Xue <xuezhiliang@huawei.com");
> +MODULE_DESCRIPTION("Hisi PMIC Power key driver");
> +MODULE_LICENSE("GPL v2");

2 module licences? I guess GPL v2 is the right one, drop the first one
please.

> +
> +

Drop 2 ending empty lines.

> -- 
> 1.9.1
> 

Thanks.

-- 
Dmitry

Re: [RFC][PATCH 4/5] drivers: input: powerkey for HISI 65xx SoC

[John Stultz]

On Wed, Jun 1, 2016 at 7:10 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi John,
>
> On Wed, Jun 01, 2016 at 02:27:39PM -0700, John Stultz wrote:
>> From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>>
>> This driver provides a input driver for the power button on the
>> HiSi 65xx SoC for boards like HiKey.
>>
>> This driver was originally by Zhiliang Xue <xuezhiliang@huawei.com>
>> then basically rewritten by Jorge, but preserving the original
>> module author credits.
>>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Pawel Moll <pawel.moll@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>> Cc: Kumar Gala <galak@codeaurora.org>
>> Cc: Lee Jones <lee.jones@linaro.org>
>> Cc: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>> Cc: Wei Xu <xuwei5@hisilicon.com>
>> Cc: Guodong Xu <guodong.xu@linaro.org>
>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>> [jstultz: Reworked commit message, folded in other fixes/cleanups
>> from Jorge, and made a few small fixes and cleanups of my own]
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>> ---
>>  drivers/input/misc/Kconfig         |   8 ++
>>  drivers/input/misc/Makefile        |   1 +
>>  drivers/input/misc/hisi_powerkey.c | 228 +++++++++++++++++++++++++++++++++++++
>>  3 files changed, 237 insertions(+)
>>  create mode 100644 drivers/input/misc/hisi_powerkey.c
>>
>> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
>> index 1f2337a..2e57bbd 100644
>> --- a/drivers/input/misc/Kconfig
>> +++ b/drivers/input/misc/Kconfig
>> @@ -796,4 +796,12 @@ config INPUT_DRV2667_HAPTICS
>>         To compile this driver as a module, choose M here: the
>>         module will be called drv2667-haptics.
>>
>> +config HISI_POWERKEY
>> +     tristate "Hisilicon PMIC ONKEY support"
>
> Any depends on? Something MACH_XX || COMPILE_TEST?

Hey Dmitry!

Thanks so much for the review! I've got almost all your suggestions
integrated (and it greatly simplifies things) and will resend
tomorrow.

One comment on your question below...

>> +             ret = devm_request_threaded_irq(dev, irq, NULL,
>> +                                     irq_info[i].handler, IRQF_ONESHOT,
>> +                                     irq_info[i].name, priv);
>
> Why threaded irq? Seems wasteful to have 3 threads for this.

As this is a nested interrupt, devm_request_irq was failing unless it
was threaded.
Any ideas for something better?

thanks
-john

Re: [RFC][PATCH 4/5] drivers: input: powerkey for HISI 65xx SoC

[Dmitry Torokhov]

On Thu, Jun 02, 2016 at 03:10:33PM -0700, John Stultz wrote:
> On Wed, Jun 1, 2016 at 7:10 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Hi John,
> >
> > On Wed, Jun 01, 2016 at 02:27:39PM -0700, John Stultz wrote:
> >> From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> >>
> >> This driver provides a input driver for the power button on the
> >> HiSi 65xx SoC for boards like HiKey.
> >>
> >> This driver was originally by Zhiliang Xue <xuezhiliang@huawei.com>
> >> then basically rewritten by Jorge, but preserving the original
> >> module author credits.
> >>
> >> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >> Cc: Rob Herring <robh+dt@kernel.org>
> >> Cc: Pawel Moll <pawel.moll@arm.com>
> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> >> Cc: Kumar Gala <galak@codeaurora.org>
> >> Cc: Lee Jones <lee.jones@linaro.org>
> >> Cc: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> >> Cc: Wei Xu <xuwei5@hisilicon.com>
> >> Cc: Guodong Xu <guodong.xu@linaro.org>
> >> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> >> [jstultz: Reworked commit message, folded in other fixes/cleanups
> >> from Jorge, and made a few small fixes and cleanups of my own]
> >> Signed-off-by: John Stultz <john.stultz@linaro.org>
> >> ---
> >>  drivers/input/misc/Kconfig         |   8 ++
> >>  drivers/input/misc/Makefile        |   1 +
> >>  drivers/input/misc/hisi_powerkey.c | 228 +++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 237 insertions(+)
> >>  create mode 100644 drivers/input/misc/hisi_powerkey.c
> >>
> >> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> >> index 1f2337a..2e57bbd 100644
> >> --- a/drivers/input/misc/Kconfig
> >> +++ b/drivers/input/misc/Kconfig
> >> @@ -796,4 +796,12 @@ config INPUT_DRV2667_HAPTICS
> >>         To compile this driver as a module, choose M here: the
> >>         module will be called drv2667-haptics.
> >>
> >> +config HISI_POWERKEY
> >> +     tristate "Hisilicon PMIC ONKEY support"
> >
> > Any depends on? Something MACH_XX || COMPILE_TEST?
> 
> Hey Dmitry!
> 
> Thanks so much for the review! I've got almost all your suggestions
> integrated (and it greatly simplifies things) and will resend
> tomorrow.

Actually, I was thinking about it some more and I wonder if it would not
be even simpler if we had 3 separate interrupt handlers, one for key
press, one fore key release, and another for toggling. Then you woudl
not need to iterate through IRQ numbers to figure out the action and
interrupt handlers would be really tiny:

static irqreturn_t hi65xx_power_press_isr(int irq, void *q)
{
	struct hi65xx_priv *p = q;

	pm_wakeup_event(&p->dev, MAX_HELD_TIME);
	input_report_key(p->input, KEY_POWER, 1);
	input_sync(p->input);
}

static irqreturn_t hi65xx_power_release_isr(int irq, void *q)
{
	struct hi65xx_priv *p = q;

	pm_wakeup_event(&p->dev, MAX_HELD_TIME); // Needed ?
	input_report_key(p->input, KEY_POWER, 0);
	input_sync(p->input);
}

static irqreturn_t hi65xx_restart_toggle_isr(int irq, void *q)
{
	struct hi65xx_priv *p = q;
	int value = test_bit(KEY_RESTART, p->input->keys);

	pm_wakeup_event(&p->dev, MAX_HELD_TIME);
	input_report_key(p->input, KEY_RESTART, !value);
	input_sync(p->input);
}

static struct hi65xx_isr_info {
	const char *name;
	irqreturn_t (*handler)(int irq, void *q);
} hi65xx_isr_info[] = {
	{ .name = "down", .handler = hi65xx_power_press_isr },
	{ .name = "up", .handler = hi65xx_power_release_isr },
	{ .name = "hold 4s", .handler = hi65xx_restart_toggle_isr },
};

> 
> One comment on your question below...
> 
> >> +             ret = devm_request_threaded_irq(dev, irq, NULL,
> >> +                                     irq_info[i].handler, IRQF_ONESHOT,
> >> +                                     irq_info[i].name, priv);
> >
> > Why threaded irq? Seems wasteful to have 3 threads for this.
> 
> As this is a nested interrupt, devm_request_irq was failing unless it
> was threaded.
> Any ideas for something better?

Ahh, that is unfortunate, but I guess we'll have to live with it. Please
use devm_request_any_context_irq() then to show that the driver itself
does not need threaded interrupt but platform may provide it.

Thanks.

-- 
Dmitry

Re: [RFC][PATCH 4/5] drivers: input: powerkey for HISI 65xx SoC

[John Stultz]

On Thu, Jun 2, 2016 at 3:47 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Jun 02, 2016 at 03:10:33PM -0700, John Stultz wrote:
>> On Wed, Jun 1, 2016 at 7:10 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > Hi John,
>> >
>> > On Wed, Jun 01, 2016 at 02:27:39PM -0700, John Stultz wrote:
>> >> From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>> >>
>> >> This driver provides a input driver for the power button on the
>> >> HiSi 65xx SoC for boards like HiKey.
>> >>
>> >> This driver was originally by Zhiliang Xue <xuezhiliang@huawei.com>
>> >> then basically rewritten by Jorge, but preserving the original
>> >> module author credits.
>> >>
>> >> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> >> Cc: Rob Herring <robh+dt@kernel.org>
>> >> Cc: Pawel Moll <pawel.moll@arm.com>
>> >> Cc: Mark Rutland <mark.rutland@arm.com>
>> >> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
>> >> Cc: Kumar Gala <galak@codeaurora.org>
>> >> Cc: Lee Jones <lee.jones@linaro.org>
>> >> Cc: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>> >> Cc: Wei Xu <xuwei5@hisilicon.com>
>> >> Cc: Guodong Xu <guodong.xu@linaro.org>
>> >> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>> >> [jstultz: Reworked commit message, folded in other fixes/cleanups
>> >> from Jorge, and made a few small fixes and cleanups of my own]
>> >> Signed-off-by: John Stultz <john.stultz@linaro.org>
>> >> ---
>> >>  drivers/input/misc/Kconfig         |   8 ++
>> >>  drivers/input/misc/Makefile        |   1 +
>> >>  drivers/input/misc/hisi_powerkey.c | 228 +++++++++++++++++++++++++++++++++++++
>> >>  3 files changed, 237 insertions(+)
>> >>  create mode 100644 drivers/input/misc/hisi_powerkey.c
>> >>
>> >> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
>> >> index 1f2337a..2e57bbd 100644
>> >> --- a/drivers/input/misc/Kconfig
>> >> +++ b/drivers/input/misc/Kconfig
>> >> @@ -796,4 +796,12 @@ config INPUT_DRV2667_HAPTICS
>> >>         To compile this driver as a module, choose M here: the
>> >>         module will be called drv2667-haptics.
>> >>
>> >> +config HISI_POWERKEY
>> >> +     tristate "Hisilicon PMIC ONKEY support"
>> >
>> > Any depends on? Something MACH_XX || COMPILE_TEST?
>>
>> Hey Dmitry!
>>
>> Thanks so much for the review! I've got almost all your suggestions
>> integrated (and it greatly simplifies things) and will resend
>> tomorrow.
>
> Actually, I was thinking about it some more and I wonder if it would not
> be even simpler if we had 3 separate interrupt handlers, one for key
> press, one fore key release, and another for toggling. Then you woudl
> not need to iterate through IRQ numbers to figure out the action and
> interrupt handlers would be really tiny:
>
> static irqreturn_t hi65xx_power_press_isr(int irq, void *q)
> {
>         struct hi65xx_priv *p = q;
>
>         pm_wakeup_event(&p->dev, MAX_HELD_TIME);
>         input_report_key(p->input, KEY_POWER, 1);
>         input_sync(p->input);
> }
>
> static irqreturn_t hi65xx_power_release_isr(int irq, void *q)
> {
>         struct hi65xx_priv *p = q;
>
>         pm_wakeup_event(&p->dev, MAX_HELD_TIME); // Needed ?
>         input_report_key(p->input, KEY_POWER, 0);
>         input_sync(p->input);
> }
>
> static irqreturn_t hi65xx_restart_toggle_isr(int irq, void *q)
> {
>         struct hi65xx_priv *p = q;
>         int value = test_bit(KEY_RESTART, p->input->keys);
>
>         pm_wakeup_event(&p->dev, MAX_HELD_TIME);
>         input_report_key(p->input, KEY_RESTART, !value);
>         input_sync(p->input);
> }
>
> static struct hi65xx_isr_info {
>         const char *name;
>         irqreturn_t (*handler)(int irq, void *q);
> } hi65xx_isr_info[] = {
>         { .name = "down", .handler = hi65xx_power_press_isr },
>         { .name = "up", .handler = hi65xx_power_release_isr },
>         { .name = "hold 4s", .handler = hi65xx_restart_toggle_isr },
> };

Hrm.. Ok. I can rework it this way. If you were curious, here's what
the code currently looks like:
https://git.linaro.org/people/john.stultz/android-dev.git/commitdiff/2b0de7d8ba22d188e961a4ffef6d95030ef31c15

But I'll rework it and submit it tomorrow.


>> One comment on your question below...
>>
>> >> +             ret = devm_request_threaded_irq(dev, irq, NULL,
>> >> +                                     irq_info[i].handler, IRQF_ONESHOT,
>> >> +                                     irq_info[i].name, priv);
>> >
>> > Why threaded irq? Seems wasteful to have 3 threads for this.
>>
>> As this is a nested interrupt, devm_request_irq was failing unless it
>> was threaded.
>> Any ideas for something better?
>
> Ahh, that is unfortunate, but I guess we'll have to live with it. Please
> use devm_request_any_context_irq() then to show that the driver itself
> does not need threaded interrupt but platform may provide it.

Sounds good. Will do.

thanks
-john

Re: [RFC][PATCH 4/5] drivers: input: powerkey for HISI 65xx SoC

[Dmitry Torokhov]

On Thu, Jun 02, 2016 at 04:15:27PM -0700, John Stultz wrote:
> On Thu, Jun 2, 2016 at 3:47 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Thu, Jun 02, 2016 at 03:10:33PM -0700, John Stultz wrote:
> >> On Wed, Jun 1, 2016 at 7:10 PM, Dmitry Torokhov
> >> <dmitry.torokhov@gmail.com> wrote:
> >> > Hi John,
> >> >
> >> > On Wed, Jun 01, 2016 at 02:27:39PM -0700, John Stultz wrote:
> >> >> From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> >> >>
> >> >> This driver provides a input driver for the power button on the
> >> >> HiSi 65xx SoC for boards like HiKey.
> >> >>
> >> >> This driver was originally by Zhiliang Xue <xuezhiliang@huawei.com>
> >> >> then basically rewritten by Jorge, but preserving the original
> >> >> module author credits.
> >> >>
> >> >> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >> >> Cc: Rob Herring <robh+dt@kernel.org>
> >> >> Cc: Pawel Moll <pawel.moll@arm.com>
> >> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> >> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> >> >> Cc: Kumar Gala <galak@codeaurora.org>
> >> >> Cc: Lee Jones <lee.jones@linaro.org>
> >> >> Cc: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> >> >> Cc: Wei Xu <xuwei5@hisilicon.com>
> >> >> Cc: Guodong Xu <guodong.xu@linaro.org>
> >> >> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> >> >> [jstultz: Reworked commit message, folded in other fixes/cleanups
> >> >> from Jorge, and made a few small fixes and cleanups of my own]
> >> >> Signed-off-by: John Stultz <john.stultz@linaro.org>
> >> >> ---
> >> >>  drivers/input/misc/Kconfig         |   8 ++
> >> >>  drivers/input/misc/Makefile        |   1 +
> >> >>  drivers/input/misc/hisi_powerkey.c | 228 +++++++++++++++++++++++++++++++++++++
> >> >>  3 files changed, 237 insertions(+)
> >> >>  create mode 100644 drivers/input/misc/hisi_powerkey.c
> >> >>
> >> >> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> >> >> index 1f2337a..2e57bbd 100644
> >> >> --- a/drivers/input/misc/Kconfig
> >> >> +++ b/drivers/input/misc/Kconfig
> >> >> @@ -796,4 +796,12 @@ config INPUT_DRV2667_HAPTICS
> >> >>         To compile this driver as a module, choose M here: the
> >> >>         module will be called drv2667-haptics.
> >> >>
> >> >> +config HISI_POWERKEY
> >> >> +     tristate "Hisilicon PMIC ONKEY support"
> >> >
> >> > Any depends on? Something MACH_XX || COMPILE_TEST?
> >>
> >> Hey Dmitry!
> >>
> >> Thanks so much for the review! I've got almost all your suggestions
> >> integrated (and it greatly simplifies things) and will resend
> >> tomorrow.
> >
> > Actually, I was thinking about it some more and I wonder if it would not
> > be even simpler if we had 3 separate interrupt handlers, one for key
> > press, one fore key release, and another for toggling. Then you woudl
> > not need to iterate through IRQ numbers to figure out the action and
> > interrupt handlers would be really tiny:
> >
> > static irqreturn_t hi65xx_power_press_isr(int irq, void *q)
> > {
> >         struct hi65xx_priv *p = q;
> >
> >         pm_wakeup_event(&p->dev, MAX_HELD_TIME);
> >         input_report_key(p->input, KEY_POWER, 1);
> >         input_sync(p->input);
> > }
> >
> > static irqreturn_t hi65xx_power_release_isr(int irq, void *q)
> > {
> >         struct hi65xx_priv *p = q;
> >
> >         pm_wakeup_event(&p->dev, MAX_HELD_TIME); // Needed ?
> >         input_report_key(p->input, KEY_POWER, 0);
> >         input_sync(p->input);
> > }
> >
> > static irqreturn_t hi65xx_restart_toggle_isr(int irq, void *q)
> > {
> >         struct hi65xx_priv *p = q;
> >         int value = test_bit(KEY_RESTART, p->input->keys);
> >
> >         pm_wakeup_event(&p->dev, MAX_HELD_TIME);
> >         input_report_key(p->input, KEY_RESTART, !value);
> >         input_sync(p->input);
> > }
> >
> > static struct hi65xx_isr_info {
> >         const char *name;
> >         irqreturn_t (*handler)(int irq, void *q);
> > } hi65xx_isr_info[] = {
> >         { .name = "down", .handler = hi65xx_power_press_isr },
> >         { .name = "up", .handler = hi65xx_power_release_isr },
> >         { .name = "hold 4s", .handler = hi65xx_restart_toggle_isr },
> > };
> 
> Hrm.. Ok. I can rework it this way. If you were curious, here's what
> the code currently looks like:
> https://git.linaro.org/people/john.stultz/android-dev.git/commitdiff/2b0de7d8ba22d188e961a4ffef6d95030ef31c15
> 
> But I'll rework it and submit it tomorrow.

Actually, it is up to you, I'm fine with the above as well. You can
probably drop of_irq.h form there though.

> 
> 
> >> One comment on your question below...
> >>
> >> >> +             ret = devm_request_threaded_irq(dev, irq, NULL,
> >> >> +                                     irq_info[i].handler, IRQF_ONESHOT,
> >> >> +                                     irq_info[i].name, priv);
> >> >
> >> > Why threaded irq? Seems wasteful to have 3 threads for this.
> >>
> >> As this is a nested interrupt, devm_request_irq was failing unless it
> >> was threaded.
> >> Any ideas for something better?
> >
> > Ahh, that is unfortunate, but I guess we'll have to live with it. Please
> > use devm_request_any_context_irq() then to show that the driver itself
> > does not need threaded interrupt but platform may provide it.
> 
> Sounds good. Will do.

Thanks.

-- 
Dmitry

[RFC][PATCH 5/5] arm64: dts: Add powerkey info to pmic for hi6220-hikey

[John Stultz]

From: Guodong Xu <guodong.xu@linaro.org>

Add powerkey entry to the HiKey dts.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
Cc: Wei Xu <xuwei5@hisilicon.com>
Cc: Guodong Xu <guodong.xu@linaro.org>
Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
index e92a30c..c8e49a6 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
@@ -144,6 +144,13 @@
 		#interrupt-cells = <2>;
 		pmic-gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
 
+		powerkey:powerkey@b1{
+			compatible = "hisilicon,hi6552-powerkey";
+			interrupt-parent = <&pmic>;
+			interrupts = <6 0>, <5 0>, <4 0>;
+			interrupt-names = "down", "up", "hold 4s";
+		};
+
 		regulators {
 			ldo2: LDO2 {
 				regulator-name = "LDO2_2V8";
-- 
1.9.1

Re: [RFC][PATCH 5/5] arm64: dts: Add powerkey info to pmic for hi6220-hikey

[Rob Herring]

On Wed, Jun 1, 2016 at 4:27 PM, John Stultz <john.stultz@linaro.org> wrote:
> From: Guodong Xu <guodong.xu@linaro.org>
>
> Add powerkey entry to the HiKey dts.
>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> Cc: Wei Xu <xuwei5@hisilicon.com>
> Cc: Guodong Xu <guodong.xu@linaro.org>
> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> index e92a30c..c8e49a6 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> @@ -144,6 +144,13 @@
>                 #interrupt-cells = <2>;
>                 pmic-gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
>
> +               powerkey:powerkey@b1{

nit: you probably don't need the label and unit-address is not needed, so just:

powerkey {

> +                       compatible = "hisilicon,hi6552-powerkey";
> +                       interrupt-parent = <&pmic>;
> +                       interrupts = <6 0>, <5 0>, <4 0>;
> +                       interrupt-names = "down", "up", "hold 4s";
> +               };
> +
>                 regulators {
>                         ldo2: LDO2 {
>                                 regulator-name = "LDO2_2V8";
> --
> 1.9.1
>