From: Alexandre Belloni <alexandre.belloni@free-electrons.com>
To: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Alessandro Zummo <a.zummo@towertech.it>,
linux-rtc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 6/6] rtc: s3c: Handle clock enable failures
Date: Sat, 24 Jun 2017 22:55:32 +0200 [thread overview]
Message-ID: <20170624205532.eqykr37ynpvprklf@piout.net> (raw)
In-Reply-To: <20170616192807.10347-6-krzk@kernel.org>
Hi,
On 16/06/2017 at 21:28:07 +0200, Krzysztof Kozlowski wrote:
> clk_enable() can fail so handle such case.
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
> drivers/rtc/rtc-s3c.c | 72 ++++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 57 insertions(+), 15 deletions(-)
>
I've applied the whole series. However, quite often, on a platform,
clk_prepare/enable are ensured to never fail and I find that the added
complexity to handle failures is not worth it.
> diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
> index 0cb2f27a30b4..a8992c227f61 100644
> --- a/drivers/rtc/rtc-s3c.c
> +++ b/drivers/rtc/rtc-s3c.c
> @@ -68,18 +68,32 @@ struct s3c_rtc_data {
> void (*disable) (struct s3c_rtc *info);
> };
>
> -static void s3c_rtc_enable_clk(struct s3c_rtc *info)
> +static int s3c_rtc_enable_clk(struct s3c_rtc *info)
> {
> unsigned long irq_flags;
> + int ret = 0;
>
> spin_lock_irqsave(&info->alarm_clk_lock, irq_flags);
> +
> if (info->clk_disabled) {
> - clk_enable(info->rtc_clk);
> - if (info->data->needs_src_clk)
> - clk_enable(info->rtc_src_clk);
> + ret = clk_enable(info->rtc_clk);
> + if (ret)
> + goto out;
> +
> + if (info->data->needs_src_clk) {
> + ret = clk_enable(info->rtc_src_clk);
> + if (ret) {
> + clk_disable(info->rtc_clk);
> + goto out;
> + }
> + }
> info->clk_disabled = false;
> }
> +
> +out:
> spin_unlock_irqrestore(&info->alarm_clk_lock, irq_flags);
> +
> + return ret;
> }
>
> static void s3c_rtc_disable_clk(struct s3c_rtc *info)
> @@ -122,10 +136,13 @@ static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
> {
> struct s3c_rtc *info = dev_get_drvdata(dev);
> unsigned int tmp;
> + int ret;
>
> dev_dbg(info->dev, "%s: aie=%d\n", __func__, enabled);
>
> - s3c_rtc_enable_clk(info);
> + ret = s3c_rtc_enable_clk(info);
> + if (ret)
> + return ret;
>
> tmp = readb(info->base + S3C2410_RTCALM) & ~S3C2410_RTCALM_ALMEN;
>
> @@ -136,10 +153,13 @@ static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
>
> s3c_rtc_disable_clk(info);
>
> - if (enabled)
> - s3c_rtc_enable_clk(info);
> - else
> + if (enabled) {
> + ret = s3c_rtc_enable_clk(info);
> + if (ret)
> + return ret;
> + } else {
> s3c_rtc_disable_clk(info);
> + }
>
> return 0;
> }
> @@ -147,10 +167,14 @@ static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
> /* Set RTC frequency */
> static int s3c_rtc_setfreq(struct s3c_rtc *info, int freq)
> {
> + int ret;
> +
> if (!is_power_of_2(freq))
> return -EINVAL;
>
> - s3c_rtc_enable_clk(info);
> + ret = s3c_rtc_enable_clk(info);
> + if (ret)
> + return ret;
> spin_lock_irq(&info->pie_lock);
>
> if (info->data->set_freq)
> @@ -167,8 +191,11 @@ static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
> {
> struct s3c_rtc *info = dev_get_drvdata(dev);
> unsigned int have_retried = 0;
> + int ret;
>
> - s3c_rtc_enable_clk(info);
> + ret = s3c_rtc_enable_clk(info);
> + if (ret)
> + return ret;
>
> retry_get_time:
> rtc_tm->tm_min = readb(info->base + S3C2410_RTCMIN);
> @@ -212,6 +239,7 @@ static int s3c_rtc_settime(struct device *dev, struct rtc_time *tm)
> {
> struct s3c_rtc *info = dev_get_drvdata(dev);
> int year = tm->tm_year - 100;
> + int ret;
>
> dev_dbg(dev, "set time %04d.%02d.%02d %02d:%02d:%02d\n",
> 1900 + tm->tm_year, tm->tm_mon, tm->tm_mday,
> @@ -224,7 +252,9 @@ static int s3c_rtc_settime(struct device *dev, struct rtc_time *tm)
> return -EINVAL;
> }
>
> - s3c_rtc_enable_clk(info);
> + ret = s3c_rtc_enable_clk(info);
> + if (ret)
> + return ret;
>
> writeb(bin2bcd(tm->tm_sec), info->base + S3C2410_RTCSEC);
> writeb(bin2bcd(tm->tm_min), info->base + S3C2410_RTCMIN);
> @@ -243,8 +273,11 @@ static int s3c_rtc_getalarm(struct device *dev, struct rtc_wkalrm *alrm)
> struct s3c_rtc *info = dev_get_drvdata(dev);
> struct rtc_time *alm_tm = &alrm->time;
> unsigned int alm_en;
> + int ret;
>
> - s3c_rtc_enable_clk(info);
> + ret = s3c_rtc_enable_clk(info);
> + if (ret)
> + return ret;
>
> alm_tm->tm_sec = readb(info->base + S3C2410_ALMSEC);
> alm_tm->tm_min = readb(info->base + S3C2410_ALMMIN);
> @@ -293,6 +326,7 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
> struct s3c_rtc *info = dev_get_drvdata(dev);
> struct rtc_time *tm = &alrm->time;
> unsigned int alrm_en;
> + int ret;
> int year = tm->tm_year - 100;
>
> dev_dbg(dev, "s3c_rtc_setalarm: %d, %04d.%02d.%02d %02d:%02d:%02d\n",
> @@ -300,7 +334,9 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
> 1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
> tm->tm_hour, tm->tm_min, tm->tm_sec);
>
> - s3c_rtc_enable_clk(info);
> + ret = s3c_rtc_enable_clk(info);
> + if (ret)
> + return ret;
>
> alrm_en = readb(info->base + S3C2410_RTCALM) & S3C2410_RTCALM_ALMEN;
> writeb(0x00, info->base + S3C2410_RTCALM);
> @@ -349,8 +385,11 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
> static int s3c_rtc_proc(struct device *dev, struct seq_file *seq)
> {
> struct s3c_rtc *info = dev_get_drvdata(dev);
> + int ret;
>
> - s3c_rtc_enable_clk(info);
> + ret = s3c_rtc_enable_clk(info);
> + if (ret)
> + return ret;
>
> if (info->data->enable_tick)
> info->data->enable_tick(info, seq);
> @@ -589,8 +628,11 @@ static int s3c_rtc_probe(struct platform_device *pdev)
> static int s3c_rtc_suspend(struct device *dev)
> {
> struct s3c_rtc *info = dev_get_drvdata(dev);
> + int ret;
>
> - s3c_rtc_enable_clk(info);
> + ret = s3c_rtc_enable_clk(info);
> + if (ret)
> + return ret;
>
> /* save TICNT for anyone using periodic interrupts */
> if (info->data->save_tick_cnt)
> --
> 2.9.3
>
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
next prev parent reply other threads:[~2017-06-24 20:55 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-16 19:28 [PATCH 1/6] rtc: s3c: Jump to central exit point on getting src clock error Krzysztof Kozlowski
2017-06-16 19:28 ` [PATCH 2/6] rtc: s3c: Minor white-space cleanups Krzysztof Kozlowski
2017-06-16 19:28 ` [PATCH 3/6] rtc: s3c: Drop unneeded cast to void pointer Krzysztof Kozlowski
2017-06-16 19:28 ` [PATCH 4/6] rtc: s3c: Do not remove const from rodata memory Krzysztof Kozlowski
2017-06-16 19:28 ` [PATCH 5/6] rtc: s3c: Handle clock prepare failures in probe Krzysztof Kozlowski
2017-06-16 19:28 ` [PATCH 6/6] rtc: s3c: Handle clock enable failures Krzysztof Kozlowski
2017-06-24 20:55 ` Alexandre Belloni [this message]
2017-06-25 7:16 ` Krzysztof Kozlowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170624205532.eqykr37ynpvprklf@piout.net \
--to=alexandre.belloni@free-electrons.com \
--cc=a.zummo@towertech.it \
--cc=krzk@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rtc@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®