diff mbox

[V4,3/3] rtc: omap: Support regulator supply for RTC

Message ID 1414490332-14856-4-git-send-email-lokeshvutla@ti.com
State New, archived
Headers show

Commit Message

Lokesh Vutla Oct. 28, 2014, 9:58 a.m. UTC
Certain SoCs such as DRA7, RTC is an independent voltage domain of it's own
and on platforms such as DRA7-evm, this may be supplied by individual
regulator on it's own.
So make the OMAP RTC driver support a power regulator.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
- Dropped the Reviewed-by tags as this patch is changed from previous version.
 Documentation/devicetree/bindings/rtc/rtc-omap.txt |  6 ++++
 drivers/rtc/rtc-omap.c                             | 41 +++++++++++++++++++++-
 2 files changed, 46 insertions(+), 1 deletion(-)

Comments

Nishanth Menon Oct. 28, 2014, 2:02 p.m. UTC | #1
On 10/28/2014 04:58 AM, Lokesh Vutla wrote:
> Certain SoCs such as DRA7, RTC is an independent voltage domain of it's own
> and on platforms such as DRA7-evm, this may be supplied by individual
> regulator on it's own.
> So make the OMAP RTC driver support a power regulator.
> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
> - Dropped the Reviewed-by tags as this patch is changed from previous version.
>  Documentation/devicetree/bindings/rtc/rtc-omap.txt |  6 ++++
>  drivers/rtc/rtc-omap.c                             | 41 +++++++++++++++++++++-
>  2 files changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/rtc-omap.txt b/Documentation/devicetree/bindings/rtc/rtc-omap.txt
> index 750efd4..c1d84ac 100644
> --- a/Documentation/devicetree/bindings/rtc/rtc-omap.txt
> +++ b/Documentation/devicetree/bindings/rtc/rtc-omap.txt
> @@ -15,6 +15,9 @@ Required properties:
>  Optional properties:
>  - ti,system-power-controller: whether the rtc is controlling the system power
>    through pmic_power_en
> +- vrtc-supply: phandle to the regulator device tree node.
> +- vrtc-minuV: Minimum required voltage in uV, If default voltage needs to be changed
> +- vrtc-maxuV: Maximum acceptable voltage in uV, If default voltage needs to be changed
>  
>  Example:
>  
> @@ -25,4 +28,7 @@ rtc@1c23000 {
>  		      19>;
>  	interrupt-parent = <&intc>;
>  	ti,system-power-controller;
> +	vrtc-supply = <&ldo9_reg>;
> +	vrtc-minuV = <1050000>;
> +	vrtc-maxuV = <1050000>;


why would we want to duplicate machine constraints that regulators
already have? if there is a constant voltage needed, then it should be
compatible property as it is not really a configuration option, right?

[...]
Felipe Balbi Oct. 28, 2014, 3:01 p.m. UTC | #2
On Tue, Oct 28, 2014 at 03:28:52PM +0530, Lokesh Vutla wrote:
> Certain SoCs such as DRA7, RTC is an independent voltage domain of it's own
> and on platforms such as DRA7-evm, this may be supplied by individual
> regulator on it's own.
> So make the OMAP RTC driver support a power regulator.
> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
> - Dropped the Reviewed-by tags as this patch is changed from previous version.
>  Documentation/devicetree/bindings/rtc/rtc-omap.txt |  6 ++++
>  drivers/rtc/rtc-omap.c                             | 41 +++++++++++++++++++++-
>  2 files changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/rtc-omap.txt b/Documentation/devicetree/bindings/rtc/rtc-omap.txt
> index 750efd4..c1d84ac 100644
> --- a/Documentation/devicetree/bindings/rtc/rtc-omap.txt
> +++ b/Documentation/devicetree/bindings/rtc/rtc-omap.txt
> @@ -15,6 +15,9 @@ Required properties:
>  Optional properties:
>  - ti,system-power-controller: whether the rtc is controlling the system power
>    through pmic_power_en
> +- vrtc-supply: phandle to the regulator device tree node.
> +- vrtc-minuV: Minimum required voltage in uV, If default voltage needs to be changed
> +- vrtc-maxuV: Maximum acceptable voltage in uV, If default voltage needs to be changed

huh ? minuV and maxuV is already part of the regulator binding itself.

> @@ -514,6 +516,37 @@ static int omap_rtc_probe(struct platform_device *pdev)
>  	if (IS_ERR(rtc->base))
>  		return PTR_ERR(rtc->base);
>  
> +	rtc->supply = devm_regulator_get_optional(&pdev->dev, "vrtc");

I'm not sure if this is optional either, it's just that many of our
current DTS don't really pass a regulator to RTC, right ?

> +	if (IS_ERR(rtc->supply)) {
> +		if (PTR_ERR(rtc->supply) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
> +		rtc->supply = NULL;
> +	}
> +
> +	if (rtc->supply) {
> +		of_property_read_u32(pdev->dev.of_node, "vrtc-minuV",
> +				     &vrtc_minuV);
> +		of_property_read_u32(pdev->dev.of_node, "vrtc-maxuV",
> +				     &vrtc_maxuV);
> +		if (vrtc_minuV && vrtc_maxuV) {
> +			ret = regulator_set_voltage(rtc->supply,
> +						    vrtc_minuV, vrtc_maxuV);
> +			if (ret) {
> +				dev_err(&pdev->dev, "failed to set volt %d\n",
> +					ret);
> +				return ret;
> +			}
> +		}

I'd really like to Mark's comments here but I was under the impression
that if the binding already gives min_microvolt == max_microvolt then
driver shouldn't really care about a set_voltage. Mark ?

> +
> +		ret = regulator_enable(rtc->supply);
> +		if (ret) {
> +			dev_err(&pdev->dev, "regulator enable failed %d\n",
> +				ret);
> +			return ret;
> +		}
> +	}
> +
>  	platform_set_drvdata(pdev, rtc);
>  
>  	/* Enable the clock/module so that we can access the registers */
> @@ -624,6 +657,9 @@ err:
>  	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  
> +	if (rtc->supply)
> +		regulator_disable(rtc->supply);
> +
>  	return ret;
>  }
>  
> @@ -649,6 +685,9 @@ static int __exit omap_rtc_remove(struct platform_device *pdev)
>  	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  
> +	if (rtc->supply)
> +		regulator_disable(rtc->supply);
> +
>  	return 0;
>  }
>  
> -- 
> 1.9.1
>
Mark Brown Oct. 28, 2014, 3:11 p.m. UTC | #3
On Tue, Oct 28, 2014 at 10:01:48AM -0500, Felipe Balbi wrote:
> On Tue, Oct 28, 2014 at 03:28:52PM +0530, Lokesh Vutla wrote:

