| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 1 Jun 2016 19:10:19 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: John Stultz <john.stultz@...aro.org>
Cc: lkml <linux-kernel@...r.kernel.org>,
Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
Lee Jones <lee.jones@...aro.org>,
Wei Xu <xuwei5@...ilicon.com>,
Guodong Xu <guodong.xu@...aro.org>
Subject: Re: [RFC][PATCH 4/5] drivers: input: powerkey for HISI 65xx SoC
Hi John,
On Wed, Jun 01, 2016 at 02:27:39PM -0700, John Stultz wrote:
> From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@...aro.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@...wei.com>
> then basically rewritten by Jorge, but preserving the original
> module author credits.
>
> Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>
> Cc: Rob Herring <robh+dt@...nel.org>
> Cc: Pawel Moll <pawel.moll@....com>
> Cc: Mark Rutland <mark.rutland@....com>
> Cc: Ian Campbell <ijc+devicetree@...lion.org.uk>
> Cc: Kumar Gala <galak@...eaurora.org>
> Cc: Lee Jones <lee.jones@...aro.org>
> Cc: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@...aro.org>
> Cc: Wei Xu <xuwei5@...ilicon.com>
> Cc: Guodong Xu <guodong.xu@...aro.org>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@...aro.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@...aro.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@...wei.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
Powered by blists - more mailing lists