Messages in this thread | | | Date | Thu, 4 Dec 2014 16:12:07 -0800 | From | Andrew Morton <> | Subject | Re: [RESEND PATCHv2] drivers/rtc/rtc-snvs: Add clock support |
| |
On Wed, 3 Dec 2014 11:28:55 +0530 Sanchayan Maity <maitysanchayan@gmail.com> wrote:
> This patch adds clock enable and disable support for > the SNVS peripheral, which is required for using the > RTC within the SNVS block. > > The clock is not strictly enforced, as this would > break the i.MX devices. The clocking for the i.MX > devices seems to be enabled elsewhere and enabling > RTC SNVS for Vybrid results in a crash. This patch > adds the clock support but also makes it optional > so Vybrid platform can use the clock if defined > while making sure not to break i.MX . > > ...
I assume this hasn't been tested with CONFIG_HAVE_CLK=n. It all *looks* OK, but the clk API does its tests for clk==NULL in some very deep places.
> --- a/drivers/rtc/rtc-snvs.c > +++ b/drivers/rtc/rtc-snvs.c . > ... > > @@ -288,10 +302,16 @@ static int snvs_rtc_probe(struct platform_device *pdev) > if (IS_ERR(data->rtc)) { > ret = PTR_ERR(data->rtc); > dev_err(&pdev->dev, "failed to register rtc: %d\n", ret); > - return ret; > + goto error_rtc_device_register; > } > > return 0; > + > +error_rtc_device_register: > + if (data->clk) > + clk_disable_unprepare(data->clk);
From my reading, there's no need to test data->clk here - clk_disable_unprepare() handles that.
It doesn't hurt to test it - a reasonable approach would be to look around at what other users of the clk API are doing, and do that.
> + return ret; > } > > #ifdef CONFIG_PM_SLEEP > @@ -302,16 +322,26 @@ static int snvs_rtc_suspend(struct device *dev) > if (device_may_wakeup(dev)) > enable_irq_wake(data->irq); > > + if (data->clk) > + clk_disable_unprepare(data->clk);
Ditto.
> return 0; > } > > static int snvs_rtc_resume(struct device *dev) > { > struct snvs_rtc_data *data = dev_get_drvdata(dev); > + int ret; > > if (device_may_wakeup(dev)) > disable_irq_wake(data->irq); > > + if (data->clk) { > + ret = clk_prepare_enable(data->clk); > + if (ret) > + return ret; > + }
It could be omitted here also.
> return 0; > } > #endif
| |