Patchwork [6/7] rtc: sa1100: enable clk support

login
register
mail settings
Submitter Haojian Zhuang
Date Feb. 21, 2012, 9:04 a.m.
Message ID <1329815096-6200-7-git-send-email-haojian.zhuang@marvell.com>
Download mbox | patch
Permalink /patch/142266/
State New
Headers show

Comments

Haojian Zhuang - Feb. 21, 2012, 9:04 a.m.
Add clk support in sa1100 rtc.

Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
---
 drivers/rtc/rtc-sa1100.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
Arnd Bergmann - Feb. 22, 2012, 12:29 p.m.
On Tuesday 21 February 2012, Haojian Zhuang wrote:
> @@ -306,6 +308,13 @@ static int sa1100_rtc_probe(struct platform_device *pdev)
>         if (!info)
>                 return -ENOMEM;
>  
> +       info->clk = clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(info->clk)) {
> +               dev_err(&pdev->dev, "failed to find rtc clock source\n");
> +               ret = PTR_ERR(info->clk);
> +               goto err_clk;
> +       }
> +       clk_prepare_enable(info->clk);
>         info->iobase = res->start;
>         info->iosize = resource_size(res);
>         info->irq_1hz = irq_1hz;
> @@ -379,6 +388,9 @@ err_dev:
>         iounmap(info->reg_base);
>  err_map:
>         platform_set_drvdata(pdev, NULL);
> +       clk_disable_unprepare(info->clk);
> +       clk_put(info->clk);
> +err_clk:
>         kfree(info);
>         return ret;

I wonder whether it would be easier to just make the clk handling
conditional here, like

	info->clk = clk_get(&pdev->dev, NULL);
	if (PTR_ERR(info->clk) == -ENOENT)
		info->clk = NULL;
	if (IS_ERR(info->clk)) {
		ret =  PTR_ERR(info->clk);
		goto err_clk;
	}

	if (info->clk)
		clk_prepare_enable(info->clk);
	...
	if (info->clk) {
		clk_disable_unprepare(info->clk);
		clk_put(info->clk);
	}
		
With this, you would no longer need to add dummy clocks in platforms
that really have no clock handling.

	Arnd
Haojian Zhuang - Feb. 22, 2012, 1:16 p.m.
On Wed, Feb 22, 2012 at 8:29 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 21 February 2012, Haojian Zhuang wrote:
>> @@ -306,6 +308,13 @@ static int sa1100_rtc_probe(struct platform_device *pdev)
>>         if (!info)
>>                 return -ENOMEM;
>>
>> +       info->clk = clk_get(&pdev->dev, NULL);
>> +       if (IS_ERR(info->clk)) {
>> +               dev_err(&pdev->dev, "failed to find rtc clock source\n");
>> +               ret = PTR_ERR(info->clk);
>> +               goto err_clk;
>> +       }
>> +       clk_prepare_enable(info->clk);
>>         info->iobase = res->start;
>>         info->iosize = resource_size(res);
>>         info->irq_1hz = irq_1hz;
>> @@ -379,6 +388,9 @@ err_dev:
>>         iounmap(info->reg_base);
>>  err_map:
>>         platform_set_drvdata(pdev, NULL);
>> +       clk_disable_unprepare(info->clk);
>> +       clk_put(info->clk);
>> +err_clk:
>>         kfree(info);
>>         return ret;
>
> I wonder whether it would be easier to just make the clk handling
> conditional here, like
>
>        info->clk = clk_get(&pdev->dev, NULL);
>        if (PTR_ERR(info->clk) == -ENOENT)
>                info->clk = NULL;
>        if (IS_ERR(info->clk)) {
>                ret =  PTR_ERR(info->clk);
>                goto err_clk;
>        }
>
>        if (info->clk)
>                clk_prepare_enable(info->clk);
>        ...
>        if (info->clk) {
>                clk_disable_unprepare(info->clk);
>                clk_put(info->clk);
>        }
>
> With this, you would no longer need to add dummy clocks in platforms
> that really have no clock handling.
>
>        Arnd

