[PATCH v2 4/6] rtc: pm8xxx: add support for devicetree

[PATCH v2 4/6] rtc: pm8xxx: add support for devicetree

[Josh Cartwright]

Add support for describing the PM8921/PM8058 RTC in device tree.

Additionally:
   - drop support for describing the RTC using platform data,
     as there are no current in tree users who do so.
   - make allow_set_time a device-specific flag, instead of mucking
     with the rtc_ops

Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/rtc/rtc-pm8xxx.c       | 52 +++++++++++++++++++++++-------------------
 include/linux/mfd/pm8xxx/rtc.h | 25 --------------------
 2 files changed, 29 insertions(+), 48 deletions(-)
 delete mode 100644 include/linux/mfd/pm8xxx/rtc.h

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index 91ac2e3..6e3cd34 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -9,7 +9,7 @@
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  */
-
+#include <linux/of.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/rtc.h>
@@ -19,9 +19,6 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
-#include <linux/mfd/pm8xxx/rtc.h>
-
-
 /* RTC Register offsets from RTC CTRL REG */
 #define PM8XXX_ALARM_CTRL_OFFSET	0x01
 #define PM8XXX_RTC_WRITE_OFFSET		0x02
@@ -39,6 +36,7 @@
  * struct pm8xxx_rtc -  rtc driver internal structure
  * @rtc:		rtc device for this driver.
  * @regmap:		regmap used to access RTC registers
+ * @allow_set_time:	indicates whether writing to the RTC is allowed
  * @rtc_alarm_irq:	rtc alarm irq number.
  * @rtc_base:		address of rtc control register.
  * @rtc_read_base:	base address of read registers.
