[PATCH 1/6] rtc: s3c: Jump to central exit point on getting src clock error

[PATCH 1/6] rtc: s3c: Jump to central exit point on getting src clock error

[Krzysztof Kozlowski]

In other error paths in probe, centralized exit point was used so make
this consistent.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/rtc/rtc-s3c.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index d44fb34df8fe..c5aa7a35d07f 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -510,8 +510,7 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 			else
 				dev_dbg(&pdev->dev,
 					"probe deferred due to missing rtc src clk\n");
-			clk_disable_unprepare(info->rtc_clk);
-			return ret;
+			goto err_src_clk;
 		}
 		clk_prepare_enable(info->rtc_src_clk);
 	}
@@ -575,6 +574,7 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 
 	if (info->data->needs_src_clk)
 		clk_disable_unprepare(info->rtc_src_clk);
+err_src_clk:
 	clk_disable_unprepare(info->rtc_clk);
 
 	return ret;
-- 
2.9.3

[PATCH 2/6] rtc: s3c: Minor white-space cleanups

[Krzysztof Kozlowski]

Minor cleanups to make the code easier to read. No functional changes.
1. Remove one space before labels as this is nowadays mostly preferred.
2. Fix indentation of arguments in function calls.
3. Split structure member declaration.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/rtc/rtc-s3c.c | 47 +++++++++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index c5aa7a35d07f..2b503dab7957 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -49,7 +49,8 @@ struct s3c_rtc {
 	spinlock_t pie_lock;
 	spinlock_t alarm_clk_lock;
 
-	int ticnt_save, ticnt_en_save;
+	int ticnt_save;
+	int ticnt_en_save;
 	bool wake_en;
 };
 
@@ -169,7 +170,7 @@ static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 
 	s3c_rtc_enable_clk(info);
 
- retry_get_time:
+retry_get_time:
 	rtc_tm->tm_min  = readb(info->base + S3C2410_RTCMIN);
 	rtc_tm->tm_hour = readb(info->base + S3C2410_RTCHOUR);
 	rtc_tm->tm_mday = readb(info->base + S3C2410_RTCDATE);
@@ -199,8 +200,8 @@ static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 	rtc_tm->tm_year += 100;
 
 	dev_dbg(dev, "read time %04d.%02d.%02d %02d:%02d:%02d\n",
-		 1900 + rtc_tm->tm_year, rtc_tm->tm_mon, rtc_tm->tm_mday,
-		 rtc_tm->tm_hour, rtc_tm->tm_min, rtc_tm->tm_sec);
+		1900 + rtc_tm->tm_year, rtc_tm->tm_mon, rtc_tm->tm_mday,
+		rtc_tm->tm_hour, rtc_tm->tm_min, rtc_tm->tm_sec);
 
 	rtc_tm->tm_mon -= 1;
 
@@ -213,8 +214,8 @@ static int s3c_rtc_settime(struct device *dev, struct rtc_time *tm)
 	int year = tm->tm_year - 100;
 
 	dev_dbg(dev, "set time %04d.%02d.%02d %02d:%02d:%02d\n",
-		 1900 + tm->tm_year, tm->tm_mon, tm->tm_mday,
-		 tm->tm_hour, tm->tm_min, tm->tm_sec);
+		1900 + tm->tm_year, tm->tm_mon, tm->tm_mday,
+		tm->tm_hour, tm->tm_min, tm->tm_sec);
 
 	/* we get around y2k by simply not supporting it */
 
@@ -259,9 +260,9 @@ static int s3c_rtc_getalarm(struct device *dev, struct rtc_wkalrm *alrm)
 	alrm->enabled = (alm_en & S3C2410_RTCALM_ALMEN) ? 1 : 0;
 
 	dev_dbg(dev, "read alarm %d, %04d.%02d.%02d %02d:%02d:%02d\n",
-		 alm_en,
-		 1900 + alm_tm->tm_year, alm_tm->tm_mon, alm_tm->tm_mday,
-		 alm_tm->tm_hour, alm_tm->tm_min, alm_tm->tm_sec);
+		alm_en,
+		1900 + alm_tm->tm_year, alm_tm->tm_mon, alm_tm->tm_mday,
+		alm_tm->tm_hour, alm_tm->tm_min, alm_tm->tm_sec);
 
 	/* decode the alarm enable field */
 	if (alm_en & S3C2410_RTCALM_SECEN)