It sounds good. I'll update it.

Thanks
Haojian
Russell King - ARM Linux - Feb. 22, 2012, 1:20 p.m.
On Wed, Feb 22, 2012 at 12:29:17PM +0000, Arnd Bergmann wrote:
> On Tuesday 21 February 2012, Haojian Zhuang wrote:
> > @@ -306,6 +308,13 @@ static int sa1100_rtc_probe(struct platform_device *pdev)
> >         if (!info)
> >                 return -ENOMEM;
> >  
> > +       info->clk = clk_get(&pdev->dev, NULL);
> > +       if (IS_ERR(info->clk)) {
> > +               dev_err(&pdev->dev, "failed to find rtc clock source\n");
> > +               ret = PTR_ERR(info->clk);
> > +               goto err_clk;
> > +       }
> > +       clk_prepare_enable(info->clk);
> >         info->iobase = res->start;
> >         info->iosize = resource_size(res);
> >         info->irq_1hz = irq_1hz;
> > @@ -379,6 +388,9 @@ err_dev:
> >         iounmap(info->reg_base);
> >  err_map:
> >         platform_set_drvdata(pdev, NULL);
> > +       clk_disable_unprepare(info->clk);
> > +       clk_put(info->clk);
> > +err_clk:
> >         kfree(info);
> >         return ret;
> 
> I wonder whether it would be easier to just make the clk handling
> conditional here, like

Eww.  No, just keep this hidden beneath the clk API.  Return a NULL
clock for this device, and ensure that all the standard clk API
functions know internally that that means 'no action required'.

There's no need to pollute drivers with this platform specific knowledge.
Arnd Bergmann - Feb. 22, 2012, 1:47 p.m.
On Wednesday 22 February 2012, Russell King - ARM Linux wrote:
> Eww.  No, just keep this hidden beneath the clk API.  Return a NULL
> clock for this device, and ensure that all the standard clk API
> functions know internally that that means 'no action required'.
> 
> There's no need to pollute drivers with this platform specific knowledge.

Good point, that does sound much cleaner than my suggestion.

Thanks,

	Arnd
Russell King - ARM Linux - Feb. 23, 2012, 10:34 a.m.
On Tue, Feb 21, 2012 at 05:04:55PM +0800, Haojian Zhuang wrote:
> @@ -306,6 +308,13 @@ static int sa1100_rtc_probe(struct platform_device *pdev)
>  	if (!info)
>  		return -ENOMEM;
>  
> +	info->clk = clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(info->clk)) {
> +		dev_err(&pdev->dev, "failed to find rtc clock source\n");
> +		ret = PTR_ERR(info->clk);
> +		goto err_clk;
> +	}
> +	clk_prepare_enable(info->clk);

What about checking for errors from clk_prepare_enable() ?

