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 <[email protected]>
Signed-off-by: Courtney Cavin <[email protected]>
Signed-off-by: Bjorn Andersson <[email protected]>
---
.../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
From: Courtney Cavin <[email protected]>
Signed-off-by: Courtney Cavin <[email protected]>
Signed-off-by: Bjorn Andersson <[email protected]>
---
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
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:
> > +
> > + 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:
>
> > Hi Bjorn,
> >
>
> Hi,
>
> > 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.
> >
>
> 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 <[email protected]>
>
> Signed-off-by: Courtney Cavin <[email protected]>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> 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:
> > 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 <[email protected]> wrote:
> From: Courtney Cavin <[email protected]>
>
> Signed-off-by: Courtney Cavin <[email protected]>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> .../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>
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 [email protected]
> 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:
> > From: Courtney Cavin <[email protected]>
> >
> > Signed-off-by: Courtney Cavin <[email protected]>
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> > 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?
>
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.
> > +
> > + 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
> > + .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:
>
> > On Tuesday 07 October 2014 06:42 AM, Bjorn Andersson wrote:
> > > From: Courtney Cavin <[email protected]>
> > >
> > > Signed-off-by: Courtney Cavin <[email protected]>
> > > Signed-off-by: Bjorn Andersson <[email protected]>
> > > ---
> > > 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?
> >
>
> 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:
>
> >
> > 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.
> >
>
> 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:
> > 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:
[..]
> > > > 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.
>
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.
[..]
> >
> > 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
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:
>
> > On Tue, 2014-10-07 at 11:46 -0700, Bjorn Andersson wrote:
> > > 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:
> [..]
> > > > > 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.
> >
>
> 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
<[email protected]> wrote:
> On Tue, Oct 07, 2014 at 04:30:46PM -0700, Bjorn Andersson wrote:
>> On Tue 07 Oct 02:54 PDT 2014, Kiran Padwal wrote:
>>
[..]
>> > 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