[PATCH 1/2] watchdog: renesas-wdt: Check the return value from clk_prepare_enable()

[PATCH 1/2] watchdog: renesas-wdt: Check the return value from clk_prepare_enable()

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@nxp.com>

clk_prepare_enable() may fail, so we should better check its return value
and propagate it in the case of error.

Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 drivers/watchdog/renesas_wdt.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index cf61c92..ca3dc4c 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -62,9 +62,13 @@ static int rwdt_init_timeout(struct watchdog_device *wdev)
 
 static int rwdt_start(struct watchdog_device *wdev)
 {
+	int ret;
+
 	struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
 
-	clk_prepare_enable(priv->clk);
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return ret;
 
 	rwdt_write(priv, priv->cks, RWTCSRA);
 	rwdt_init_timeout(wdev);
-- 
2.7.4

[PATCH 2/2] watchdog: bcm7038: Check the return value from clk_prepare_enable()

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@nxp.com>

clk_prepare_enable() may fail, so we should better check its return value
and propagate it in the case of error.

Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 drivers/watchdog/bcm7038_wdt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/bcm7038_wdt.c b/drivers/watchdog/bcm7038_wdt.c
index c1b8e534..f88f546 100644
--- a/drivers/watchdog/bcm7038_wdt.c
+++ b/drivers/watchdog/bcm7038_wdt.c
@@ -136,7 +136,9 @@ static int bcm7038_wdt_probe(struct platform_device *pdev)
 	wdt->clk = devm_clk_get(dev, NULL);
 	/* If unable to get clock, use default frequency */
 	if (!IS_ERR(wdt->clk)) {
-		clk_prepare_enable(wdt->clk);
+		err = clk_prepare_enable(wdt->clk);
+		if (err)
+			return err;
 		wdt->rate = clk_get_rate(wdt->clk);
 		/* Prevent divide-by-zero exception */
 		if (!wdt->rate)
-- 
2.7.4

Re: [PATCH 1/2] watchdog: renesas-wdt: Check the return value from clk_prepare_enable()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 495 bytes --]

On Sat, Jul 22, 2017 at 05:22:58PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@nxp.com>
> 
> clk_prepare_enable() may fail, so we should better check its return value
> and propagate it in the case of error.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>

True. Yet, I have a patch in preparation which fully switches to
RuntimePM which will make manual clk handling superfluous.

I'd prefer to drop this patch because I'd need to revert it anyhow.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] watchdog: renesas-wdt: Check the return value from clk_prepare_enable()

[Guenter Roeck]

On Sat, Jul 22, 2017 at 10:31:59PM +0200, Wolfram Sang wrote:
> On Sat, Jul 22, 2017 at 05:22:58PM -0300, Fabio Estevam wrote:
> > From: Fabio Estevam <fabio.estevam@nxp.com>
> > 
> > clk_prepare_enable() may fail, so we should better check its return value
> > and propagate it in the case of error.
> > 
> > Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> 
> True. Yet, I have a patch in preparation which fully switches to
> RuntimePM which will make manual clk handling superfluous.
> 

It won't need clk_prepare_enable() anymore ?

Guenter

Re: [PATCH 1/2] watchdog: renesas-wdt: Check the return value from clk_prepare_enable()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 95 bytes --]


> It won't need clk_prepare_enable() anymore ?

Nope, RuntimePM does all the clock handling.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] watchdog: bcm7038: Check the return value from clk_prepare_enable()

[Guenter Roeck]

On Sat, Jul 22, 2017 at 05:22:59PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@nxp.com>
> 
> clk_prepare_enable() may fail, so we should better check its return value
> and propagate it in the case of error.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/bcm7038_wdt.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/bcm7038_wdt.c b/drivers/watchdog/bcm7038_wdt.c
> index c1b8e534..f88f546 100644
> --- a/drivers/watchdog/bcm7038_wdt.c
> +++ b/drivers/watchdog/bcm7038_wdt.c
> @@ -136,7 +136,9 @@ static int bcm7038_wdt_probe(struct platform_device *pdev)
>  	wdt->clk = devm_clk_get(dev, NULL);
>  	/* If unable to get clock, use default frequency */
>  	if (!IS_ERR(wdt->clk)) {
> -		clk_prepare_enable(wdt->clk);
> +		err = clk_prepare_enable(wdt->clk);

One of those situations where devm_clk_prepare_enable() would be really
useful.

Wonder if it would make sense introduce subsystem specific functions, such
as devm_watchdog_clk_prepare_enable(). That would at least be better than
devm_add_action() in each driver.

Guenter

> +		if (err)
> +			return err;
>  		wdt->rate = clk_get_rate(wdt->clk);
>  		/* Prevent divide-by-zero exception */
>  		if (!wdt->rate)
> -- 
> 2.7.4
> 

Re: [PATCH 1/2] watchdog: renesas-wdt: Check the return value from clk_prepare_enable()

[Guenter Roeck]

On Sat, Jul 22, 2017 at 11:28:19PM +0200, Wolfram Sang wrote:
> 
> > It won't need clk_prepare_enable() anymore ?
> 
> Nope, RuntimePM does all the clock handling.
> 

What if runtime PM is disabled ?

Thanks,
Guenter

Re: [PATCH 1/2] watchdog: renesas-wdt: Check the return value from clk_prepare_enable()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 322 bytes --]

On Sat, Jul 22, 2017 at 02:30:11PM -0700, Guenter Roeck wrote:
> On Sat, Jul 22, 2017 at 11:28:19PM +0200, Wolfram Sang wrote:
> > 
> > > It won't need clk_prepare_enable() anymore ?
> > 
> > Nope, RuntimePM does all the clock handling.
> > 
> What if runtime PM is disabled ?

PM is selected by RENESAS_ARCH.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]