I assume that this is the clock required to access the peripheral, rather
than the timekeeping clock?  If so, does it need to be kept enabled all
the time the driver is probed, or can the clock be prepared & enabled and
disabled & unprepared when the device is opened/closed ?
Haojian Zhuang - Feb. 23, 2012, 10:40 a.m.
On Thu, Feb 23, 2012 at 6:34 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Feb 21, 2012 at 05:04:55PM +0800, Haojian Zhuang wrote:
>> @@ -306,6 +308,13 @@ static int sa1100_rtc_probe(struct platform_device *pdev)
>>       if (!info)
>>               return -ENOMEM;
>>
>> +     info->clk = clk_get(&pdev->dev, NULL);
>> +     if (IS_ERR(info->clk)) {
>> +             dev_err(&pdev->dev, "failed to find rtc clock source\n");
>> +             ret = PTR_ERR(info->clk);
>> +             goto err_clk;
>> +     }
>> +     clk_prepare_enable(info->clk);
>
> What about checking for errors from clk_prepare_enable() ?
>
> I assume that this is the clock required to access the peripheral, rather
> than the timekeeping clock?  If so, does it need to be kept enabled all
> the time the driver is probed, or can the clock be prepared & enabled and
> disabled & unprepared when the device is opened/closed ?
>
Since some platform code is using RTC register in their suspend/resume
routine, I have to always enable the clock of rtc module.
Russell King - ARM Linux - Feb. 23, 2012, 10:54 a.m.
On Thu, Feb 23, 2012 at 06:40:40PM +0800, Haojian Zhuang wrote:
> On Thu, Feb 23, 2012 at 6:34 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Tue, Feb 21, 2012 at 05:04:55PM +0800, Haojian Zhuang wrote:
> >> @@ -306,6 +308,13 @@ static int sa1100_rtc_probe(struct platform_device *pdev)
> >>       if (!info)
> >>               return -ENOMEM;
> >>
> >> +     info->clk = clk_get(&pdev->dev, NULL);
> >> +     if (IS_ERR(info->clk)) {
> >> +             dev_err(&pdev->dev, "failed to find rtc clock source\n");
> >> +             ret = PTR_ERR(info->clk);
> >> +             goto err_clk;
> >> +     }
> >> +     clk_prepare_enable(info->clk);
> >
> > What about checking for errors from clk_prepare_enable() ?
> >
> > I assume that this is the clock required to access the peripheral, rather
> > than the timekeeping clock?  If so, does it need to be kept enabled all
> > the time the driver is probed, or can the clock be prepared & enabled and
> > disabled & unprepared when the device is opened/closed ?
> >
> Since some platform code is using RTC register in their suspend/resume
> routine, I have to always enable the clock of rtc module.

So what happens if they don't have the RTC module loaded in their kernel?
The proper solution to this is that they need to take care of that clock
themselves and stop relying on a module being loaded or built-in.

Patch

diff --git a/drivers/rtc/rtc-sa1100.c b/drivers/rtc/rtc-sa1100.c
index 90425ce..f031f4d 100644
--- a/drivers/rtc/rtc-sa1100.c
+++ b/drivers/rtc/rtc-sa1100.c
@@ -23,6 +23,7 @@ 
 
 #include <linux/platform_device.h>
 #include <linux/module.h>
+#include <linux/clk.h>
 #include <linux/rtc.h>
 #include <linux/init.h>
 #include <linux/io.h>
@@ -63,6 +64,7 @@  struct sa1100_rtc {
 	int			irq_1hz;
 	int			irq_alarm;
 	struct rtc_device	*rtc;
+	struct clk		*clk;
 
 	void __iomem		*reg_base;
 	void __iomem		*reg_rcnr;
@@ -306,6 +308,13 @@  static int sa1100_rtc_probe(struct platform_device *pdev)
 	if (!info)
 		return -ENOMEM;
 
+	info->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(info->clk)) {
+		dev_err(&pdev->dev, "failed to find rtc clock source\n");
+		ret = PTR_ERR(info->clk);
+		goto err_clk;
+	}
+	clk_prepare_enable(info->clk);
 	info->iobase = res->start;
 	info->iosize = resource_size(res);
 	info->irq_1hz = irq_1hz;
@@ -379,6 +388,9 @@  err_dev:
 	iounmap(info->reg_base);
 err_map:
 	platform_set_drvdata(pdev, NULL);
+	clk_disable_unprepare(info->clk);
+	clk_put(info->clk);
+err_clk:
 	kfree(info);
 	return ret;
 }
@@ -391,6 +403,8 @@  static int sa1100_rtc_remove(struct platform_device *pdev)
 		rtc_device_unregister(info->rtc);
 		platform_set_drvdata(pdev, NULL);
 		iounmap(info->reg_base);
+		clk_disable_unprepare(info->clk);
+		clk_put(info->clk);
 		kfree(info);
 	}