@@ -51,6 +49,7 @@
 struct pm8xxx_rtc {
 	struct rtc_device *rtc;
 	struct regmap *regmap;
+	bool allow_set_time;
 	int rtc_alarm_irq;
 	int rtc_base;
 	int rtc_read_base;
@@ -75,6 +74,9 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	u8 value[NUM_8_BIT_RTC_REGS], alarm_enabled = 0, ctrl_reg;
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 
+	if (!rtc_dd->allow_set_time)
+		return -EACCES;
+
 	rtc_tm_to_time(tm, &secs);
 
 	for (i = 0; i < NUM_8_BIT_RTC_REGS; i++) {
@@ -298,8 +300,9 @@ rtc_rw_fail:
 	return rc;
 }
 
-static struct rtc_class_ops pm8xxx_rtc_ops = {
+static const struct rtc_class_ops pm8xxx_rtc_ops = {
 	.read_time	= pm8xxx_rtc_read_time,
+	.set_time	= pm8xxx_rtc_set_time,
 	.set_alarm	= pm8xxx_rtc_set_alarm,
 	.read_alarm	= pm8xxx_rtc_read_alarm,
 	.alarm_irq_enable = pm8xxx_rtc_alarm_irq_enable,
@@ -353,18 +356,26 @@ rtc_alarm_handled:
 	return IRQ_HANDLED;
 }
 
+/*
+ * Hardcoded RTC bases until IORESOURCE_REG mapping is figured out
+ */
+static const struct of_device_id pm8xxx_id_table[] = {
+	{ .compatible = "qcom,pm8921-rtc", .data = (void *) 0x11D },
+	{ .compatible = "qcom,pm8058-rtc", .data = (void *) 0x1E8 },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, pm8xxx_id_table);
+
 static int pm8xxx_rtc_probe(struct platform_device *pdev)
 {
 	int rc;
 	unsigned int ctrl_reg;
-	bool rtc_write_enable = false;
 	struct pm8xxx_rtc *rtc_dd;
-	struct resource *rtc_resource;
-	const struct pm8xxx_rtc_platform_data *pdata =
-						dev_get_platdata(&pdev->dev);
+	const struct of_device_id *match;
 
-	if (pdata != NULL)
-		rtc_write_enable = pdata->rtc_write_enable;
+	match = of_match_node(pm8xxx_id_table, pdev->dev.of_node);
+	if (!match)
+		return -ENXIO;
 
 	rtc_dd = devm_kzalloc(&pdev->dev, sizeof(*rtc_dd), GFP_KERNEL);
 	if (rtc_dd == NULL)
@@ -385,14 +396,10 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 		return -ENXIO;
 	}
 
-	rtc_resource = platform_get_resource_byname(pdev, IORESOURCE_IO,
-						    "pmic_rtc_base");
-	if (!(rtc_resource && rtc_resource->start)) {
-		dev_err(&pdev->dev, "RTC IO resource absent!\n");
-		return -ENXIO;
-	}
+	rtc_dd->allow_set_time = of_property_read_bool(pdev->dev.of_node,
+						      "allow-set-time");
 
-	rtc_dd->rtc_base = rtc_resource->start;
+	rtc_dd->rtc_base = (long) match->data;
 
 	/* Setup RTC register addresses */
 	rtc_dd->rtc_write_base = rtc_dd->rtc_base + PM8XXX_RTC_WRITE_OFFSET;
@@ -419,8 +426,6 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 	}
 
 	rtc_dd->ctrl_reg = ctrl_reg;
-	if (rtc_write_enable)
-		pm8xxx_rtc_ops.set_time = pm8xxx_rtc_set_time;
 
 	platform_set_drvdata(pdev, rtc_dd);
 
@@ -479,9 +484,10 @@ static SIMPLE_DEV_PM_OPS(pm8xxx_rtc_pm_ops,
 static struct platform_driver pm8xxx_rtc_driver = {
 	.probe		= pm8xxx_rtc_probe,
 	.driver	= {
-		.name	= PM8XXX_RTC_DEV_NAME,
-		.owner	= THIS_MODULE,
-		.pm	= &pm8xxx_rtc_pm_ops,
+		.name		= "rtc-pm8xxx",
+		.owner		= THIS_MODULE,
+		.pm		= &pm8xxx_rtc_pm_ops,
+		.of_match_table	= pm8xxx_id_table,
 	},
 };
 
diff --git a/include/linux/mfd/pm8xxx/rtc.h b/include/linux/mfd/pm8xxx/rtc.h
deleted file mode 100644
index 14f1983..0000000
--- a/include/linux/mfd/pm8xxx/rtc.h
+++ /dev/null
@@ -1,25 +0,0 @@
-/* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
- *
- * 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.
- */
-
-#ifndef __RTC_PM8XXX_H__
-#define __RTC_PM8XXX_H__
-
-#define PM8XXX_RTC_DEV_NAME     "rtc-pm8xxx"
-/**
- * struct pm8xxx_rtc_pdata - RTC driver platform data
- * @rtc_write_enable: variable stating RTC write capability
- */
-struct pm8xxx_rtc_platform_data {
-	bool rtc_write_enable;
-};
-
-#endif /* __RTC_PM8XXX_H__ */
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2 4/6] rtc: pm8xxx: add support for devicetree

[Andrew Morton]

On Mon, 10 Mar 2014 13:44:47 -0500 Josh Cartwright <joshc@codeaurora.org> wrote:

> Add support for describing the PM8921/PM8058 RTC in device tree.
> 
> Additionally:
>    - drop support for describing the RTC using platform data,
>      as there are no current in tree users who do so.
>    - make allow_set_time a device-specific flag, instead of mucking
>      with the rtc_ops

Is this tested with and without CONFIG_OF?

Re: [PATCH v2 4/6] rtc: pm8xxx: add support for devicetree

[Josh Cartwright]

On Mon, Mar 10, 2014 at 01:47:48PM -0700, Andrew Morton wrote:
> On Mon, 10 Mar 2014 13:44:47 -0500 Josh Cartwright <joshc@codeaurora.org> wrote:
> 
> > Add support for describing the PM8921/PM8058 RTC in device tree.
> > 
> > Additionally:
> >    - drop support for describing the RTC using platform data,
> >      as there are no current in tree users who do so.
> >    - make allow_set_time a device-specific flag, instead of mucking
> >      with the rtc_ops
> 
> Is this tested with and without CONFIG_OF?

It's been runtime tested with CONFIG_OF and I ensured it at least built
when !CONFIG_OF, although the drivers useless in that configuration.

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

[PATCH v2 6/6] documentation: bindings: document PMIC8921/8058 RTC

[Josh Cartwright]

This RTC is found on the Qualcomm 8921 and 8058 PMICs.

Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
---
 .../devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt     | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt

diff --git a/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt
new file mode 100644
index 0000000..3eaa582
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt
@@ -0,0 +1,21 @@
+* Real-Time Clock for Qualcomm 8058/8921 PMICs
+
+Required properties:
+- compatible: should be one of the following.
+    * "qcom,pm8058-rtc"
+    * "qcom,pm8921-rtc"
+- reg: base address of the register region
+- interrupts: interrupt list for the RTC, must contain a single interrupt
+              specifier for the alarm interrupt
+
+Optional properties:
+- allow-set-time: indicates that the setting of RTC time is allowed by
+                  the host CPU
+
+Example:
+
+	rtc@11d {
+		compatible = "qcom,pm8921-rtc";
+		reg = <0x11d>;
+		interrupts = <0x27 0>;
+	};
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2 6/6] documentation: bindings: document PMIC8921/8058 RTC

