From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752202AbcFBCK1 (ORCPT ); Wed, 1 Jun 2016 22:10:27 -0400 Received: from mail-pf0-f194.google.com ([209.85.192.194]:36541 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751828AbcFBCKZ (ORCPT ); Wed, 1 Jun 2016 22:10:25 -0400 Date: Wed, 1 Jun 2016 19:10:19 -0700 From: Dmitry Torokhov To: John Stultz Cc: lkml , Jorge Ramirez-Ortiz , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Lee Jones , Wei Xu , Guodong Xu Subject: Re: [RFC][PATCH 4/5] drivers: input: powerkey for HISI 65xx SoC Message-ID: <20160602021019.GA28273@dtor-ws> References: <1464816460-18750-1-git-send-email-john.stultz@linaro.org> <1464816460-18750-5-git-send-email-john.stultz@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1464816460-18750-5-git-send-email-john.stultz@linaro.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi John, On Wed, Jun 01, 2016 at 02:27:39PM -0700, John Stultz wrote: > From: Jorge Ramirez-Ortiz > > 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 > then basically rewritten by Jorge, but preserving the original > module author credits. > > Cc: Dmitry Torokhov > Cc: Rob Herring > Cc: Pawel Moll > Cc: Mark Rutland > Cc: Ian Campbell > Cc: Kumar Gala > Cc: Lee Jones > Cc: Jorge Ramirez-Ortiz > Cc: Wei Xu > Cc: Guodong Xu > Signed-off-by: Jorge Ramirez-Ortiz > [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 > --- > 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* 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 +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