| 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: Thu, 2 Jun 2016 16:33:47 -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
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@...il.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@...il.com> wrote:
> >> > 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?
> >>
> >> 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
Powered by blists - more mailing lists