| Message ID | 1414126425-13198-4-git-send-email-lokeshvutla@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 24, 2014 at 10:23:45AM +0530, Lokesh Vutla wrote: > On some Soc's RTC is powered by an external power regulator. > e.g. RTC on DRA7 SoC. Make the OMAP RTC driver support a > power regulator. > > Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> > --- > Documentation/devicetree/bindings/rtc/rtc-omap.txt | 3 +++ > drivers/rtc/rtc-omap.c | 24 ++++++++++++++++++++++ > 2 files changed, 27 insertions(+) > > diff --git a/Documentation/devicetree/bindings/rtc/rtc-omap.txt b/Documentation/devicetree/bindings/rtc/rtc-omap.txt > index 750efd4..e7ad12b 100644 > --- a/Documentation/devicetree/bindings/rtc/rtc-omap.txt > +++ b/Documentation/devicetree/bindings/rtc/rtc-omap.txt > @@ -15,6 +15,8 @@ Required properties: > Optional properties: > - ti,system-power-controller: whether the rtc is controlling the system power > through pmic_power_en > +Optional Properties: No need to repeat the "Optional Properties" header. > +- vrtc-supply: phandle to the regulator device tree node if needed > > Example: > > @@ -25,4 +27,5 @@ rtc@1c23000 { > 19>; > interrupt-parent = <&intc>; > ti,system-power-controller; > + vrtc-supply = <&ldo9_reg>; > }; > diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c > index d9bb5e7..61fe630 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) > @@ -516,6 +518,22 @@ static int omap_rtc_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, rtc); Also could you move the regulator allocation before this platform_set_drvdata when resending? It's not required, but that call sort of separates the allocations from the device initialisation currently. > + 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) { > + ret = regulator_enable(rtc->supply); > + if (ret) { > + dev_err(&pdev->dev, "regulator enable failed\n"); > + return ret; > + } > + } > + > /* Enable the clock/module so that we can access the registers */ > pm_runtime_enable(&pdev->dev); > pm_runtime_get_sync(&pdev->dev); > @@ -624,6 +642,9 @@ err: > pm_runtime_put_sync(&pdev->dev); > pm_runtime_disable(&pdev->dev); > > + if (rtc->supply) > + regulator_disable(rtc->supply); > + > return ret; > } > > @@ -649,6 +670,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; > } Looks good otherwise. Feel free to add Reviewed-by: Johan Hovold <johan@kernel.org> Thanks, Johan
On Friday 24 October 2014 01:23 PM, Johan Hovold wrote: > On Fri, Oct 24, 2014 at 10:23:45AM +0530, Lokesh Vutla wrote: >> On some Soc's RTC is powered by an external power regulator. >> e.g. RTC on DRA7 SoC. Make the OMAP RTC driver support a >> power regulator. >> >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> >> --- >> Documentation/devicetree/bindings/rtc/rtc-omap.txt | 3 +++ >> drivers/rtc/rtc-omap.c | 24 ++++++++++++++++++++++ >> 2 files changed, 27 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/rtc/rtc-omap.txt b/Documentation/devicetree/bindings/rtc/rtc-omap.txt >> index 750efd4..e7ad12b 100644 >> --- a/Documentation/devicetree/bindings/rtc/rtc-omap.txt >> +++ b/Documentation/devicetree/bindings/rtc/rtc-omap.txt >> @@ -15,6 +15,8 @@ Required properties: >> Optional properties: >> - ti,system-power-controller: whether the rtc is controlling the system power >> through pmic_power_en >> +Optional Properties: > > No need to repeat the "Optional Properties" header. Oops sorry. will remove it. > >> +- vrtc-supply: phandle to the regulator device tree node if needed >> >> Example: >> >> @@ -25,4 +27,5 @@ rtc@1c23000 { >> 19>; >> interrupt-parent = <&intc>; >> ti,system-power-controller; >> + vrtc-supply = <&ldo9_reg>; >> }; >> diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c >> index d9bb5e7..61fe630 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) >> @@ -516,6 +518,22 @@ static int omap_rtc_probe(struct platform_device *pdev) >> >> platform_set_drvdata(pdev, rtc); > > Also could you move the regulator allocation before this > platform_set_drvdata when resending? It's not required, but that call > sort of separates the allocations from the device initialisation > currently. Okay. Thanks and regards, Lokesh > >> + 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) { >> + ret = regulator_enable(rtc->supply); >> + if (ret) { >> + dev_err(&pdev->dev, "regulator enable failed\n"); >> + return ret; >> + } >> + } >> + >> /* Enable the clock/module so that we can access the registers */ >> pm_runtime_enable(&pdev->dev); >> pm_runtime_get_sync(&pdev->dev); >> @@ -624,6 +642,9 @@ err: >> pm_runtime_put_sync(&pdev->dev); >> pm_runtime_disable(&pdev->dev); >> >> + if (rtc->supply) >> + regulator_disable(rtc->supply); >> + >> return ret; >> } >> >> @@ -649,6 +670,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; >> } > > Looks good otherwise. Feel free to add > > Reviewed-by: Johan Hovold <johan@kernel.org> > > Thanks, > Johan >
On 10/23/2014 11:53 PM, Lokesh Vutla wrote: > On some Soc's RTC is powered by an external power regulator. SoC ? -> could you rephrase this to indicate "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. > e.g. RTC on DRA7 SoC. Make the OMAP RTC driver support a > power regulator. Question ofcourse is what voltage would you like that regulator to be at? As you are aware, certain LDOs and SMPS can drive varying voltage and just enable/disable would do just the default voltage of the SMPS/LDO, right? OR am i missing something here? > > Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> > --- > Documentation/devicetree/bindings/rtc/rtc-omap.txt | 3 +++ > drivers/rtc/rtc-omap.c | 24 ++++++++++++++++++++++ > 2 files changed, 27 insertions(+) > > diff --git a/Documentation/devicetree/bindings/rtc/rtc-omap.txt b/Documentation/devicetree/bindings/rtc/rtc-omap.txt > index 750efd4..e7ad12b 100644 > --- a/Documentation/devicetree/bindings/rtc/rtc-omap.txt > +++ b/Documentation/devicetree/bindings/rtc/rtc-omap.txt > @@ -15,6 +15,8 @@ Required properties: > Optional properties: > - ti,system-power-controller: whether the rtc is controlling the system power > through pmic_power_en > +Optional Properties: ^^ already commented on.. > +- vrtc-supply: phandle to the regulator device tree node if needed "phandle to supply regulator" ? since it is optional, "if needed" is redundant? > > Example: > > @@ -25,4 +27,5 @@ rtc@1c23000 { > 19>; > interrupt-parent = <&intc>; > ti,system-power-controller; > + vrtc-supply = <&ldo9_reg>; > }; > diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c > index d9bb5e7..61fe630 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) > @@ -516,6 +518,22 @@ static int omap_rtc_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, rtc); > > + 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) { > + ret = regulator_enable(rtc->supply); > + if (ret) { > + dev_err(&pdev->dev, "regulator enable failed\n"); would be nice to print the result as well - since it helps debug from log a little easier.
Hi, On Fri, Oct 24, 2014 at 09:53:12AM +0200, Johan Hovold wrote: > On Fri, Oct 24, 2014 at 10:23:45AM +0530, Lokesh Vutla wrote: > > On some Soc's RTC is powered by an external power regulator. > > e.g. RTC on DRA7 SoC. Make the OMAP RTC driver support a > > power regulator. > > > > Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> > > --- > > Documentation/devicetree/bindings/rtc/rtc-omap.txt | 3 +++ > > drivers/rtc/rtc-omap.c | 24 ++++++++++++++++++++++ > > 2 files changed, 27 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/rtc/rtc-omap.txt b/Documentation/devicetree/bindings/rtc/rtc-omap.txt > > index 750efd4..e7ad12b 100644 > > --- a/Documentation/devicetree/bindings/rtc/rtc-omap.txt > > +++ b/Documentation/devicetree/bindings/rtc/rtc-omap.txt > > @@ -15,6 +15,8 @@ Required properties: > > Optional properties: > > - ti,system-power-controller: whether the rtc is controlling the system power > > through pmic_power_en > > +Optional Properties: > > No need to repeat the "Optional Properties" header. > > > +- vrtc-supply: phandle to the regulator device tree node if needed > > > > Example: > > > > @@ -25,4 +27,5 @@ rtc@1c23000 { > > 19>; > > interrupt-parent = <&intc>; > > ti,system-power-controller; > > + vrtc-supply = <&ldo9_reg>; > > }; > > diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c > > index d9bb5e7..61fe630 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) > > @@ -516,6 +518,22 @@ static int omap_rtc_probe(struct platform_device *pdev) > > > > platform_set_drvdata(pdev, rtc); > > Also could you move the regulator allocation before this > platform_set_drvdata when resending? It's not required, but that call > sort of separates the allocations from the device initialisation > currently. > > > + 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) { > > + ret = regulator_enable(rtc->supply); > > + if (ret) { > > + dev_err(&pdev->dev, "regulator enable failed\n"); > > + return ret; > > + } > > + } > > + > > /* Enable the clock/module so that we can access the registers */ > > pm_runtime_enable(&pdev->dev); > > pm_runtime_get_sync(&pdev->dev); > > @@ -624,6 +642,9 @@ err: > > pm_runtime_put_sync(&pdev->dev); > > pm_runtime_disable(&pdev->dev); > > > > + if (rtc->supply) > > + regulator_disable(rtc->supply); > > + > > return ret; > > } > > > > @@ -649,6 +670,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; > > } > > Looks good otherwise. Feel free to add > > Reviewed-by: Johan Hovold <johan@kernel.org> Conditional on Johan's comments: Reviewed-by: Felipe Balbi <balbi@ti.com>
On Fri, Oct 24, 2014 at 08:26:43AM -0500, Nishanth Menon wrote: > On 10/23/2014 11:53 PM, Lokesh Vutla wrote: > > On some Soc's RTC is powered by an external power regulator. > SoC ? -> could you rephrase this to indicate "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. > > > e.g. RTC on DRA7 SoC. Make the OMAP RTC driver support a > > power regulator. > > Question ofcourse is what voltage would you like that regulator to be > at? As you are aware, certain LDOs and SMPS can drive varying voltage > and just enable/disable would do just the default voltage of the > SMPS/LDO, right? OR am i missing something here? just pass the correct voltage through DTS as we do for all other regulators ? It's only tricky when we have a range of acceptable voltages but for RTC, IIRC, it's always a set voltage (1.0V or 1.8V).
On 10/24/2014 10:43 AM, Felipe Balbi wrote: > On Fri, Oct 24, 2014 at 08:26:43AM -0500, Nishanth Menon wrote: >> On 10/23/2014 11:53 PM, Lokesh Vutla wrote: >>> On some Soc's RTC is powered by an external power regulator. >> SoC ? -> could you rephrase this to indicate "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. >> >>> e.g. RTC on DRA7 SoC. Make the OMAP RTC driver support a >>> power regulator. >> >> Question ofcourse is what voltage would you like that regulator to be >> at? As you are aware, certain LDOs and SMPS can drive varying voltage >> and just enable/disable would do just the default voltage of the >> SMPS/LDO, right? OR am i missing something here? > > just pass the correct voltage through DTS as we do for all other > regulators ? It's only tricky when we have a range of acceptable > voltages but for RTC, IIRC, it's always a set voltage (1.0V or 1.8V). > you mean min=max=1.8V board constraint - sure it will work, except I might expect to see that in the example? OR expect the driver to explicitly set it.
On Friday 24 October 2014 06:56 PM, Nishanth Menon wrote: > On 10/23/2014 11:53 PM, Lokesh Vutla wrote: >> On some Soc's RTC is powered by an external power regulator. > SoC ? -> could you rephrase this to indicate "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. Ok. will do it. > >> e.g. RTC on DRA7 SoC. Make the OMAP RTC driver support a >> power regulator. > > Question ofcourse is what voltage would you like that regulator to be > at? As you are aware, certain LDOs and SMPS can drive varying voltage > and just enable/disable would do just the default voltage of the > SMPS/LDO, right? OR am i missing something here? Yes I agree that enable/disable would just do the default voltage of SMPS/LDO. If default voltage needs to be changed, driver should explicitly call regulator_set_voltage. Currently this is missing. Ill update and repost it. Thanks for pointing it out. > >> >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> >> --- >> Documentation/devicetree/bindings/rtc/rtc-omap.txt | 3 +++ >> drivers/rtc/rtc-omap.c | 24 ++++++++++++++++++++++ >> 2 files changed, 27 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/rtc/rtc-omap.txt b/Documentation/devicetree/bindings/rtc/rtc-omap.txt >> index 750efd4..e7ad12b 100644 >> --- a/Documentation/devicetree/bindings/rtc/rtc-omap.txt >> +++ b/Documentation/devicetree/bindings/rtc/rtc-omap.txt >> @@ -15,6 +15,8 @@ Required properties: >> Optional properties: >> - ti,system-power-controller: whether the rtc is controlling the system power >> through pmic_power_en >> +Optional Properties: > ^^ already commented on.. > >> +- vrtc-supply: phandle to the regulator device tree node if needed > > "phandle to supply regulator" ? since it is optional, "if needed" is > redundant? Okay. will update it in next version. > >> >> Example: >> >> @@ -25,4 +27,5 @@ rtc@1c23000 { >> 19>; >> interrupt-parent = <&intc>; >> ti,system-power-controller; >> + vrtc-supply = <&ldo9_reg>; >> }; >> diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c >> index d9bb5e7..61fe630 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) >> @@ -516,6 +518,22 @@ static int omap_rtc_probe(struct platform_device *pdev) >> >> platform_set_drvdata(pdev, rtc); >> >> + 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) { >> + ret = regulator_enable(rtc->supply); >> + if (ret) { >> + dev_err(&pdev->dev, "regulator enable failed\n"); > > would be nice to print the result as well - since it helps debug from > log a little easier. Will update it in next version. Thanks and regards, Lokesh > > >
diff --git a/Documentation/devicetree/bindings/rtc/rtc-omap.txt b/Documentation/devicetree/bindings/rtc/rtc-omap.txt index 750efd4..e7ad12b 100644 --- a/Documentation/devicetree/bindings/rtc/rtc-omap.txt +++ b/Documentation/devicetree/bindings/rtc/rtc-omap.txt @@ -15,6 +15,8 @@ Required properties: Optional properties: - ti,system-power-controller: whether the rtc is controlling the system power through pmic_power_en +Optional Properties: +- vrtc-supply: phandle to the regulator device tree node if needed Example: @@ -25,4 +27,5 @@ rtc@1c23000 { 19>; interrupt-parent = <&intc>; ti,system-power-controller; + vrtc-supply = <&ldo9_reg>; }; diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c index d9bb5e7..61fe630 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) @@ -516,6 +518,22 @@ static int omap_rtc_probe(struct platform_device *pdev) platform_set_drvdata(pdev, rtc); + 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) { + ret = regulator_enable(rtc->supply); + if (ret) { + dev_err(&pdev->dev, "regulator enable failed\n"); + return ret; + } + } + /* Enable the clock/module so that we can access the registers */ pm_runtime_enable(&pdev->dev); pm_runtime_get_sync(&pdev->dev); @@ -624,6 +642,9 @@ err: pm_runtime_put_sync(&pdev->dev); pm_runtime_disable(&pdev->dev); + if (rtc->supply) + regulator_disable(rtc->supply); + return ret; } @@ -649,6 +670,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; }
On some Soc's RTC is powered by an external power regulator. e.g. RTC on DRA7 SoC. Make the OMAP RTC driver support a power regulator. Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> --- Documentation/devicetree/bindings/rtc/rtc-omap.txt | 3 +++ drivers/rtc/rtc-omap.c | 24 ++++++++++++++++++++++ 2 files changed, 27 insertions(+)