[Rob Herring]

On Mon, Mar 10, 2014 at 1:44 PM, Josh Cartwright <joshc@codeaurora.org> wrote:
> This RTC is found on the Qualcomm 8921 and 8058 PMICs.
>
> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> ---
>  .../devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt     | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt
>
> diff --git a/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt
> new file mode 100644
> index 0000000..3eaa582
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt
> @@ -0,0 +1,21 @@
> +* Real-Time Clock for Qualcomm 8058/8921 PMICs
> +
> +Required properties:
> +- compatible: should be one of the following.
> +    * "qcom,pm8058-rtc"
> +    * "qcom,pm8921-rtc"
> +- reg: base address of the register region

Wait a second. This is all part of an MFD device. I think this whole
binding does not make sense on it's own. The DT should describe the
PMIC, not necessarily some sub-component of it. Perhaps there are sub
nodes of the PMIC node, but that would be all part of the PMIC
binding. Having sub drivers for an MFD is really a Linux problem that
doesn't belong in DT.

Rob

Re: [PATCH v2 6/6] documentation: bindings: document PMIC8921/8058 RTC

[Josh Cartwright]

On Mon, Mar 10, 2014 at 04:22:51PM -0500, Rob Herring wrote:
> On Mon, Mar 10, 2014 at 1:44 PM, Josh Cartwright <joshc-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> > This RTC is found on the Qualcomm 8921 and 8058 PMICs.
> >
> > Signed-off-by: Josh Cartwright <joshc-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> > ---
> >  .../devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt     | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt
> >
> > diff --git a/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt
> > new file mode 100644
> > index 0000000..3eaa582
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt
> > @@ -0,0 +1,21 @@
> > +* Real-Time Clock for Qualcomm 8058/8921 PMICs
> > +
> > +Required properties:
> > +- compatible: should be one of the following.
> > +    * "qcom,pm8058-rtc"
> > +    * "qcom,pm8921-rtc"
> > +- reg: base address of the register region
> 
> Wait a second. This is all part of an MFD device. I think this whole
> binding does not make sense on it's own. The DT should describe the
> PMIC, not necessarily some sub-component of it. Perhaps there are sub
> nodes of the PMIC node, but that would be all part of the PMIC
> binding. Having sub drivers for an MFD is really a Linux problem that
> doesn't belong in DT.

Okay, this makes sense, I can move this documentation to
Documentation/bindings/mfd/qcom,pm8xxx.txt where the PMIC binding
currently is.

Note that this does introduce a dependency on Lee Jones' mfd tree, where
DT support for the parent pm8921 driver is pending for 3.15.

Andrew, how would you prefer I proceed?  Would you like me to respin
this patch?  The whole series?  An incremental patch?

Thanks,
  Josh

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
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

Re: [PATCH v2 6/6] documentation: bindings: document PMIC8921/8058 RTC

[Andrew Morton]

On Mon, 10 Mar 2014 17:11:32 -0500 Josh Cartwright <joshc@codeaurora.org> wrote:

> On Mon, Mar 10, 2014 at 04:22:51PM -0500, Rob Herring wrote:
> > On Mon, Mar 10, 2014 at 1:44 PM, Josh Cartwright <joshc@codeaurora.org> wrote:
> > > This RTC is found on the Qualcomm 8921 and 8058 PMICs.
> > >
> > > Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> > > ---
> > >  .../devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt     | 21 +++++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt
> > > new file mode 100644
> > > index 0000000..3eaa582
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt
> > > @@ -0,0 +1,21 @@
> > > +* Real-Time Clock for Qualcomm 8058/8921 PMICs
> > > +
> > > +Required properties:
> > > +- compatible: should be one of the following.
> > > +    * "qcom,pm8058-rtc"
> > > +    * "qcom,pm8921-rtc"
> > > +- reg: base address of the register region
> > 
> > Wait a second. This is all part of an MFD device. I think this whole
> > binding does not make sense on it's own. The DT should describe the
> > PMIC, not necessarily some sub-component of it. Perhaps there are sub
> > nodes of the PMIC node, but that would be all part of the PMIC
> > binding. Having sub drivers for an MFD is really a Linux problem that
> > doesn't belong in DT.
> 
> Okay, this makes sense, I can move this documentation to
> Documentation/bindings/mfd/qcom,pm8xxx.txt where the PMIC binding
> currently is.
> 
> Note that this does introduce a dependency on Lee Jones' mfd tree, where
> DT support for the parent pm8921 driver is pending for 3.15.
> 
> Andrew, how would you prefer I proceed?  Would you like me to respin
> this patch?  The whole series?  An incremental patch?

