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

[John Stultz]

On Thu, Jun 2, 2016 at 3:47 PM, Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> 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@xxxxxxxxx> wrote:
>> > Hi John,
>> >
>> > On Wed, Jun 01, 2016 at 02:27:39PM -0700, John Stultz wrote:
>> >> From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@xxxxxxxxxx>
>> >>
>> >> 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@xxxxxxxxxx>
>> >> then basically rewritten by Jorge, but preserving the original
>> >> module author credits.
>> >>
>> >> Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
>> >> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
>> >> Cc: Pawel Moll <pawel.moll@xxxxxxx>
>> >> Cc: Mark Rutland <mark.rutland@xxxxxxx>
>> >> Cc: Ian Campbell <ijc+devicetree@xxxxxxxxxxxxxx>
>> >> Cc: Kumar Gala <galak@xxxxxxxxxxxxxx>
>> >> Cc: Lee Jones <lee.jones@xxxxxxxxxx>
>> >> Cc: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@xxxxxxxxxx>
>> >> Cc: Wei Xu <xuwei5@xxxxxxxxxxxxx>
>> >> Cc: Guodong Xu <guodong.xu@xxxxxxxxxx>
>> >> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@xxxxxxxxxx>
>> >> [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@xxxxxxxxxx>
>> >> ---
>> >>  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