| Message ID | 1414490332-14856-4-git-send-email-lokeshvutla@ti.com |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
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? [...]
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 >
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 --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; }
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(-)