Re: [PATCH V3 3/3] rtc: omap: Support regulator supply for RTC

[Lokesh Vutla <lokeshvutla@ti.com>]

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
>