| 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
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
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
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.
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
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 ?
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.
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); }
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(-)