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

On Tue, Mar 13, 2018 at 07:40:48PM -0700, Bjorn Andersson wrote:

On Tue 13 Mar 01:35 PDT 2018, Tirupathi Reddy wrote:

quoted

Add resin key support to handle different types of key events
defined in different platforms.

Signed-off-by: Tirupathi Reddy <redacted>
---
 .../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.

quoted

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

quoted

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

quoted

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

quoted

+	input_sync(pwrkey->input);
+
+	return IRQ_HANDLED;
+}
+

Regards,
Bjorn

-- 
Dmitry