Re: [PATCH V2] Input: pm8941-pwrkey: add resin key capabilities

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Tue, Mar 13, 2018 at 07:40:48PM -0700, Bjorn Andersson wrote:
> On Tue 13 Mar 01:35 PDT 2018, Tirupathi Reddy wrote:
> 
> > Add resin key support to handle different types of key events
> > defined in different platforms.
> > 
> > Signed-off-by: Tirupathi Reddy <tirupath@codeaurora.org>
> > ---
> >  .../bindings/input/qcom,pm8941-pwrkey.txt          | 20 ++++++-
> >  drivers/input/misc/pm8941-pwrkey.c                 | 65 +++++++++++++++++++++-
> >  2 files changed, 83 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/qcom,pm8941-pwrkey.txt b/Documentation/devicetree/bindings/input/qcom,pm8941-pwrkey.txt
> > index 07bf55f..f499cf8 100644
> > --- a/Documentation/devicetree/bindings/input/qcom,pm8941-pwrkey.txt
> > +++ b/Documentation/devicetree/bindings/input/qcom,pm8941-pwrkey.txt
> > @@ -20,6 +20,14 @@ PROPERTIES
> >  		    defined by the binding document describing the node's
> >  		    interrupt parent.
> >  
> > +- interrupt-names:
> > +	Usage: required
> > +	Value type: <stringlist>
> > +	Definition: Interrupt names.  This list must match up 1-to-1 with the
> > +		    interrupts specified in the 'interrupts' property. "kpdpwr"
> > +		    must be specified and should be the first entry of the list.
> > +		    "resin" may be specified for some platforms.
> > +
> >  - debounce:
> >  	Usage: optional
> >  	Value type: <u32>
> > @@ -32,12 +40,22 @@ PROPERTIES
> >  	Definition: presence of this property indicates that the KPDPWR_N pin
> >  		    should be configured for pull up.
> >  
> > +- linux,code:
> > +	Usage: required for "resin" key
> > +	Value type: <u32>
> > +	Definition: The input key-code associated with the resin key.
> > +		    Use the linux event codes defined in
> > +		    include/dt-bindings/input/linux-event-codes.h
> 
> For completeness sake I think this should list both the key code for the
> power key and for the RESIN key.
> 
> > +
> >  EXAMPLE
> >  
> >  	pwrkey@800 {
> >  		compatible = "qcom,pm8941-pwrkey";
> >  		reg = <0x800>;
> > -		interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>;
> > +		interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>,
> > +			     <0x0 0x8 1 IRQ_TYPE_EDGE_BOTH>;
> > +		interrupt-names = "kpdpwr", "resin";
> >  		debounce = <15625>;
> >  		bias-pull-up;
> 
> Do we need to support defining the bias for the resin as well?
> 
> Perhaps it makes more sense to describe the RESIN key with a separate
> compatible and define that in a node of its own (we can still implement
> this in the same driver - if there's any chance of reuse...).
> 
> > +		linux,code = <KEY_VOLUMEDOWN>;
> >  	};
> > diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
> > index 18ad956..2fffc7c 100644
> > --- a/drivers/input/misc/pm8941-pwrkey.c
> > +++ b/drivers/input/misc/pm8941-pwrkey.c
> > @@ -28,6 +28,7 @@
> >  
> >  #define PON_RT_STS			0x10
> >  #define  PON_KPDPWR_N_SET		BIT(0)
> > +#define  PON_RESIN_N_SET		BIT(1)
> >  
> >  #define PON_PS_HOLD_RST_CTL		0x5a
> >  #define PON_PS_HOLD_RST_CTL2		0x5b
> > @@ -46,6 +47,7 @@
> >  struct pm8941_pwrkey {
> >  	struct device *dev;
> >  	int irq;
> > +	u32 resin_key_code;
> >  	u32 baseaddr;
> >  	struct regmap *regmap;
> >  	struct input_dev *input;
> > @@ -130,6 +132,24 @@ static irqreturn_t pm8941_pwrkey_irq(int irq, void *_data)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > +static irqreturn_t pm8941_resinkey_irq(int irq, void *_data)
> > +{
> > +	struct pm8941_pwrkey *pwrkey = _data;
> > +	unsigned int sts;
> > +	int error;
> > +
> > +	error = regmap_read(pwrkey->regmap,
> > +			    pwrkey->baseaddr + PON_RT_STS, &sts);
> 
> This needs to be broken because the line is 81 chars long, if you use a
> shorter name for the return value (e.g. "ret") you don't need to do
> this.

I prefer keeping it as "error" please.

> 
> > +	if (error)
> > +		return IRQ_HANDLED;
> > +
> > +	input_report_key(pwrkey->input, pwrkey->resin_key_code,
> > +			 !!(sts & PON_RESIN_N_SET));
> 
> Put pwrkey->resin_key_code in a local variable named "key" (or even
> key_code) and you don't need to line wrap this one.
> 
> > +	input_sync(pwrkey->input);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> 
> Regards,
> Bjorn

-- 
Dmitry