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

[Dmitry Torokhov]

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 },
};

> 
> 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.

Thanks.

-- 
Dmitry