From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161079AbcFBXPa (ORCPT ); Thu, 2 Jun 2016 19:15:30 -0400 Received: from mail-oi0-f42.google.com ([209.85.218.42]:35892 "EHLO mail-oi0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932090AbcFBXP2 (ORCPT ); Thu, 2 Jun 2016 19:15:28 -0400 MIME-Version: 1.0 In-Reply-To: <20160602224728.GA34193@dtor-ws> References: <1464816460-18750-1-git-send-email-john.stultz@linaro.org> <1464816460-18750-5-git-send-email-john.stultz@linaro.org> <20160602021019.GA28273@dtor-ws> <20160602224728.GA34193@dtor-ws> Date: Thu, 2 Jun 2016 16:15:27 -0700 Message-ID: Subject: Re: [RFC][PATCH 4/5] drivers: input: powerkey for HISI 65xx SoC From: John Stultz To: Dmitry Torokhov Cc: lkml , Jorge Ramirez-Ortiz , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Lee Jones , Wei Xu , Guodong Xu Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jun 2, 2016 at 3:47 PM, Dmitry Torokhov 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 >> wrote: >> > 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? >> >> 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