A new version of [6/6] sounds appropriate.

[PATCH v3 6/6] mfd: devicetree: bindings: add pm8xxx RTC description

[Josh Cartwright]

The PM8xxx family of PMICs contain an RTC.  This RTC is described as a
subnode of the PM8xxx.  Document these bindings, and replace the pwrkey
node in the example with the RTC, which is now described in this
document.

While we're here, add a short description to the device tree bindings
describing what the the PM8xxx devices are and how they are expected to
be used.

Signed-off-by: Josh Cartwright <joshc-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
Andrew-

Here's a new 6/6.  Like mentioned earlier, this is based on the current MFD
document that's in Lee's tree pending for 3.15.  It may be best to get yours
and Rob's Ack and have Lee take it through his tree.

Thanks,
   Josh

 .../devicetree/bindings/mfd/qcom,pm8xxx.txt        | 45 +++++++++++++++++++---
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8xxx.txt b/Documentation/devicetree/bindings/mfd/qcom,pm8xxx.txt
index e3fe625..03518dc 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,pm8xxx.txt
+++ b/Documentation/devicetree/bindings/mfd/qcom,pm8xxx.txt
@@ -1,6 +1,9 @@
 Qualcomm PM8xxx PMIC multi-function devices
 
-PROPERTIES
+The PM8xxx family of Power Management ICs are used to provide regulated
+voltages and other various functionality to Qualcomm SoCs.
+
+= PROPERTIES
 
 - compatible:
 	Usage: required
@@ -45,7 +48,37 @@ PROPERTIES
 	Value type: <empty>
 	Definition: identifies this node as an interrupt controller
 
-EXAMPLE
+= SUBCOMPONENTS
+
+The PMIC contains multiple independent functions, each described in a subnode.
+The below bindings specify the set of valid subnodes.
+
+== Real-Time Clock
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be one of:
+		    "qcom,pm8058-rtc"
+		    "qcom,pm8921-rtc"
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: single entry specifying the base address of the RTC registers
+
+- interrupts:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: single entry specifying the RTC's alarm interrupt
+
+- allow-set-time:
+	Usage: optional
+	Value type: <empty>
+	Definition: indicates that the setting of RTC time is allowed by
+		    the host CPU
+
+= EXAMPLE
 
 	pmicintc: pmic@0 {
 		compatible = "qcom,pm8921";
@@ -55,9 +88,9 @@ EXAMPLE
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		pwrkey {
-			compatible = "qcom,pm8921-pwrkey";
-			interrupt-parent = <&pmicintc>;
-			interrupts = <50 1>, <51 1>;
+		rtc@11d {
+			compatible = "qcom,pm8921-rtc";
+			reg = <0x11d>;
+			interrupts = <0x27 0>;
 		};
 	};
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

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

Re: [PATCH v3 6/6] mfd: devicetree: bindings: add pm8xxx RTC description

[Lee Jones]

> The PM8xxx family of PMICs contain an RTC.  This RTC is described as a
> subnode of the PM8xxx.  Document these bindings, and replace the pwrkey
> node in the example with the RTC, which is now described in this
> document.
> 
> While we're here, add a short description to the device tree bindings
> describing what the the PM8xxx devices are and how they are expected to
> be used.
> 
> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> ---
> Andrew-
> 
> Here's a new 6/6.  Like mentioned earlier, this is based on the current MFD
> document that's in Lee's tree pending for 3.15.  It may be best to get yours
> and Rob's Ack and have Lee take it through his tree.

After discussing with Andrew, I think this is better to go through my
tree as you suggested, in order to mitigate any merge conflicts during
the merge window.

Patch applied to the MFD tree with Andrew's Ack, thanks.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog