[PATCH 0/2] Qualcomm PM8941 power key driver

These patches add dt bindings and a device driver for the power key block in
the Qualcomm PM8941 pmic.

Courtney Cavin (2):
  input: Add Qualcomm PM8941 power key driver
  input: pm8941-pwrkey: Add DT binding documentation

 .../bindings/input/qcom,pm8941-pwrkey.txt          |   43 +++++
 drivers/input/misc/Kconfig                         |   12 ++
 drivers/input/misc/Makefile                        |    1 +
 drivers/input/misc/pm8941-pwrkey.c                 |  196 ++++++++++++++++++++
 4 files changed, 252 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/qcom,pm8941-pwrkey.txt
 create mode 100644 drivers/input/misc/pm8941-pwrkey.c

-- 
1.7.9.5

From: Courtney Cavin <redacted>

Signed-off-by: Courtney Cavin <redacted>
Signed-off-by: Bjorn Andersson <redacted>
---
 drivers/input/misc/Kconfig         |   12 +++
 drivers/input/misc/Makefile        |    1 +
 drivers/input/misc/pm8941-pwrkey.c |  196 ++++++++++++++++++++++++++++++++++++
 3 files changed, 209 insertions(+)
 create mode 100644 drivers/input/misc/pm8941-pwrkey.c

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 2ff4425..abe615c 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig

@@ -105,6 +105,18 @@ config INPUT_PCSPKR
 	  To compile this driver as a module, choose M here: the
 	  module will be called pcspkr.
 
+config INPUT_PM8941_PWRKEY
+	tristate "Qualcomm PM8X41 power key support"
+	depends on MFD_SPMI_PMIC
+	help
+	  Say Y here if you want support for the power key usually found
+	  on boards using a Qualcomm PM8941 compatible PMIC.
+
+	  If unsure, say Y.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pm8941-pwrkey.
+
 config INPUT_PM8XXX_VIBRATOR
 	tristate "Qualcomm PM8XXX vibrator support"
 	depends on MFD_PM8XXX

diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 4955ad3..e4ab02c 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile

@@ -44,6 +44,7 @@ obj-$(CONFIG_INPUT_PCAP)		+= pcap_keys.o
 obj-$(CONFIG_INPUT_PCF50633_PMU)	+= pcf50633-input.o
 obj-$(CONFIG_INPUT_PCF8574)		+= pcf8574_keypad.o
 obj-$(CONFIG_INPUT_PCSPKR)		+= pcspkr.o
+obj-$(CONFIG_INPUT_PM8941_PWRKEY)	+= pm8941-pwrkey.o
 obj-$(CONFIG_INPUT_PM8XXX_VIBRATOR)	+= pm8xxx-vibrator.o
 obj-$(CONFIG_INPUT_PMIC8XXX_PWRKEY)	+= pmic8xxx-pwrkey.o
 obj-$(CONFIG_INPUT_POWERMATE)		+= powermate.o

diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
new file mode 100644
index 0000000..58df8bc
--- /dev/null
+++ b/drivers/input/misc/pm8941-pwrkey.c

@@ -0,0 +1,196 @@
+/* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
+ * Copyright (c) 2014, Sony Mobile Communications Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/slab.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/log2.h>
+#include <linux/of.h>
+
+#define PON_RT_STS		0x10
+#define PON_PULL_CTL		0x70
+#define PON_DBC_CTL		0x71
+
+#define PON_DBC_DELAY_MASK	0x7
+#define PON_KPDPWR_N_SET	BIT(0)
+#define PON_KPDPWR_PULL_UP	BIT(1)
+
+struct pm8941_pwrkey {
+	int irq;
+	u32 baseaddr;
+	struct regmap *regmap;
+	struct input_dev *input;
+};
+
+static irqreturn_t pm8941_pwrkey_irq(int irq, void *_data)
+{
+	struct pm8941_pwrkey *pwrkey = _data;
+	unsigned int sts;
+	int rc;
+
+	rc = regmap_read(pwrkey->regmap, pwrkey->baseaddr + PON_RT_STS, &sts);
+	if (rc)
+		return IRQ_HANDLED;
+
+	input_report_key(pwrkey->input, KEY_POWER, !!(sts & PON_KPDPWR_N_SET));
+	input_sync(pwrkey->input);
+
+	return IRQ_HANDLED;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int pm8941_pwrkey_suspend(struct device *dev)
+{
+	struct pm8941_pwrkey *pwrkey = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		enable_irq_wake(pwrkey->irq);
+
+	return 0;
+}
+
+static int pm8941_pwrkey_resume(struct device *dev)
+{
+	struct pm8941_pwrkey *pwrkey = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		disable_irq_wake(pwrkey->irq);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(pm8941_pwr_key_pm_ops,
+		pm8941_pwrkey_suspend, pm8941_pwrkey_resume);
+
+static int pm8941_pwrkey_probe(struct platform_device *pdev)
+{
+	struct pm8941_pwrkey *pwrkey;
+	bool pull_up;
+	u32 req_delay;
+	int rc;
+
+	if (of_property_read_u32(pdev->dev.of_node, "debounce", &req_delay))
+		req_delay = 15625;
+
+	if (req_delay > 2000000 || req_delay == 0) {
+		dev_err(&pdev->dev, "invalid debounce time: %u\n", req_delay);
+		return -EINVAL;
+	}
+
+	pull_up = of_property_read_bool(pdev->dev.of_node, "bias-pull-up");
+
+	pwrkey = devm_kzalloc(&pdev->dev, sizeof(*pwrkey), GFP_KERNEL);
+	if (!pwrkey)
+		return -ENOMEM;
+
+	pwrkey->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!pwrkey->regmap) {
+		dev_err(&pdev->dev, "failed to locate regmap\n");
+		return -ENODEV;
+	}
+
+	pwrkey->irq = platform_get_irq(pdev, 0);
+	if (pwrkey->irq < 0) {
+		dev_err(&pdev->dev, "failed to get irq\n");
+		return pwrkey->irq;
+	}
+
+	rc = of_property_read_u32(pdev->dev.of_node, "reg", &pwrkey->baseaddr);
+	if (rc)
+		return rc;
+
+	pwrkey->input = devm_input_allocate_device(&pdev->dev);
+	if (!pwrkey->input) {
+		dev_dbg(&pdev->dev, "unable to allocate input device\n");
+		return -ENOMEM;
+	}
+
+	input_set_capability(pwrkey->input, EV_KEY, KEY_POWER);
+
+	pwrkey->input->name = "pm8941_pwrkey";
+	pwrkey->input->phys = "pm8941_pwrkey/input0";
+
+	req_delay = (req_delay << 6) / USEC_PER_SEC;
+	req_delay = ilog2(req_delay);
+
+	rc = regmap_update_bits(pwrkey->regmap, pwrkey->baseaddr + PON_DBC_CTL,
+			PON_DBC_DELAY_MASK, req_delay);
+	if (rc) {
+		dev_err(&pdev->dev, "failed to set debounce: %d\n", rc);
+		return rc;
+	}
+
+	rc = regmap_update_bits(pwrkey->regmap,
+			pwrkey->baseaddr + PON_PULL_CTL, PON_KPDPWR_PULL_UP,
+			pull_up ? PON_KPDPWR_PULL_UP : 0);
+	if (rc) {
+		dev_err(&pdev->dev, "failed to set pull: %d\n", rc);
+		return rc;
+	}
+
+	rc = devm_request_threaded_irq(&pdev->dev, pwrkey->irq,
+				NULL, pm8941_pwrkey_irq,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
+					IRQF_ONESHOT,
+				"pm8941_pwrkey", pwrkey);
+	if (rc) {
+		dev_err(&pdev->dev, "failed requesting IRQ: %d\n", rc);
+		return rc;
+	}
+
+	rc = input_register_device(pwrkey->input);
+	if (rc) {
+		dev_err(&pdev->dev, "failed to register input device: %d\n",
+			rc);
+		return rc;
+	}
+
+	platform_set_drvdata(pdev, pwrkey);
+	device_init_wakeup(&pdev->dev, 1);
+
+	return 0;
+}
+
+static int pm8941_pwrkey_remove(struct platform_device *pdev)
+{
+	device_init_wakeup(&pdev->dev, 0);
+
+	return 0;
+}
+
+static const struct of_device_id pm8941_pwr_key_id_table[] = {
+	{ .compatible = "qcom,pm8941-pwrkey" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pm8941_pwr_key_id_table);
+
+static struct platform_driver pm8941_pwrkey_driver = {
+	.probe		= pm8941_pwrkey_probe,
+	.remove		= pm8941_pwrkey_remove,
+	.driver		= {
+		.name	= "pm8941-pwrkey",
+		.owner	= THIS_MODULE,
+		.pm	= &pm8941_pwr_key_pm_ops,
+		.of_match_table = of_match_ptr(pm8941_pwr_key_id_table),
+	},
+};
+module_platform_driver(pm8941_pwrkey_driver);
+
+MODULE_DESCRIPTION("PM8941 Power Key driver");
+MODULE_LICENSE("GPL v2");

-- 
1.7.9.5

From: Courtney Cavin <redacted>

Signed-off-by: Courtney Cavin <redacted>
Signed-off-by: Bjorn Andersson <redacted>
---
 .../bindings/input/qcom,pm8941-pwrkey.txt          |   43 ++++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/qcom,pm8941-pwrkey.txt

diff --git a/Documentation/devicetree/bindings/input/qcom,pm8941-pwrkey.txt b/Documentation/devicetree/bindings/input/qcom,pm8941-pwrkey.txt
new file mode 100644
index 0000000..4d75e2c
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/qcom,pm8941-pwrkey.txt

@@ -0,0 +1,43 @@
+Qualcomm PM8941 PMIC Power Key
+
+PROPERTIES
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be one of:
+		    "qcom,pm8941-pwrkey"
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: base address of registers for block
+
+- interrupts:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: key change interrupt; The format of the specifier is
+		    defined by the binding document describing the node's
+		    interrupt parent.
+
+- debounce:
+	Usage: optional
+	Value type: <u32>
+	Definition: time in microseconds that key must be pressed or released
+		    for state change interrupt to trigger.
+
+- bias-pull-up:
+	Usage: optional
+	Value type: <empty>
+	Definition: presence of this property indicates that the KPDPWR_N pin
+		    should be configured for pull up.
+
+EXAMPLE
+
+	pwrkey@800 {
+		compatible = "qcom,pm8941-pwrkey";
+		reg = <0x800>;
+		interrupts = <0x0 0x8 0 0>;
+		debounce = <15625>;
+		bias-pull-up;
+	};

-- 
1.7.9.5

Hi Bjorn,

On Mon, Oct 06, 2014 at 06:12:00PM -0700, Bjorn Andersson wrote:

+
+	rc = devm_request_threaded_irq(&pdev->dev, pwrkey->irq,
+				NULL, pm8941_pwrkey_irq,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |

Do we need to hard-code IRQ flags like this? I'd rather we respect DT
data.

Thanks.

-- 
Dmitry

On Mon 06 Oct 18:30 PDT 2014, Dmitry Torokhov wrote:

Hi Bjorn,

Hi,

On Mon, Oct 06, 2014 at 06:12:00PM -0700, Bjorn Andersson wrote:

quoted

+
+	rc = devm_request_threaded_irq(&pdev->dev, pwrkey->irq,
+				NULL, pm8941_pwrkey_irq,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |

Do we need to hard-code IRQ flags like this? I'd rather we respect DT
data.

We could "move" it to DT but I don't think there's any other combination of
these flags would make sense. If you insist I can remove it though.

Regards,
Bjorn

On Mon, Oct 06, 2014 at 06:52:51PM -0700, Bjorn Andersson wrote:

On Mon 06 Oct 18:30 PDT 2014, Dmitry Torokhov wrote:

quoted

Hi Bjorn,

Hi,

quoted

On Mon, Oct 06, 2014 at 06:12:00PM -0700, Bjorn Andersson wrote:

quoted

+
+	rc = devm_request_threaded_irq(&pdev->dev, pwrkey->irq,
+				NULL, pm8941_pwrkey_irq,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |

Do we need to hard-code IRQ flags like this? I'd rather we respect DT
data.

We could "move" it to DT but I don't think there's any other combination of
these flags would make sense. If you insist I can remove it though.

No, that's fine if you are certain we want to react on both edges always.

Thanks.

-- 
Dmitry

Hi Bjorn,

On Mon, 2014-10-06 at 18:11 -0700, Bjorn Andersson wrote:

These patches add dt bindings and a device driver for the power key block in
the Qualcomm PM8941 pmic.

Courtney Cavin (2):
  input: Add Qualcomm PM8941 power key driver
  input: pm8941-pwrkey: Add DT binding documentation

 .../bindings/input/qcom,pm8941-pwrkey.txt          |   43 +++++
 drivers/input/misc/Kconfig                         |   12 ++
 drivers/input/misc/Makefile                        |    1 +
 drivers/input/misc/pm8941-pwrkey.c                 |  196 ++++++++++++++++++++
 4 files changed, 252 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/qcom,pm8941-pwrkey.txt
 create mode 100644 drivers/input/misc/pm8941-pwrkey.c

Any reason why we cannot reuse pm8xxx-pwrkey driver? It have been
converted to regmap already. 

Regards,
Ivan 

On Tuesday 07 October 2014 06:42 AM, Bjorn Andersson wrote:

From: Courtney Cavin <redacted>

Signed-off-by: Courtney Cavin <redacted>
Signed-off-by: Bjorn Andersson <redacted>
---
 drivers/input/misc/Kconfig         |   12 +++
 drivers/input/misc/Makefile        |    1 +
 drivers/input/misc/pm8941-pwrkey.c |  196 ++++++++++++++++++++++++++++++++++++

<snip>

+
+	platform_set_drvdata(pdev, pwrkey);
+	device_init_wakeup(&pdev->dev, 1);
+
+	return 0;
+}
+
+static int pm8941_pwrkey_remove(struct platform_device *pdev)
+{
+	device_init_wakeup(&pdev->dev, 0);

Shouldn't we unregister input device?

+
+	return 0;
+}
+
+static const struct of_device_id pm8941_pwr_key_id_table[] = {
+	{ .compatible = "qcom,pm8941-pwrkey" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pm8941_pwr_key_id_table);
+
+static struct platform_driver pm8941_pwrkey_driver = {
+	.probe		= pm8941_pwrkey_probe,
+	.remove		= pm8941_pwrkey_remove,
+	.driver		= {
+		.name	= "pm8941-pwrkey",
+		.owner	= THIS_MODULE,

This field can be removed because this driver which use the
module_platform_driver api as this is overridden in
platform_driver_register.

+		.pm	= &pm8941_pwr_key_pm_ops,
+		.of_match_table = of_match_ptr(pm8941_pwr_key_id_table),
+	},
+};
+module_platform_driver(pm8941_pwrkey_driver);
+
+MODULE_DESCRIPTION("PM8941 Power Key driver");
+MODULE_LICENSE("GPL v2");

May be you can add module author name.

Thanks,
--Kiran

On Tue 07 Oct 02:01 PDT 2014, Ivan T. Ivanov wrote:

Hi Bjorn,

On Mon, 2014-10-06 at 18:11 -0700, Bjorn Andersson wrote:

quoted

These patches add dt bindings and a device driver for the power key block in
the Qualcomm PM8941 pmic.

Courtney Cavin (2):
  input: Add Qualcomm PM8941 power key driver
  input: pm8941-pwrkey: Add DT binding documentation

 .../bindings/input/qcom,pm8941-pwrkey.txt          |   43 +++++
 drivers/input/misc/Kconfig                         |   12 ++
 drivers/input/misc/Makefile                        |    1 +
 drivers/input/misc/pm8941-pwrkey.c                 |  196 ++++++++++++++++++++
 4 files changed, 252 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/qcom,pm8941-pwrkey.txt
 create mode 100644 drivers/input/misc/pm8941-pwrkey.c

Any reason why we cannot reuse pm8xxx-pwrkey driver? It have been
converted to regmap already. 

The boilerplate code is the same, but configuration registers have different
layout and values written in them are different. The pm8xxx block have separate
interrupts for press and release events while pm8941 have one interrupt for
both, so the pm8941 must read out the irq status bits to figure out which event
it was.

Maybe if we introduce some vagueness related to interrupts in the dt binding
documentation for pm8xxx we could simply reuse that binding.

Regards,
Bjorn

On Oct 6, 2014, at 8:12 PM, Bjorn Andersson [off-list ref] wrote:

boolean, is probably better than <empty> here.

+	Definition: presence of this property indicates that the KPDPWR_N pin
+		    should be configured for pull up.
+
+EXAMPLE
+
+	pwrkey@800 {
+		compatible = "qcom,pm8941-pwrkey";
+		reg = <0x800>;
+		interrupts = <0x0 0x8 0 0>;
+		debounce = <15625>;
+		bias-pull-up;
+	};
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

On Tue 07 Oct 02:54 PDT 2014, Kiran Padwal wrote:

On Tuesday 07 October 2014 06:42 AM, Bjorn Andersson wrote:

quoted

From: Courtney Cavin <redacted>

Signed-off-by: Courtney Cavin <redacted>
Signed-off-by: Bjorn Andersson <redacted>
---
 drivers/input/misc/Kconfig         |   12 +++
 drivers/input/misc/Makefile        |    1 +
 drivers/input/misc/pm8941-pwrkey.c |  196 ++++++++++++++++++++++++++++++++++++

<snip>

quoted

+
+	platform_set_drvdata(pdev, pwrkey);
+	device_init_wakeup(&pdev->dev, 1);
+
+	return 0;
+}
+
+static int pm8941_pwrkey_remove(struct platform_device *pdev)
+{
+	device_init_wakeup(&pdev->dev, 0);

Shouldn't we unregister input device?

It's allocated with devm_input_allocate_device() so I assumed that it goes away
upon the device being removed. Looking at input_register_device() seems to
confirm that.

quoted

+
+	return 0;
+}
+
+static const struct of_device_id pm8941_pwr_key_id_table[] = {
+	{ .compatible = "qcom,pm8941-pwrkey" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pm8941_pwr_key_id_table);
+
+static struct platform_driver pm8941_pwrkey_driver = {
+	.probe		= pm8941_pwrkey_probe,
+	.remove		= pm8941_pwrkey_remove,
+	.driver		= {
+		.name	= "pm8941-pwrkey",
+		.owner	= THIS_MODULE,

This field can be removed because this driver which use the
module_platform_driver api as this is overridden in
platform_driver_register.

Thanks

quoted

+		.pm	= &pm8941_pwr_key_pm_ops,
+		.of_match_table = of_match_ptr(pm8941_pwr_key_id_table),
+	},
+};
+module_platform_driver(pm8941_pwrkey_driver);
+
+MODULE_DESCRIPTION("PM8941 Power Key driver");
+MODULE_LICENSE("GPL v2");

May be you can add module author name.

Courtney based this driver on the skeleton from pmic8xxx-pwrkey, so I don't
think it's right to say that there's any particular author. And you have the
git log...

Regards,
Bjorn

On Tue, Oct 07, 2014 at 04:30:46PM -0700, Bjorn Andersson wrote:

On Tue 07 Oct 02:54 PDT 2014, Kiran Padwal wrote:

quoted

On Tuesday 07 October 2014 06:42 AM, Bjorn Andersson wrote:

quoted

From: Courtney Cavin <redacted>

Signed-off-by: Courtney Cavin <redacted>
Signed-off-by: Bjorn Andersson <redacted>
---
 drivers/input/misc/Kconfig         |   12 +++
 drivers/input/misc/Makefile        |    1 +
 drivers/input/misc/pm8941-pwrkey.c |  196 ++++++++++++++++++++++++++++++++++++

<snip>

quoted

+
+	platform_set_drvdata(pdev, pwrkey);
+	device_init_wakeup(&pdev->dev, 1);
+
+	return 0;
+}
+
+static int pm8941_pwrkey_remove(struct platform_device *pdev)
+{
+	device_init_wakeup(&pdev->dev, 0);

Shouldn't we unregister input device?

It's allocated with devm_input_allocate_device() so I assumed that it goes away
upon the device being removed. Looking at input_register_device() seems to
confirm that.

Yes, devices allocated with devm_input_allocate_device() will be
unregistered and freed automatically.

Thanks.

-- 
Dmitry

On Tue, 2014-10-07 at 11:46 -0700, Bjorn Andersson wrote:

On Tue 07 Oct 02:01 PDT 2014, Ivan T. Ivanov wrote:

quoted

Hi Bjorn,

On Mon, 2014-10-06 at 18:11 -0700, Bjorn Andersson wrote:

quoted

These patches add dt bindings and a device driver for the power key block in
the Qualcomm PM8941 pmic.

Courtney Cavin (2):
  input: Add Qualcomm PM8941 power key driver
  input: pm8941-pwrkey: Add DT binding documentation

 .../bindings/input/qcom,pm8941-pwrkey.txt          |   43 +++++
 drivers/input/misc/Kconfig                         |   12 ++
 drivers/input/misc/Makefile                        |    1 +
 drivers/input/misc/pm8941-pwrkey.c                 |  196 ++++++++++++++++++++
 4 files changed, 252 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/qcom,pm8941-pwrkey.txt
 create mode 100644 drivers/input/misc/pm8941-pwrkey.c

Any reason why we cannot reuse pm8xxx-pwrkey driver? It have been
converted to regmap already. 

The boilerplate code is the same,

The boilerplate code is almost 100% :-)

 but configuration registers have different
layout and values written in them are different. 

We talk about 3 registers and 2 bit defines. struct regmap_field
should be able to help here.

The pm8xxx block have separate
interrupts for press and release events while pm8941 have one interrupt for
both, so the pm8941 must read out the irq status bits to figure out which event
it was.

Optional interrupt property? If both are defined hook old ISR, if its
only one hook pm8941 ISR?

Maybe if we introduce some vagueness related to interrupts in the dt binding
documentation for pm8xxx we could simply reuse that binding.

I would not say vagueness, we just can say that pm8941 did not have
second interrupt?

Regards,
Ivan

Regards,
Bjorn

On Wed 08 Oct 02:50 PDT 2014, Ivan T. Ivanov wrote:

On Tue, 2014-10-07 at 11:46 -0700, Bjorn Andersson wrote:

quoted

On Tue 07 Oct 02:01 PDT 2014, Ivan T. Ivanov wrote:

quoted

Hi Bjorn,

On Mon, 2014-10-06 at 18:11 -0700, Bjorn Andersson wrote:

[..]

quoted

quoted

quoted

 create mode 100644 drivers/input/misc/pm8941-pwrkey.c

Any reason why we cannot reuse pm8xxx-pwrkey driver? It have been
converted to regmap already. 

The boilerplate code is the same,

The boilerplate code is almost 100% :-)

quoted

 but configuration registers have different
layout and values written in them are different. 

We talk about 3 registers and 2 bit defines. struct regmap_field
should be able to help here.

You're totally right, we could rewrite the driver to use regmap_field and make
the rest of the differences conditional. In my eyes we end up with two drivers
in one file - but it can be done.

A difference however is that in pm8941 the ps hold behavious (reboot vs power
off) is controlled by this same block. So I have an additional patch that adds
a restart handler here that sets the pmic in the right state before we pull
pshold (but I haven't been able to test it properly).

In pm8xxx this is handled in the pmic misc block and does not belong in this
driver.

[..]

quoted

Maybe if we introduce some vagueness related to interrupts in the dt binding
documentation for pm8xxx we could simply reuse that binding.

I would not say vagueness, we just can say that pm8941 did not have
second interrupt?

I don't like having conditional documentation - it's not only that the second
interrupt is missing, the first one have different meaning. But you're right
that it's a minor thing and can be done.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Bjorn,

On Fri, 2014-10-24 at 08:23 -0700, Bjorn Andersson wrote:

On Wed 08 Oct 02:50 PDT 2014, Ivan T. Ivanov wrote:

quoted

On Tue, 2014-10-07 at 11:46 -0700, Bjorn Andersson wrote:

quoted

On Tue 07 Oct 02:01 PDT 2014, Ivan T. Ivanov wrote:

quoted

Hi Bjorn,

On Mon, 2014-10-06 at 18:11 -0700, Bjorn Andersson wrote:

[..]

quoted

quoted

quoted

quoted

 create mode 100644 drivers/input/misc/pm8941-pwrkey.c

Any reason why we cannot reuse pm8xxx-pwrkey driver? It have been
converted to regmap already.

The boilerplate code is the same,

The boilerplate code is almost 100% :-)

quoted

 but configuration registers have different
layout and values written in them are different.

We talk about 3 registers and 2 bit defines. struct regmap_field
should be able to help here.

You're totally right, we could rewrite the driver to use regmap_field and make
the rest of the differences conditional. In my eyes we end up with two drivers
in one file - but it can be done.

A difference however is that in pm8941 the ps hold behavious (reboot vs power
off) is controlled by this same block. So I have an additional patch that adds
a restart handler here that sets the pmic in the right state before we pull
pshold (but I haven't been able to test it properly).

In pm8xxx this is handled in the pmic misc block and does not belong in this
driver.

Ok, Your plan is to bring up support for QPNP power-on PMIC sub function into this
driver. In this case, I agree, it will be cleaner to have separate driver for this.

Regards,
Ivan

On Tue, Oct 7, 2014 at 4:42 PM, Dmitry Torokhov
[off-list ref] wrote:

On Tue, Oct 07, 2014 at 04:30:46PM -0700, Bjorn Andersson wrote:

quoted

On Tue 07 Oct 02:54 PDT 2014, Kiran Padwal wrote:

[..]

quoted

quoted

Shouldn't we unregister input device?

It's allocated with devm_input_allocate_device() so I assumed that it goes away
upon the device being removed. Looking at input_register_device() seems to
confirm that.

Yes, devices allocated with devm_input_allocate_device() will be
unregistered and freed automatically.

Thanks.

Hi Dmitry,

Will you pick this up? (couldn't find it in linux-next...)
Or is there anything else you need from me?

Regards,
Bjorn