> > +- vrtc-minuV: Minimum required voltage in uV, If default voltage needs to be changed
> > +- vrtc-maxuV: Maximum acceptable voltage in uV, If default voltage needs to be changed

> huh ? minuV and maxuV is already part of the regulator binding itself.

No, they aren't - there's bindings for setting constraints but not
bindings for setting values in consumers since consumers generally
understand why they are setting a voltage if they do so.  I'd expect
that we'd have a property for whatever system feature might cause us to
need to explicitly set the voltage if we need to vary the voltage at
runtime, or alternatively for systems to set a voltage through the
constraints on the supply rather than having properties on the consumer
- why are they here?

If for some reason we really need these properties they should be -max-uV
and -min-uV.

> > @@ -514,6 +516,37 @@ static int omap_rtc_probe(struct platform_device *pdev)
> >  	if (IS_ERR(rtc->base))
> >  		return PTR_ERR(rtc->base);
> >  
> > +	rtc->supply = devm_regulator_get_optional(&pdev->dev, "vrtc");

> I'm not sure if this is optional either, it's just that many of our
> current DTS don't really pass a regulator to RTC, right ?

If the RTC can run without power that would certainly be most
impressive.  I can't see any reason for this driver to use anything
other than a standard regulator_get().

> > +		if (vrtc_minuV && vrtc_maxuV) {
> > +			ret = regulator_set_voltage(rtc->supply,
> > +						    vrtc_minuV, vrtc_maxuV);
> > +			if (ret) {
> > +				dev_err(&pdev->dev, "failed to set volt %d\n",
> > +					ret);
> > +				return ret;
> > +			}
> > +		}