@@ -295,9 +296,9 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 	int year = tm->tm_year - 100;
 
 	dev_dbg(dev, "s3c_rtc_setalarm: %d, %04d.%02d.%02d %02d:%02d:%02d\n",
-		 alrm->enabled,
-		 1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
-		 tm->tm_hour, tm->tm_min, tm->tm_sec);
+		alrm->enabled,
+		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);
 
@@ -378,8 +379,7 @@ static void s3c24xx_rtc_enable(struct s3c_rtc *info)
 		dev_info(info->dev, "rtc disabled, re-enabling\n");
 
 		tmp = readw(info->base + S3C2410_RTCCON);
-		writew(tmp | S3C2410_RTCCON_RTCEN,
-			info->base + S3C2410_RTCCON);
+		writew(tmp | S3C2410_RTCCON_RTCEN, info->base + S3C2410_RTCCON);
 	}
 
 	if (con & S3C2410_RTCCON_CNTSEL) {
@@ -387,7 +387,7 @@ static void s3c24xx_rtc_enable(struct s3c_rtc *info)
 
 		tmp = readw(info->base + S3C2410_RTCCON);
 		writew(tmp & ~S3C2410_RTCCON_CNTSEL,
-			info->base + S3C2410_RTCCON);
+		       info->base + S3C2410_RTCCON);
 	}
 
 	if (con & S3C2410_RTCCON_CLKRST) {
@@ -395,7 +395,7 @@ static void s3c24xx_rtc_enable(struct s3c_rtc *info)
 
 		tmp = readw(info->base + S3C2410_RTCCON);
 		writew(tmp & ~S3C2410_RTCCON_CLKRST,
-			info->base + S3C2410_RTCCON);
+		       info->base + S3C2410_RTCCON);
 	}
 }
 
@@ -481,7 +481,7 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 	}
 
 	dev_dbg(&pdev->dev, "s3c2410_rtc: tick irq %d, alarm irq %d\n",
-		 info->irq_tick, info->irq_alarm);
+		info->irq_tick, info->irq_alarm);
 
 	/* get the memory region */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -520,7 +520,7 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 		info->data->enable(info);
 
 	dev_dbg(&pdev->dev, "s3c2410_rtc: RTCCON=%02x\n",
-		 readw(info->base + S3C2410_RTCCON));
+		readw(info->base + S3C2410_RTCCON));
 
 	device_init_wakeup(&pdev->dev, 1);
 
@@ -540,7 +540,7 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 
 	/* register RTC and exit */
 	info->rtc = devm_rtc_device_register(&pdev->dev, "s3c", &s3c_rtcops,
-				  THIS_MODULE);
+					     THIS_MODULE);
 	if (IS_ERR(info->rtc)) {
 		dev_err(&pdev->dev, "cannot attach rtc\n");
 		ret = PTR_ERR(info->rtc);
@@ -548,14 +548,14 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 	}
 
 	ret = devm_request_irq(&pdev->dev, info->irq_alarm, s3c_rtc_alarmirq,
-			  0,  "s3c2410-rtc alarm", info);
+			       0, "s3c2410-rtc alarm", info);
 	if (ret) {
 		dev_err(&pdev->dev, "IRQ%d error %d\n", info->irq_alarm, ret);
 		goto err_nortc;
 	}
 
 	ret = devm_request_irq(&pdev->dev, info->irq_tick, s3c_rtc_tickirq,
-			  0,  "s3c2410-rtc tick", info);
+			       0, "s3c2410-rtc tick", info);
 	if (ret) {
 		dev_err(&pdev->dev, "IRQ%d error %d\n", info->irq_tick, ret);
 		goto err_nortc;
@@ -568,7 +568,7 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 
 	return 0;
 
- err_nortc:
+err_nortc:
 	if (info->data->disable)
 		info->data->disable(info);
 
@@ -747,8 +747,7 @@ static void s3c6410_rtc_restore_tick_cnt(struct s3c_rtc *info)
 	writel(info->ticnt_save, info->base + S3C2410_TICNT);
 	if (info->ticnt_en_save) {
 		con = readw(info->base + S3C2410_RTCCON);
-		writew(con | info->ticnt_en_save,
-				info->base + S3C2410_RTCCON);
+		writew(con | info->ticnt_en_save, info->base + S3C2410_RTCCON);
 	}
 }
 
-- 
2.9.3

[PATCH 3/6] rtc: s3c: Drop unneeded cast to void pointer

[Krzysztof Kozlowski]

