Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1411544 > unrolled thread

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

Started byJohn Stultz <john.stultz@linaro.org>
First post2016-06-01 23:30 +0200
Last post2016-06-03 01:40 +0200
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC][PATCH 4/5] drivers: input: powerkey for HISI 65xx SoC John Stultz <john.stultz@linaro.org> - 2016-06-01 23:30 +0200
    Re: [RFC][PATCH 4/5] drivers: input: powerkey for HISI 65xx SoC Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-06-02 04:20 +0200
      Re: [RFC][PATCH 4/5] drivers: input: powerkey for HISI 65xx SoC John Stultz <john.stultz@linaro.org> - 2016-06-03 00:20 +0200
        Re: [RFC][PATCH 4/5] drivers: input: powerkey for HISI 65xx SoC Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-06-03 00:50 +0200
          Re: [RFC][PATCH 4/5] drivers: input: powerkey for HISI 65xx SoC John Stultz <john.stultz@linaro.org> - 2016-06-03 01:20 +0200
            Re: [RFC][PATCH 4/5] drivers: input: powerkey for HISI 65xx SoC Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-06-03 01:40 +0200

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

FromJohn Stultz <john.stultz@linaro.org>
Date2016-06-01 23:30 +0200
Subject[RFC][PATCH 4/5] drivers: input: powerkey for HISI 65xx SoC
Message-ID<rFlSa-5vL-13@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1411766

FromDmitry Torokhov <dmitry.torokhov@gmail.com>
Date2016-06-02 04:20 +0200
Message-ID<rFqoO-8qx-5@gated-at.bofh.it>
In reply to#1411544
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

[toc] | [prev] | [next] | [standalone]

#1412586

FromJohn Stultz <john.stultz@linaro.org>
Date2016-06-03 00:20 +0200
Message-ID<rFJ86-3ml-19@gated-at.bofh.it>
In reply to#1411766
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

[toc] | [prev] | [next] | [standalone]

#1412592

FromDmitry Torokhov <dmitry.torokhov@gmail.com>
Date2016-06-03 00:50 +0200
Message-ID<rFJB7-3vM-1@gated-at.bofh.it>
In reply to#1412586
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

[toc] | [prev] | [next] | [standalone]

#1412608

FromJohn Stultz <john.stultz@linaro.org>
Date2016-06-03 01:20 +0200
Message-ID<rFK49-3Uu-5@gated-at.bofh.it>
In reply to#1412592
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

[toc] | [prev] | [next] | [standalone]

#1412616

FromDmitry Torokhov <dmitry.torokhov@gmail.com>
Date2016-06-03 01:40 +0200
Message-ID<rFKnv-40T-15@gated-at.bofh.it>
In reply to#1412608
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web