> I'd really like to Mark's comments here but I was under the impression
> that if the binding already gives min_microvolt == max_microvolt then
> driver shouldn't really care about a set_voltage. Mark ?

That's what happens for the standard properties on the supplying
regulator, these properties are separate to that.  Like I say I'm not
sure I understand why this is being done on a per-consumer basis.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/rtc/rtc-omap.txt b/Documentation/devicetree/bindings/rtc/rtc-omap.txt
index 750efd4..c1d84ac 100644
--- a/Documentation/devicetree/bindings/rtc/rtc-omap.txt
+++ b/Documentation/devicetree/bindings/rtc/rtc-omap.txt
@@ -15,6 +15,9 @@  Required properties:
 Optional properties:
 - ti,system-power-controller: whether the rtc is controlling the system power
   through pmic_power_en
+- vrtc-supply: phandle to the regulator device tree node.
+- vrtc-minuV: Minimum required voltage in uV, If default voltage needs to be changed
+- vrtc-maxuV: Maximum acceptable voltage in uV, If default voltage needs to be changed
 
 Example:
 
@@ -25,4 +28,7 @@  rtc@1c23000 {
 		      19>;
 	interrupt-parent = <&intc>;
 	ti,system-power-controller;
+	vrtc-supply = <&ldo9_reg>;
+	vrtc-minuV = <1050000>;
+	vrtc-maxuV = <1050000>;
 };
diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index d9bb5e7..7f1358a 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -25,6 +25,7 @@ 
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/io.h>
+#include <linux/regulator/consumer.h>
 
 /*
  * The OMAP RTC is a year/month/day/hours/minutes/seconds BCD clock
@@ -134,6 +135,7 @@  struct omap_rtc {
 	u8 interrupts_reg;
 	bool is_pmic_controller;
 	const struct omap_rtc_device_type *type;
+	struct regulator *supply;
 };
 
 static inline u8 rtc_read(struct omap_rtc *rtc, unsigned int reg)
@@ -484,7 +486,7 @@  static int omap_rtc_probe(struct platform_device *pdev)
 	u8 reg, mask, new_ctrl;
 	const struct platform_device_id *id_entry;
 	const struct of_device_id *of_id;
-	int ret;
+	int ret, vrtc_minuV = 0, vrtc_maxuV = 0;
 
 	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
 	if (!rtc)
@@ -514,6 +516,37 @@  static int omap_rtc_probe(struct platform_device *pdev)
 	if (IS_ERR(rtc->base))
 		return PTR_ERR(rtc->base);
 
+	rtc->supply = devm_regulator_get_optional(&pdev->dev, "vrtc");
+	if (IS_ERR(rtc->supply)) {
+		if (PTR_ERR(rtc->supply) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
+		rtc->supply = NULL;
+	}
+
+	if (rtc->supply) {
+		of_property_read_u32(pdev->dev.of_node, "vrtc-minuV",
+				     &vrtc_minuV);
+		of_property_read_u32(pdev->dev.of_node, "vrtc-maxuV",
+				     &vrtc_maxuV);
+		if (vrtc_minuV && vrtc_maxuV) {
+			ret = regulator_set_voltage(rtc->supply,
+						    vrtc_minuV, vrtc_maxuV);
+			if (ret) {
+				dev_err(&pdev->dev, "failed to set volt %d\n",
+					ret);
+				return ret;
+			}
+		}
+
+		ret = regulator_enable(rtc->supply);
+		if (ret) {
+			dev_err(&pdev->dev, "regulator enable failed %d\n",
+				ret);
+			return ret;
+		}
+	}
+
 	platform_set_drvdata(pdev, rtc);
 
 	/* Enable the clock/module so that we can access the registers */
@@ -624,6 +657,9 @@  err:
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
+	if (rtc->supply)
+		regulator_disable(rtc->supply);
+
 	return ret;
 }
 
@@ -649,6 +685,9 @@  static int __exit omap_rtc_remove(struct platform_device *pdev)
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
+	if (rtc->supply)
+		regulator_disable(rtc->supply);
+
 	return 0;
 }