There is no need for casting to void pointer for of_device_id data.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/rtc/rtc-s3c.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index 2b503dab7957..bfc8660ff1e7 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -801,19 +801,19 @@ static struct s3c_rtc_data const s3c6410_rtc_data = {
 static const struct of_device_id s3c_rtc_dt_match[] = {
 	{
 		.compatible = "samsung,s3c2410-rtc",
-		.data = (void *)&s3c2410_rtc_data,
+		.data = &s3c2410_rtc_data,
 	}, {
 		.compatible = "samsung,s3c2416-rtc",
-		.data = (void *)&s3c2416_rtc_data,
+		.data = &s3c2416_rtc_data,
 	}, {
 		.compatible = "samsung,s3c2443-rtc",
-		.data = (void *)&s3c2443_rtc_data,
+		.data = &s3c2443_rtc_data,
 	}, {
 		.compatible = "samsung,s3c6410-rtc",
-		.data = (void *)&s3c6410_rtc_data,
+		.data = &s3c6410_rtc_data,
 	}, {
 		.compatible = "samsung,exynos3250-rtc",
-		.data = (void *)&s3c6410_rtc_data,
+		.data = &s3c6410_rtc_data,
 	},
 	{ /* sentinel */ },
 };
-- 
2.9.3

[PATCH 4/6] rtc: s3c: Do not remove const from rodata memory

[Krzysztof Kozlowski]

All instances of struct s3c_rtc_data are in fact static const thus
put in rodata so we should not drop the const while getting the pointer
to them.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/rtc/rtc-s3c.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index bfc8660ff1e7..c666b95fb8d7 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -41,7 +41,7 @@ struct s3c_rtc {
 	struct clk *rtc_src_clk;
 	bool clk_disabled;
 
-	struct s3c_rtc_data *data;
+	const struct s3c_rtc_data *data;
 
 	int irq_alarm;
 	int irq_tick;
@@ -437,12 +437,12 @@ static int s3c_rtc_remove(struct platform_device *pdev)
 
 static const struct of_device_id s3c_rtc_dt_match[];
 
-static struct s3c_rtc_data *s3c_rtc_get_data(struct platform_device *pdev)
+static const struct s3c_rtc_data *s3c_rtc_get_data(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
 
 	match = of_match_node(s3c_rtc_dt_match, pdev->dev.of_node);
-	return (struct s3c_rtc_data *)match->data;
+	return match->data;
 }
 
 static int s3c_rtc_probe(struct platform_device *pdev)
-- 
2.9.3

[PATCH 5/6] rtc: s3c: Handle clock prepare failures in probe

[Krzysztof Kozlowski]

clk_prepare_enable() can fail so handle such case.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/rtc/rtc-s3c.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index c666b95fb8d7..0cb2f27a30b4 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -498,7 +498,9 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 			dev_dbg(&pdev->dev, "probe deferred due to missing rtc clk\n");
 		return ret;
 	}
-	clk_prepare_enable(info->rtc_clk);
+	ret = clk_prepare_enable(info->rtc_clk);
+	if (ret)
+		return ret;
 
 	if (info->data->needs_src_clk) {
 		info->rtc_src_clk = devm_clk_get(&pdev->dev, "rtc_src");
@@ -512,7 +514,9 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 					"probe deferred due to missing rtc src clk\n");
 			goto err_src_clk;
 		}
-		clk_prepare_enable(info->rtc_src_clk);
+		ret = clk_prepare_enable(info->rtc_src_clk);
+		if (ret)
+			goto err_src_clk;
 	}
 
 	/* check to see if everything is setup correctly */
-- 
2.9.3

[PATCH 6/6] rtc: s3c: Handle clock enable failures

[Krzysztof Kozlowski]

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(-)

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

Re: [PATCH 6/6] rtc: s3c: Handle clock enable failures

[Alexandre Belloni]

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

Re: [PATCH 6/6] rtc: s3c: Handle clock enable failures

[Krzysztof Kozlowski]

On Sat, Jun 24, 2017 at 10:55 PM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> 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.

Indeed, the platform clocks usually cannot fail (although not always,
e.g. some PLLs might return ETIMEDOUT on sync timeout) but in this
case one of the clocks is an I2C-controlled clock from PMIC. Its
clk_prepare() can fail. It does not implement clk_enable() but this
might change in the future so to avoid unexpected issues, it is better
just to handle this properly.

Best regards,
Krzysztof