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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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