[PATCH 1/2] watchdog: s3c2410_wdt: Use Use devm_clk_get[_optional]_enabled() helpers

[PATCH 1/2] watchdog: s3c2410_wdt: Use Use devm_clk_get[_optional]_enabled() helpers

[Guenter Roeck]

The devm_clk_get[_optional]_enabled() helpers:
    - call devm_clk_get[_optional]()
    - call clk_prepare_enable() and register what is needed in order to
      call clk_disable_unprepare() when needed, as a managed resource.

This simplifies the code and avoids the calls to clk_disable_unprepare().

While at it, use dev_err_probe consistently, and use its return value
to return the error code.

Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/s3c2410_wdt.c | 45 +++++++---------------------------
 1 file changed, 9 insertions(+), 36 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 200ba236a72e..a1fcb79b0b7c 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -661,35 +661,17 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	if (IS_ERR(wdt->reg_base))
 		return PTR_ERR(wdt->reg_base);
 
-	wdt->bus_clk = devm_clk_get(dev, "watchdog");
-	if (IS_ERR(wdt->bus_clk)) {
-		dev_err(dev, "failed to find bus clock\n");
-		return PTR_ERR(wdt->bus_clk);
-	}
-
-	ret = clk_prepare_enable(wdt->bus_clk);
-	if (ret < 0) {
-		dev_err(dev, "failed to enable bus clock\n");
-		return ret;
-	}
+	wdt->bus_clk = devm_clk_get_enabled(dev, "watchdog");
+	if (IS_ERR(wdt->bus_clk))
+		return dev_err_probe(dev, PTR_ERR(wdt->bus_clk), "failed to get bus clock\n");
 
 	/*
 	 * "watchdog_src" clock is optional; if it's not present -- just skip it
 	 * and use "watchdog" clock as both bus and source clock.
 	 */
-	wdt->src_clk = devm_clk_get_optional(dev, "watchdog_src");
-	if (IS_ERR(wdt->src_clk)) {
-		dev_err_probe(dev, PTR_ERR(wdt->src_clk),
-			      "failed to get source clock\n");
-		ret = PTR_ERR(wdt->src_clk);
-		goto err_bus_clk;
-	}
-
-	ret = clk_prepare_enable(wdt->src_clk);
-	if (ret) {
-		dev_err(dev, "failed to enable source clock\n");
-		goto err_bus_clk;
-	}
+	wdt->src_clk = devm_clk_get_optional_enabled(dev, "watchdog_src");
+	if (IS_ERR(wdt->src_clk))
+		return dev_err_probe(dev, PTR_ERR(wdt->src_clk), "failed to get source clock\n");
 
 	wdt->wdt_device.min_timeout = 1;
 	wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt);
@@ -710,7 +692,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 				 S3C2410_WATCHDOG_DEFAULT_TIME);
 		} else {
 			dev_err(dev, "failed to use default timeout\n");
-			goto err_src_clk;
+			return ret;
 		}
 	}
 
@@ -718,7 +700,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 			       pdev->name, pdev);
 	if (ret != 0) {
 		dev_err(dev, "failed to install irq (%d)\n", ret);
-		goto err_src_clk;
+		return ret;
 	}
 
 	watchdog_set_nowayout(&wdt->wdt_device, nowayout);
@@ -744,7 +726,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 
 	ret = watchdog_register_device(&wdt->wdt_device);
 	if (ret)
-		goto err_src_clk;
+		return ret;
 
 	ret = s3c2410wdt_enable(wdt, true);
 	if (ret < 0)
@@ -766,12 +748,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
  err_unregister:
 	watchdog_unregister_device(&wdt->wdt_device);
 
- err_src_clk:
-	clk_disable_unprepare(wdt->src_clk);
-
- err_bus_clk:
-	clk_disable_unprepare(wdt->bus_clk);
-
 	return ret;
 }
 
@@ -786,9 +762,6 @@ static int s3c2410wdt_remove(struct platform_device *dev)
 
 	watchdog_unregister_device(&wdt->wdt_device);
 
-	clk_disable_unprepare(wdt->src_clk);
-	clk_disable_unprepare(wdt->bus_clk);
-
 	return 0;
 }
 
-- 
2.39.1

[PATCH 2/2] watchdog: s3c2410_wdt: Use devm_add_action_or_reset() to disable watchdog

[Guenter Roeck]

Use devm_add_action_or_reset() to disable the watchdog when the driver
is removed to simplify the code. With this in place, we can use
devm_watchdog_register_device() to register the watchdog, and the removal
function is no longer necessary.

Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/s3c2410_wdt.c | 33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index a1fcb79b0b7c..58b262ca4e88 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -623,6 +623,11 @@ s3c2410_get_wdt_drv_data(struct platform_device *pdev)
 	return variant;
 }
 
+static void s3c2410wdt_wdt_disable_action(void *data)
+{
+	s3c2410wdt_enable(data, false);
+}
+
 static int s3c2410wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -724,13 +729,17 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 		s3c2410wdt_stop(&wdt->wdt_device);
 	}
 
-	ret = watchdog_register_device(&wdt->wdt_device);
+	ret = devm_watchdog_register_device(dev, &wdt->wdt_device);
 	if (ret)
 		return ret;
 
 	ret = s3c2410wdt_enable(wdt, true);
 	if (ret < 0)
-		goto err_unregister;
+		return ret;
+
+	ret = devm_add_action_or_reset(dev, s3c2410wdt_wdt_disable_action, wdt);
+	if (ret)
+		return ret;
 
 	platform_set_drvdata(pdev, wdt);
 
@@ -744,25 +753,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 		 (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis");
 
 	return 0;
-
- err_unregister:
-	watchdog_unregister_device(&wdt->wdt_device);
-
-	return ret;
-}
-
-static int s3c2410wdt_remove(struct platform_device *dev)
-{
-	int ret;
-	struct s3c2410_wdt *wdt = platform_get_drvdata(dev);
-
-	ret = s3c2410wdt_enable(wdt, false);
-	if (ret < 0)
-		return ret;
-
-	watchdog_unregister_device(&wdt->wdt_device);
-
-	return 0;
 }
 
 static void s3c2410wdt_shutdown(struct platform_device *dev)
@@ -817,7 +807,6 @@ static DEFINE_SIMPLE_DEV_PM_OPS(s3c2410wdt_pm_ops,
 
 static struct platform_driver s3c2410wdt_driver = {
 	.probe		= s3c2410wdt_probe,
-	.remove		= s3c2410wdt_remove,
 	.shutdown	= s3c2410wdt_shutdown,
 	.id_table	= s3c2410_wdt_ids,
 	.driver		= {
-- 
2.39.1

Re: [PATCH 2/2] watchdog: s3c2410_wdt: Use devm_add_action_or_reset() to disable watchdog

[Uwe Kleine-König]

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

On Sat, Mar 04, 2023 at 08:56:53AM -0800, Guenter Roeck wrote:
> Use devm_add_action_or_reset() to disable the watchdog when the driver
> is removed to simplify the code. With this in place, we can use
> devm_watchdog_register_device() to register the watchdog, and the removal
> function is no longer necessary.

While the cleanup in this driver is good (
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
) I wonder if disabling the watchdog at .remove() is right.

At least there is an inconsistency among watchdog drivers if the
hardware is supposed to stop or not.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 2/2] watchdog: s3c2410_wdt: Use devm_add_action_or_reset() to disable watchdog

[Guenter Roeck]

On 3/6/23 01:15, Uwe Kleine-König wrote:
> On Sat, Mar 04, 2023 at 08:56:53AM -0800, Guenter Roeck wrote:
>> Use devm_add_action_or_reset() to disable the watchdog when the driver
>> is removed to simplify the code. With this in place, we can use
>> devm_watchdog_register_device() to register the watchdog, and the removal
>> function is no longer necessary.
> 
> While the cleanup in this driver is good (
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ) I wonder if disabling the watchdog at .remove() is right.
> 
> At least there is an inconsistency among watchdog drivers if the
> hardware is supposed to stop or not.
> 

Yes, it is, and it is one of those endless-argument things. Some driver
authors insist that the watchdog be stopped, some insist that it isn't.
That is why we have watchdog_stop_on_unregister().

Note I didn't use that here because the watchdog isn't stopped on unregister
but just disabled. That is slightly different, and I didn't want to change
functionality.

Guenter

Re: [PATCH 1/2] watchdog: s3c2410_wdt: Use Use devm_clk_get[_optional]_enabled() helpers

[Christophe JAILLET]

Le 04/03/2023 à 17:56, Guenter Roeck a écrit :
> The devm_clk_get[_optional]_enabled() helpers:
>      - call devm_clk_get[_optional]()
>      - call clk_prepare_enable() and register what is needed in order to
>        call clk_disable_unprepare() when needed, as a managed resource.
> 
> This simplifies the code and avoids the calls to clk_disable_unprepare().
> 
> While at it, use dev_err_probe consistently, and use its return value
> to return the error code.
> 
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>   drivers/watchdog/s3c2410_wdt.c | 45 +++++++---------------------------
>   1 file changed, 9 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 200ba236a72e..a1fcb79b0b7c 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -661,35 +661,17 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>   	if (IS_ERR(wdt->reg_base))
>   		return PTR_ERR(wdt->reg_base);
>   
> -	wdt->bus_clk = devm_clk_get(dev, "watchdog");
> -	if (IS_ERR(wdt->bus_clk)) {
> -		dev_err(dev, "failed to find bus clock\n");
> -		return PTR_ERR(wdt->bus_clk);
> -	}
> -
> -	ret = clk_prepare_enable(wdt->bus_clk);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to enable bus clock\n");
> -		return ret;
> -	}
> +	wdt->bus_clk = devm_clk_get_enabled(dev, "watchdog");
> +	if (IS_ERR(wdt->bus_clk))
> +		return dev_err_probe(dev, PTR_ERR(wdt->bus_clk), "failed to get bus clock\n");
>   
>   	/*
>   	 * "watchdog_src" clock is optional; if it's not present -- just skip it
>   	 * and use "watchdog" clock as both bus and source clock.
>   	 */
> -	wdt->src_clk = devm_clk_get_optional(dev, "watchdog_src");
> -	if (IS_ERR(wdt->src_clk)) {
> -		dev_err_probe(dev, PTR_ERR(wdt->src_clk),
> -			      "failed to get source clock\n");
> -		ret = PTR_ERR(wdt->src_clk);
> -		goto err_bus_clk;
> -	}
> -
> -	ret = clk_prepare_enable(wdt->src_clk);
> -	if (ret) {
> -		dev_err(dev, "failed to enable source clock\n");
> -		goto err_bus_clk;
> -	}
> +	wdt->src_clk = devm_clk_get_optional_enabled(dev, "watchdog_src");
> +	if (IS_ERR(wdt->src_clk))
> +		return dev_err_probe(dev, PTR_ERR(wdt->src_clk), "failed to get source clock\n");
>   
>   	wdt->wdt_device.min_timeout = 1;
>   	wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt);
> @@ -710,7 +692,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>   				 S3C2410_WATCHDOG_DEFAULT_TIME);
>   		} else {
>   			dev_err(dev, "failed to use default timeout\n");
> -			goto err_src_clk;
> +			return ret;

Hi,

Nit: this also could be "return dev_err_probe()"

>   		}
>   	}
>   
> @@ -718,7 +700,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>   			       pdev->name, pdev);
>   	if (ret != 0) {
>   		dev_err(dev, "failed to install irq (%d)\n", ret);
> -		goto err_src_clk;
> +		return ret;

Nit: this also could be "return dev_err_probe()"

CJ

>   	}
>   
>   	watchdog_set_nowayout(&wdt->wdt_device, nowayout);
> @@ -744,7 +726,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>   
>   	ret = watchdog_register_device(&wdt->wdt_device);
>   	if (ret)
> -		goto err_src_clk;
> +		return ret;
>   
>   	ret = s3c2410wdt_enable(wdt, true);
>   	if (ret < 0)
> @@ -766,12 +748,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>    err_unregister:
>   	watchdog_unregister_device(&wdt->wdt_device);
>   
> - err_src_clk:
> -	clk_disable_unprepare(wdt->src_clk);
> -
> - err_bus_clk:
> -	clk_disable_unprepare(wdt->bus_clk);
> -
>   	return ret;
>   }
>   
> @@ -786,9 +762,6 @@ static int s3c2410wdt_remove(struct platform_device *dev)
>   
>   	watchdog_unregister_device(&wdt->wdt_device);
>   
> -	clk_disable_unprepare(wdt->src_clk);
> -	clk_disable_unprepare(wdt->bus_clk);
> -
>   	return 0;
>   }
>   

Re: [PATCH 1/2] watchdog: s3c2410_wdt: Use Use devm_clk_get[_optional]_enabled() helpers

[Guenter Roeck]

On 3/4/23 13:46, Christophe JAILLET wrote:
> Le 04/03/2023 à 17:56, Guenter Roeck a écrit :
>> The devm_clk_get[_optional]_enabled() helpers:
>>      - call devm_clk_get[_optional]()
>>      - call clk_prepare_enable() and register what is needed in order to
>>        call clk_disable_unprepare() when needed, as a managed resource.
>>
>> This simplifies the code and avoids the calls to clk_disable_unprepare().
>>
>> While at it, use dev_err_probe consistently, and use its return value
>> to return the error code.
>>
>> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   drivers/watchdog/s3c2410_wdt.c | 45 +++++++---------------------------
>>   1 file changed, 9 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
>> index 200ba236a72e..a1fcb79b0b7c 100644
>> --- a/drivers/watchdog/s3c2410_wdt.c
>> +++ b/drivers/watchdog/s3c2410_wdt.c
>> @@ -661,35 +661,17 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>>       if (IS_ERR(wdt->reg_base))
>>           return PTR_ERR(wdt->reg_base);
>> -    wdt->bus_clk = devm_clk_get(dev, "watchdog");
>> -    if (IS_ERR(wdt->bus_clk)) {
>> -        dev_err(dev, "failed to find bus clock\n");
>> -        return PTR_ERR(wdt->bus_clk);
>> -    }
>> -
>> -    ret = clk_prepare_enable(wdt->bus_clk);
>> -    if (ret < 0) {
>> -        dev_err(dev, "failed to enable bus clock\n");
>> -        return ret;
>> -    }
>> +    wdt->bus_clk = devm_clk_get_enabled(dev, "watchdog");
>> +    if (IS_ERR(wdt->bus_clk))
>> +        return dev_err_probe(dev, PTR_ERR(wdt->bus_clk), "failed to get bus clock\n");
>>       /*
>>        * "watchdog_src" clock is optional; if it's not present -- just skip it
>>        * and use "watchdog" clock as both bus and source clock.
>>        */
>> -    wdt->src_clk = devm_clk_get_optional(dev, "watchdog_src");
>> -    if (IS_ERR(wdt->src_clk)) {
>> -        dev_err_probe(dev, PTR_ERR(wdt->src_clk),
>> -                  "failed to get source clock\n");
>> -        ret = PTR_ERR(wdt->src_clk);
>> -        goto err_bus_clk;
>> -    }
>> -
>> -    ret = clk_prepare_enable(wdt->src_clk);
>> -    if (ret) {
>> -        dev_err(dev, "failed to enable source clock\n");
>> -        goto err_bus_clk;
>> -    }
>> +    wdt->src_clk = devm_clk_get_optional_enabled(dev, "watchdog_src");
>> +    if (IS_ERR(wdt->src_clk))
>> +        return dev_err_probe(dev, PTR_ERR(wdt->src_clk), "failed to get source clock\n");
>>       wdt->wdt_device.min_timeout = 1;
>>       wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt);
>> @@ -710,7 +692,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>>                    S3C2410_WATCHDOG_DEFAULT_TIME);
>>           } else {
>>               dev_err(dev, "failed to use default timeout\n");
>> -            goto err_src_clk;
>> +            return ret;
> 
> Hi,
> 
> Nit: this also could be "return dev_err_probe()"
> 
>>           }
>>       }
>> @@ -718,7 +700,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>>                      pdev->name, pdev);
>>       if (ret != 0) {
>>           dev_err(dev, "failed to install irq (%d)\n", ret);
>> -        goto err_src_clk;
>> +        return ret;
> 
> Nit: this also could be "return dev_err_probe()"
> 

The primary reason to call dev_err_probe() is that the error may be
-EPROBE_DEFER, in which case the error message is suppressed.
That is not the case for those two functions; they never return
-EPROBE_DEFER. Calling dev_err_probe() would give the false impression
that the functions _might_ return -EPROBE_DEFER.

Thanks,
Guenter

Re: [PATCH 1/2] watchdog: s3c2410_wdt: Use Use devm_clk_get[_optional]_enabled() helpers

[Uwe Kleine-König]

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

Hello Guenter,

On Sat, Mar 04, 2023 at 02:10:47PM -0800, Guenter Roeck wrote:
> On 3/4/23 13:46, Christophe JAILLET wrote:
> > Le 04/03/2023 à 17:56, Guenter Roeck a écrit :
> > > The devm_clk_get[_optional]_enabled() helpers:
> > >      - call devm_clk_get[_optional]()
> > >      - call clk_prepare_enable() and register what is needed in order to
> > >        call clk_disable_unprepare() when needed, as a managed resource.
> > > 
> > > This simplifies the code and avoids the calls to clk_disable_unprepare().
> > > 
> > > While at it, use dev_err_probe consistently, and use its return value
> > > to return the error code.
> > > 
> > > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > >   drivers/watchdog/s3c2410_wdt.c | 45 +++++++---------------------------
> > >   1 file changed, 9 insertions(+), 36 deletions(-)
> > > 
> > > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > > index 200ba236a72e..a1fcb79b0b7c 100644
> > > --- a/drivers/watchdog/s3c2410_wdt.c
> > > +++ b/drivers/watchdog/s3c2410_wdt.c
> > > @@ -661,35 +661,17 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > >       if (IS_ERR(wdt->reg_base))
> > >           return PTR_ERR(wdt->reg_base);
> > > -    wdt->bus_clk = devm_clk_get(dev, "watchdog");
> > > -    if (IS_ERR(wdt->bus_clk)) {
> > > -        dev_err(dev, "failed to find bus clock\n");
> > > -        return PTR_ERR(wdt->bus_clk);
> > > -    }
> > > -
> > > -    ret = clk_prepare_enable(wdt->bus_clk);
> > > -    if (ret < 0) {
> > > -        dev_err(dev, "failed to enable bus clock\n");
> > > -        return ret;
> > > -    }
> > > +    wdt->bus_clk = devm_clk_get_enabled(dev, "watchdog");
> > > +    if (IS_ERR(wdt->bus_clk))
> > > +        return dev_err_probe(dev, PTR_ERR(wdt->bus_clk), "failed to get bus clock\n");
> > >       /*
> > >        * "watchdog_src" clock is optional; if it's not present -- just skip it
> > >        * and use "watchdog" clock as both bus and source clock.
> > >        */
> > > -    wdt->src_clk = devm_clk_get_optional(dev, "watchdog_src");
> > > -    if (IS_ERR(wdt->src_clk)) {
> > > -        dev_err_probe(dev, PTR_ERR(wdt->src_clk),
> > > -                  "failed to get source clock\n");
> > > -        ret = PTR_ERR(wdt->src_clk);
> > > -        goto err_bus_clk;
> > > -    }
> > > -
> > > -    ret = clk_prepare_enable(wdt->src_clk);
> > > -    if (ret) {
> > > -        dev_err(dev, "failed to enable source clock\n");
> > > -        goto err_bus_clk;
> > > -    }
> > > +    wdt->src_clk = devm_clk_get_optional_enabled(dev, "watchdog_src");
> > > +    if (IS_ERR(wdt->src_clk))
> > > +        return dev_err_probe(dev, PTR_ERR(wdt->src_clk), "failed to get source clock\n");
> > >       wdt->wdt_device.min_timeout = 1;
> > >       wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt);
> > > @@ -710,7 +692,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > >                    S3C2410_WATCHDOG_DEFAULT_TIME);
> > >           } else {
> > >               dev_err(dev, "failed to use default timeout\n");
> > > -            goto err_src_clk;
> > > +            return ret;
> > 
> > Hi,
> > 
> > Nit: this also could be "return dev_err_probe()"
> > 
> > >           }
> > >       }
> > > @@ -718,7 +700,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > >                      pdev->name, pdev);
> > >       if (ret != 0) {
> > >           dev_err(dev, "failed to install irq (%d)\n", ret);
> > > -        goto err_src_clk;
> > > +        return ret;
> > 
> > Nit: this also could be "return dev_err_probe()"
> > 
> 
> The primary reason to call dev_err_probe() is that the error may be
> -EPROBE_DEFER, in which case the error message is suppressed.
> That is not the case for those two functions; they never return
> -EPROBE_DEFER. Calling dev_err_probe() would give the false impression
> that the functions _might_ return -EPROBE_DEFER.

That is subjective. In my book dev_err_probe() handling -EPROBE_DEFER is
only one aspect. Another is that using it allows to have return and error
message in a single line and also that if already other exit paths use
it to get a consistent style for the emitted messages. Having said that
*I* wouldn't assume that the previous call might return -EPROBE_DEFER
just because dev_err_probe() is used.

Having said that, I also don't think there is much harm if someone
thinks that a given function (here devm_request_irq()) might return
-EPROBE_DEFER.

Just my 0.02€
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 1/2] watchdog: s3c2410_wdt: Use Use devm_clk_get[_optional]_enabled() helpers

[Krzysztof Kozlowski]

On 05/03/2023 12:15, Uwe Kleine-König wrote:
> Hello Guenter,
> 
> On Sat, Mar 04, 2023 at 02:10:47PM -0800, Guenter Roeck wrote:
>> On 3/4/23 13:46, Christophe JAILLET wrote:
>>> Le 04/03/2023 à 17:56, Guenter Roeck a écrit :
>>>> The devm_clk_get[_optional]_enabled() helpers:
>>>>      - call devm_clk_get[_optional]()
>>>>      - call clk_prepare_enable() and register what is needed in order to
>>>>        call clk_disable_unprepare() when needed, as a managed resource.
>>>>
>>>> This simplifies the code and avoids the calls to clk_disable_unprepare().
>>>>
>>>> While at it, use dev_err_probe consistently, and use its return value
>>>> to return the error code.
>>>>
>>>> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>> ---
>>>>   drivers/watchdog/s3c2410_wdt.c | 45 +++++++---------------------------
>>>>   1 file changed, 9 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
>>>> index 200ba236a72e..a1fcb79b0b7c 100644
>>>> --- a/drivers/watchdog/s3c2410_wdt.c
>>>> +++ b/drivers/watchdog/s3c2410_wdt.c
>>>> @@ -661,35 +661,17 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>>>>       if (IS_ERR(wdt->reg_base))
>>>>           return PTR_ERR(wdt->reg_base);
>>>> -    wdt->bus_clk = devm_clk_get(dev, "watchdog");
>>>> -    if (IS_ERR(wdt->bus_clk)) {
>>>> -        dev_err(dev, "failed to find bus clock\n");
>>>> -        return PTR_ERR(wdt->bus_clk);
>>>> -    }
>>>> -
>>>> -    ret = clk_prepare_enable(wdt->bus_clk);
>>>> -    if (ret < 0) {
>>>> -        dev_err(dev, "failed to enable bus clock\n");
>>>> -        return ret;
>>>> -    }
>>>> +    wdt->bus_clk = devm_clk_get_enabled(dev, "watchdog");
>>>> +    if (IS_ERR(wdt->bus_clk))
>>>> +        return dev_err_probe(dev, PTR_ERR(wdt->bus_clk), "failed to get bus clock\n");
>>>>       /*
>>>>        * "watchdog_src" clock is optional; if it's not present -- just skip it
>>>>        * and use "watchdog" clock as both bus and source clock.
>>>>        */
>>>> -    wdt->src_clk = devm_clk_get_optional(dev, "watchdog_src");
>>>> -    if (IS_ERR(wdt->src_clk)) {
>>>> -        dev_err_probe(dev, PTR_ERR(wdt->src_clk),
>>>> -                  "failed to get source clock\n");
>>>> -        ret = PTR_ERR(wdt->src_clk);
>>>> -        goto err_bus_clk;
>>>> -    }
>>>> -
>>>> -    ret = clk_prepare_enable(wdt->src_clk);
>>>> -    if (ret) {
>>>> -        dev_err(dev, "failed to enable source clock\n");
>>>> -        goto err_bus_clk;
>>>> -    }
>>>> +    wdt->src_clk = devm_clk_get_optional_enabled(dev, "watchdog_src");
>>>> +    if (IS_ERR(wdt->src_clk))
>>>> +        return dev_err_probe(dev, PTR_ERR(wdt->src_clk), "failed to get source clock\n");
>>>>       wdt->wdt_device.min_timeout = 1;
>>>>       wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt);
>>>> @@ -710,7 +692,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>>>>                    S3C2410_WATCHDOG_DEFAULT_TIME);
>>>>           } else {
>>>>               dev_err(dev, "failed to use default timeout\n");
>>>> -            goto err_src_clk;
>>>> +            return ret;
>>>
>>> Hi,
>>>
>>> Nit: this also could be "return dev_err_probe()"
>>>
>>>>           }
>>>>       }
>>>> @@ -718,7 +700,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>>>>                      pdev->name, pdev);
>>>>       if (ret != 0) {
>>>>           dev_err(dev, "failed to install irq (%d)\n", ret);
>>>> -        goto err_src_clk;
>>>> +        return ret;
>>>
>>> Nit: this also could be "return dev_err_probe()"
>>>
>>
>> The primary reason to call dev_err_probe() is that the error may be
>> -EPROBE_DEFER, in which case the error message is suppressed.
>> That is not the case for those two functions; they never return
>> -EPROBE_DEFER. Calling dev_err_probe() would give the false impression
>> that the functions _might_ return -EPROBE_DEFER.
> 
> That is subjective. In my book dev_err_probe() handling -EPROBE_DEFER is
> only one aspect. Another is that using it allows to have return and error
> message in a single line and also that if already other exit paths use
> it to get a consistent style for the emitted messages. Having said that
> *I* wouldn't assume that the previous call might return -EPROBE_DEFER
> just because dev_err_probe() is used.
> 

I agree with this. I stopped looking at dev_err_probe() as related to
deferred probe. It is just useful helper in the context of probe. With
or without defer.

Best regards,
Krzysztof

Re: [PATCH 1/2] watchdog: s3c2410_wdt: Use Use devm_clk_get[_optional]_enabled() helpers

[Guenter Roeck]

On Sun, Mar 05, 2023 at 12:15:00PM +0100, Uwe Kleine-König wrote:
> Hello Guenter,
> 
> On Sat, Mar 04, 2023 at 02:10:47PM -0800, Guenter Roeck wrote:
> > On 3/4/23 13:46, Christophe JAILLET wrote:
> > > Le 04/03/2023 à 17:56, Guenter Roeck a écrit :
> > > > The devm_clk_get[_optional]_enabled() helpers:
> > > >      - call devm_clk_get[_optional]()
> > > >      - call clk_prepare_enable() and register what is needed in order to
> > > >        call clk_disable_unprepare() when needed, as a managed resource.
> > > > 
> > > > This simplifies the code and avoids the calls to clk_disable_unprepare().
> > > > 
> > > > While at it, use dev_err_probe consistently, and use its return value
> > > > to return the error code.
> > > > 
> > > > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > > ---
> > > >   drivers/watchdog/s3c2410_wdt.c | 45 +++++++---------------------------
> > > >   1 file changed, 9 insertions(+), 36 deletions(-)
> > > > 
> > > > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > > > index 200ba236a72e..a1fcb79b0b7c 100644
> > > > --- a/drivers/watchdog/s3c2410_wdt.c
> > > > +++ b/drivers/watchdog/s3c2410_wdt.c
> > > > @@ -661,35 +661,17 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > > >       if (IS_ERR(wdt->reg_base))
> > > >           return PTR_ERR(wdt->reg_base);
> > > > -    wdt->bus_clk = devm_clk_get(dev, "watchdog");
> > > > -    if (IS_ERR(wdt->bus_clk)) {
> > > > -        dev_err(dev, "failed to find bus clock\n");
> > > > -        return PTR_ERR(wdt->bus_clk);
> > > > -    }
> > > > -
> > > > -    ret = clk_prepare_enable(wdt->bus_clk);
> > > > -    if (ret < 0) {
> > > > -        dev_err(dev, "failed to enable bus clock\n");
> > > > -        return ret;
> > > > -    }
> > > > +    wdt->bus_clk = devm_clk_get_enabled(dev, "watchdog");
> > > > +    if (IS_ERR(wdt->bus_clk))
> > > > +        return dev_err_probe(dev, PTR_ERR(wdt->bus_clk), "failed to get bus clock\n");
> > > >       /*
> > > >        * "watchdog_src" clock is optional; if it's not present -- just skip it
> > > >        * and use "watchdog" clock as both bus and source clock.
> > > >        */
> > > > -    wdt->src_clk = devm_clk_get_optional(dev, "watchdog_src");
> > > > -    if (IS_ERR(wdt->src_clk)) {
> > > > -        dev_err_probe(dev, PTR_ERR(wdt->src_clk),
> > > > -                  "failed to get source clock\n");
> > > > -        ret = PTR_ERR(wdt->src_clk);
> > > > -        goto err_bus_clk;
> > > > -    }
> > > > -
> > > > -    ret = clk_prepare_enable(wdt->src_clk);
> > > > -    if (ret) {
> > > > -        dev_err(dev, "failed to enable source clock\n");
> > > > -        goto err_bus_clk;
> > > > -    }
> > > > +    wdt->src_clk = devm_clk_get_optional_enabled(dev, "watchdog_src");
> > > > +    if (IS_ERR(wdt->src_clk))
> > > > +        return dev_err_probe(dev, PTR_ERR(wdt->src_clk), "failed to get source clock\n");
> > > >       wdt->wdt_device.min_timeout = 1;
> > > >       wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt);
> > > > @@ -710,7 +692,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > > >                    S3C2410_WATCHDOG_DEFAULT_TIME);
> > > >           } else {
> > > >               dev_err(dev, "failed to use default timeout\n");
> > > > -            goto err_src_clk;
> > > > +            return ret;
> > > 
> > > Hi,
> > > 
> > > Nit: this also could be "return dev_err_probe()"
> > > 
> > > >           }
> > > >       }
> > > > @@ -718,7 +700,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > > >                      pdev->name, pdev);
> > > >       if (ret != 0) {
> > > >           dev_err(dev, "failed to install irq (%d)\n", ret);
> > > > -        goto err_src_clk;
> > > > +        return ret;
> > > 
> > > Nit: this also could be "return dev_err_probe()"
> > > 
> > 
> > The primary reason to call dev_err_probe() is that the error may be
> > -EPROBE_DEFER, in which case the error message is suppressed.
> > That is not the case for those two functions; they never return
> > -EPROBE_DEFER. Calling dev_err_probe() would give the false impression
> > that the functions _might_ return -EPROBE_DEFER.
> 
> That is subjective. In my book dev_err_probe() handling -EPROBE_DEFER is
> only one aspect. Another is that using it allows to have return and error
> message in a single line and also that if already other exit paths use
> it to get a consistent style for the emitted messages. Having said that
> *I* wouldn't assume that the previous call might return -EPROBE_DEFER
> just because dev_err_probe() is used.
> 
> Having said that, I also don't think there is much harm if someone
> thinks that a given function (here devm_request_irq()) might return
> -EPROBE_DEFER.
> 

I guess we'll have to agree to disagree.

Guenter

[PATCH 0/3] watchdog: s3c2410_wdt: Simplify using dev_err_probe()

[Uwe Kleine-König]

Hello,

On Sun, Mar 05, 2023 at 06:31:08AM -0800, Guenter Roeck wrote:
> On Sun, Mar 05, 2023 at 12:15:00PM +0100, Uwe Kleine-König wrote:
> > On Sat, Mar 04, 2023 at 02:10:47PM -0800, Guenter Roeck wrote:
> > > The primary reason to call dev_err_probe() is that the error may be
> > > -EPROBE_DEFER, in which case the error message is suppressed.
> > > That is not the case for those two functions; they never return
> > > -EPROBE_DEFER. Calling dev_err_probe() would give the false impression
> > > that the functions _might_ return -EPROBE_DEFER.
> > 
> > That is subjective. In my book dev_err_probe() handling -EPROBE_DEFER is
> > only one aspect. Another is that using it allows to have return and error
> > message in a single line and also that if already other exit paths use
> > it to get a consistent style for the emitted messages. Having said that
> > *I* wouldn't assume that the previous call might return -EPROBE_DEFER
> > just because dev_err_probe() is used.
> > 
> > Having said that, I also don't think there is much harm if someone
> > thinks that a given function (here devm_request_irq()) might return
> > -EPROBE_DEFER.
> > 
> 
> I guess we'll have to agree to disagree.

I don't agree to disagree. In my eyes you weight the corresponding facts
in an irrational way. To have some code changes to talk about, I created
this series (on top of Guenter's patches to this driver from Saturday
[1]).

Patch 1 is necessary to effectively make use of dev_err_probe(). I admit
this annoys me a bit as it shows that dev_err_probe() isn't a plug-in
replacement, but given that it reduces the binary size a bit, it has at
least positive usefulness. (But maybe this is subjective? *shrug*)

Patch 2 unifies the format of the error messages between the error paths
that make use of dev_err_probe() and those that (only) use dev_err().
IMHO this has mixed value, but still positive in sum. While having a
consistent error log style is nice, having to manually use
dev_err_probe()'s style is a bit ugly. Together with a) the messages get
more useful mentioning the error code and b) the downside is removed by
patch 3, I still like it. (And if you don't because of b), please
consider squashing patches 2 and 3 together. In fact I only created
patch 2 to make the upsides for patch 3 more obvious and I don't mind a
squash.)

Patch 3 does the actual conversion to dev_err_probe() for all error
paths in .probe(). The (unarguable?) upsides are:

 - Reduced line count
 - Reduced binary size (and the reduction mentioned in the commit log
   doesn't even account for the shorter format strings!)

The fact where Guenter doesn't agree to me (and Krzysztof) is:

 - You cannot any more defer from the usage of dev_err_probe() vs.
   dev_err() if the function that failed before might return
   -EPROBE_DEFER.

In my eyes this is irrelevant because there is no objective reason to
have to know that. If the function might return -EPROBE_DEFER or not
shouldn't (and doesn't!) have an influence on how the driver should
behave in the error case. Also the ability to defer that property is
very unreliable. It's not even reliable in drivers/watchdog, look at how
imx2_wdt handles devm_clk_get() or keembay_wdt handles clk_get_rate()
returning zero.

So using dev_err_probe() has two big benefits in contrast to a dubious
and unreliable connection between -EPROBE_DEFER and dev_err_probe().

Best regards
Uwe

[1] https://lore.kernel.org/linux-watchdog/20230304165653.2179835-1-linux@roeck-us.net

Uwe Kleine-König (3):
  watchdog: s3c2410_wdt: Fold s3c2410_get_wdt_drv_data() into only
    caller
  watchdog: s3c2410_wdt: Unify error logging format in probe function
  watchdog: s3c2410_wdt: Simplify using dev_err_probe()

 drivers/watchdog/s3c2410_wdt.c | 93 ++++++++++++++--------------------
 1 file changed, 37 insertions(+), 56 deletions(-)


base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
prerequisite-patch-id: 775bdd863307268e1ef16250bf2f40862637b453
prerequisite-patch-id: 924ddfbe583e97e7c9a46c2460ecbc88c29ee319
-- 
2.39.1

[PATCH 1/3] watchdog: s3c2410_wdt: Fold s3c2410_get_wdt_drv_data() into only caller

[Uwe Kleine-König]

s3c2410_get_wdt_drv_data() is only called by s3c2410wdt_probe(), so the
implementation of the former can move to the latter.

scripts/bloat-o-meter reports for this change (on an ARCH=arm
allmodconfig build):

	add/remove: 1/1 grow/shrink: 0/2 up/down: 4/-129 (-125)

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/watchdog/s3c2410_wdt.c | 70 +++++++++++++++-------------------
 1 file changed, 30 insertions(+), 40 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 58b262ca4e88..7382bf038161 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -579,11 +579,27 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
 	return 0;
 }
 
-static inline const struct s3c2410_wdt_variant *
-s3c2410_get_wdt_drv_data(struct platform_device *pdev)
+static void s3c2410wdt_wdt_disable_action(void *data)
+{
+	s3c2410wdt_enable(data, false);
+}
+
+static int s3c2410wdt_probe(struct platform_device *pdev)
 {
-	const struct s3c2410_wdt_variant *variant;
 	struct device *dev = &pdev->dev;
+	const struct s3c2410_wdt_variant *variant;
+	struct s3c2410_wdt *wdt;
+	unsigned int wtcon;
+	int wdt_irq;
+	int ret;
+
+	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
+
+	wdt->dev = dev;
+	spin_lock_init(&wdt->lock);
+	wdt->wdt_device = s3c2410_wdd;
 
 	variant = of_device_get_match_data(dev);
 	if (!variant) {
@@ -597,56 +613,30 @@ s3c2410_get_wdt_drv_data(struct platform_device *pdev)
 	if (variant == &drv_data_exynos850_cl0 ||
 	    variant == &drv_data_exynosautov9_cl0) {
 		u32 index;
-		int err;
 
-		err = of_property_read_u32(dev->of_node,
+		ret = of_property_read_u32(dev->of_node,
 					   "samsung,cluster-index", &index);
-		if (err) {
+		if (ret) {
 			dev_err(dev, "failed to get cluster index\n");
-			return NULL;
+			return -EINVAL;
 		}
 
 		switch (index) {
 		case 0:
-			return variant;
+			break;
 		case 1:
-			return (variant == &drv_data_exynos850_cl0) ?
-				&drv_data_exynos850_cl1 :
-				&drv_data_exynosautov9_cl1;
+			if (variant == &drv_data_exynos850_cl0)
+				variant = &drv_data_exynos850_cl1;
+			else
+				variant = &drv_data_exynosautov9_cl1;
+			break;
 		default:
 			dev_err(dev, "wrong cluster index: %u\n", index);
-			return NULL;
+			return -EINVAL;
 		}
 	}
 #endif
-
-	return variant;
-}
-
-static void s3c2410wdt_wdt_disable_action(void *data)
-{
-	s3c2410wdt_enable(data, false);
-}
-
-static int s3c2410wdt_probe(struct platform_device *pdev)
-{
-	struct device *dev = &pdev->dev;
-	struct s3c2410_wdt *wdt;
-	unsigned int wtcon;
-	int wdt_irq;
-	int ret;
-
-	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
-	if (!wdt)
-		return -ENOMEM;
-
-	wdt->dev = dev;
-	spin_lock_init(&wdt->lock);
-	wdt->wdt_device = s3c2410_wdd;
-
-	wdt->drv_data = s3c2410_get_wdt_drv_data(pdev);
-	if (!wdt->drv_data)
-		return -EINVAL;
+	wdt->drv_data = variant;
 
 	if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
 		wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
-- 
2.39.1

Re: [PATCH 1/3] watchdog: s3c2410_wdt: Fold s3c2410_get_wdt_drv_data() into only caller

[Guenter Roeck]

On 3/6/23 01:09, Uwe Kleine-König wrote:
> s3c2410_get_wdt_drv_data() is only called by s3c2410wdt_probe(), so the
> implementation of the former can move to the latter.
> 
> scripts/bloat-o-meter reports for this change (on an ARCH=arm
> allmodconfig build):
> 
> 	add/remove: 1/1 grow/shrink: 0/2 up/down: 4/-129 (-125)
> 

The reason for separating functions in this case wasn't that the separate function
would be called several times. It was to improve code readability. If anything,
I would argue that it might sense to split the already lengthy probe function
further instead of combining it.

Maybe I am old fashioned. Maybe the old "split your code into multiple functions
if the function size is larger than X lines" no longer applies, and it is now
"never split functions unless the separated function is called more than once".
Still, I am quite concerned that accepting this patch would result in a flurry
of similar patches which would all do nothing but hurt readability, using the
same set of arguments. I really don't like where this is going. I am going to
leave it up to Wim to decide if and how to proceed.

Guenter

Re: [PATCH 1/3] watchdog: s3c2410_wdt: Fold s3c2410_get_wdt_drv_data() into only caller

[Uwe Kleine-König]

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

On Mon, Mar 06, 2023 at 09:17:23AM -0800, Guenter Roeck wrote:
> On 3/6/23 01:09, Uwe Kleine-König wrote:
> > s3c2410_get_wdt_drv_data() is only called by s3c2410wdt_probe(), so the
> > implementation of the former can move to the latter.
> > 
> > scripts/bloat-o-meter reports for this change (on an ARCH=arm
> > allmodconfig build):
> > 
> > 	add/remove: 1/1 grow/shrink: 0/2 up/down: 4/-129 (-125)
> > 
> 
> The reason for separating functions in this case wasn't that the separate function
> would be called several times. It was to improve code readability. If anything,
> I would argue that it might sense to split the already lengthy probe function
> further instead of combining it.

Agreed. For dev_err_probe() to work the following would be alternatively
possible:

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 58b262ca4e88..564919717761 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -579,8 +579,8 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
 	return 0;
 }
 
-static inline const struct s3c2410_wdt_variant *
-s3c2410_get_wdt_drv_data(struct platform_device *pdev)
+static inline int
+s3c2410_get_wdt_drv_data(struct platform_device *pdev, struct s3c2410_wdt *wdt)
 {
 	const struct s3c2410_wdt_variant *variant;
 	struct device *dev = &pdev->dev;
@@ -603,24 +603,26 @@ s3c2410_get_wdt_drv_data(struct platform_device *pdev)
 					   "samsung,cluster-index", &index);
 		if (err) {
 			dev_err(dev, "failed to get cluster index\n");
-			return NULL;
+			return -EINVAL;
 		}
 
 		switch (index) {
 		case 0:
-			return variant;
+			break;
 		case 1:
-			return (variant == &drv_data_exynos850_cl0) ?
+			variant = (variant == &drv_data_exynos850_cl0) ?
 				&drv_data_exynos850_cl1 :
 				&drv_data_exynosautov9_cl1;
+			break;
 		default:
 			dev_err(dev, "wrong cluster index: %u\n", index);
-			return NULL;
+			return -EINVAL;
 		}
 	}
 #endif
+	wdt->drv_data = variant;
 
-	return variant;
+	return 0;
 }
 
 static void s3c2410wdt_wdt_disable_action(void *data)
@@ -644,9 +646,9 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	spin_lock_init(&wdt->lock);
 	wdt->wdt_device = s3c2410_wdd;
 
-	wdt->drv_data = s3c2410_get_wdt_drv_data(pdev);
-	if (!wdt->drv_data)
-		return -EINVAL;
+	ret = s3c2410_get_wdt_drv_data(pdev, wdt);
+	if (ret)
+		return ret;
 
 	if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
 		wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,

I didn't check if it's the same as for my initial patch #1, but for an
allmodconfig ARCH=arm build this also has a nice bloatometer output:

	add/remove: 1/1 grow/shrink: 0/1 up/down: 4/-104 (-100)

This would allow to make use of dev_err_probe() in
s3c2410_get_wdt_drv_data() and still maintain the split into two
functions.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 1/3] watchdog: s3c2410_wdt: Fold s3c2410_get_wdt_drv_data() into only caller

[Guenter Roeck]

On 3/6/23 09:47, Uwe Kleine-König wrote:
> On Mon, Mar 06, 2023 at 09:17:23AM -0800, Guenter Roeck wrote:
>> On 3/6/23 01:09, Uwe Kleine-König wrote:
>>> s3c2410_get_wdt_drv_data() is only called by s3c2410wdt_probe(), so the
>>> implementation of the former can move to the latter.
>>>
>>> scripts/bloat-o-meter reports for this change (on an ARCH=arm
>>> allmodconfig build):
>>>
>>> 	add/remove: 1/1 grow/shrink: 0/2 up/down: 4/-129 (-125)
>>>
>>
>> The reason for separating functions in this case wasn't that the separate function
>> would be called several times. It was to improve code readability. If anything,
>> I would argue that it might sense to split the already lengthy probe function
>> further instead of combining it.
> 
> Agreed. For dev_err_probe() to work the following would be alternatively
> possible:
> 

That looks ok to me.

Thanks,
Guenter

> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 58b262ca4e88..564919717761 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -579,8 +579,8 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
>   	return 0;
>   }
>   
> -static inline const struct s3c2410_wdt_variant *
> -s3c2410_get_wdt_drv_data(struct platform_device *pdev)
> +static inline int
> +s3c2410_get_wdt_drv_data(struct platform_device *pdev, struct s3c2410_wdt *wdt)
>   {
>   	const struct s3c2410_wdt_variant *variant;
>   	struct device *dev = &pdev->dev;
> @@ -603,24 +603,26 @@ s3c2410_get_wdt_drv_data(struct platform_device *pdev)
>   					   "samsung,cluster-index", &index);
>   		if (err) {
>   			dev_err(dev, "failed to get cluster index\n");
> -			return NULL;
> +			return -EINVAL;
>   		}
>   
>   		switch (index) {
>   		case 0:
> -			return variant;
> +			break;
>   		case 1:
> -			return (variant == &drv_data_exynos850_cl0) ?
> +			variant = (variant == &drv_data_exynos850_cl0) ?
>   				&drv_data_exynos850_cl1 :
>   				&drv_data_exynosautov9_cl1;
> +			break;
>   		default:
>   			dev_err(dev, "wrong cluster index: %u\n", index);
> -			return NULL;
> +			return -EINVAL;
>   		}
>   	}
>   #endif
> +	wdt->drv_data = variant;
>   
> -	return variant;
> +	return 0;
>   }
>   
>   static void s3c2410wdt_wdt_disable_action(void *data)
> @@ -644,9 +646,9 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>   	spin_lock_init(&wdt->lock);
>   	wdt->wdt_device = s3c2410_wdd;
>   
> -	wdt->drv_data = s3c2410_get_wdt_drv_data(pdev);
> -	if (!wdt->drv_data)
> -		return -EINVAL;
> +	ret = s3c2410_get_wdt_drv_data(pdev, wdt);
> +	if (ret)
> +		return ret;
>   
>   	if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
>   		wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> 
> I didn't check if it's the same as for my initial patch #1, but for an
> allmodconfig ARCH=arm build this also has a nice bloatometer output:
> 
> 	add/remove: 1/1 grow/shrink: 0/1 up/down: 4/-104 (-100)
> 
> This would allow to make use of dev_err_probe() in
> s3c2410_get_wdt_drv_data() and still maintain the split into two
> functions.
> 
> Best regards
> Uwe
> 

[PATCH 2/3] watchdog: s3c2410_wdt: Unify error logging format in probe function

[Uwe Kleine-König]

In the probe function a mixture of dev_err() and dev_err_probe() is used.
Align the format of the error messages when dev_err() is used to that of
dev_err_probe() for consistency. Apart from consistency this also
mentions the human readable error code.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/watchdog/s3c2410_wdt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 7382bf038161..93df6ee331a0 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -617,7 +617,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 		ret = of_property_read_u32(dev->of_node,
 					   "samsung,cluster-index", &index);
 		if (ret) {
-			dev_err(dev, "failed to get cluster index\n");
+			dev_err(dev, "error %pe: failed to get cluster index\n", ERR_PTR(-EINVAL));
 			return -EINVAL;
 		}
 
@@ -631,7 +631,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 				variant = &drv_data_exynosautov9_cl1;
 			break;
 		default:
-			dev_err(dev, "wrong cluster index: %u\n", index);
+			dev_err(dev, "error %pe: wrong cluster index: %u\n", ERR_PTR(-EINVAL), index);
 			return -EINVAL;
 		}
 	}
@@ -642,7 +642,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 		wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
 						"samsung,syscon-phandle");
 		if (IS_ERR(wdt->pmureg)) {
-			dev_err(dev, "syscon regmap lookup failed.\n");
+			dev_err(dev, "error %pe: syscon regmap lookup failed.\n", wdt->pmureg);
 			return PTR_ERR(wdt->pmureg);
 		}
 	}
@@ -686,7 +686,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 			dev_warn(dev, "tmr_margin value out of range, default %d used\n",
 				 S3C2410_WATCHDOG_DEFAULT_TIME);
 		} else {
-			dev_err(dev, "failed to use default timeout\n");
+			dev_err(dev, "error: %pe: failed to use default timeout\n", ERR_PTR(ret));
 			return ret;
 		}
 	}
@@ -694,7 +694,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	ret = devm_request_irq(dev, wdt_irq, s3c2410wdt_irq, 0,
 			       pdev->name, pdev);
 	if (ret != 0) {
-		dev_err(dev, "failed to install irq (%d)\n", ret);
+		dev_err(dev, "error: %pe: failed to install irq\n", ERR_PTR(ret));
 		return ret;
 	}
 
-- 
2.39.1

[PATCH 3/3] watchdog: s3c2410_wdt: Simplify using dev_err_probe()

[Uwe Kleine-König]

Make use of dev_err_probe() also for error paths that don't have to
handle -EPROBE_DEFER. While the code handing -EPROBE_DEFER isn't used
for these error paths, it still simpler as it cares for pretty printing
the error code and usually needs on line less to use as it combines
message emitting and error returning.

Apart from the reduction of line count, scripts/bloat-o-meter reports
for this change (on an ARCH=arm allmodconfig build):

	add/remove: 1/1 grow/shrink: 1/2 up/down: 32/-144 (-112)

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/watchdog/s3c2410_wdt.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 93df6ee331a0..c3b50266deab 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -616,10 +616,8 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 
 		ret = of_property_read_u32(dev->of_node,
 					   "samsung,cluster-index", &index);
-		if (ret) {
-			dev_err(dev, "error %pe: failed to get cluster index\n", ERR_PTR(-EINVAL));
-			return -EINVAL;
-		}
+		if (ret)
+			return dev_err_probe(dev, -EINVAL, "failed to get cluster index\n");
 
 		switch (index) {
 		case 0:
@@ -631,8 +629,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 				variant = &drv_data_exynosautov9_cl1;
 			break;
 		default:
-			dev_err(dev, "error %pe: wrong cluster index: %u\n", ERR_PTR(-EINVAL), index);
-			return -EINVAL;
+			return dev_err_probe(dev, -EINVAL, "wrong cluster index: %u\n", index);
 		}
 	}
 #endif
@@ -641,10 +638,8 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
 		wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
 						"samsung,syscon-phandle");
-		if (IS_ERR(wdt->pmureg)) {
-			dev_err(dev, "error %pe: syscon regmap lookup failed.\n", wdt->pmureg);
-			return PTR_ERR(wdt->pmureg);
-		}
+		if (IS_ERR(wdt->pmureg))
+			return dev_err_probe(dev, PTR_ERR(wdt->pmureg), "syscon regmap lookup failed.\n");
 	}
 
 	wdt_irq = platform_get_irq(pdev, 0);
@@ -682,21 +677,17 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
 	if (ret) {
 		ret = s3c2410wdt_set_heartbeat(&wdt->wdt_device,
 					       S3C2410_WATCHDOG_DEFAULT_TIME);
-		if (ret == 0) {
+		if (ret == 0)
 			dev_warn(dev, "tmr_margin value out of range, default %d used\n",
 				 S3C2410_WATCHDOG_DEFAULT_TIME);
-		} else {
-			dev_err(dev, "error: %pe: failed to use default timeout\n", ERR_PTR(ret));
-			return ret;
-		}
+		else
+			return dev_err_probe(dev, ret, "failed to use default timeout\n");
 	}
 
 	ret = devm_request_irq(dev, wdt_irq, s3c2410wdt_irq, 0,
 			       pdev->name, pdev);
-	if (ret != 0) {
-		dev_err(dev, "error: %pe: failed to install irq\n", ERR_PTR(ret));
-		return ret;
-	}
+	if (ret != 0)
+		return dev_err_probe(dev, ret, "failed to install irq\n");
 
 	watchdog_set_nowayout(&wdt->wdt_device, nowayout);
 	watchdog_set_restart_priority(&wdt->wdt_device, 128);
-- 
2.39.1

Re: [PATCH 1/2] watchdog: s3c2410_wdt: Use Use devm_clk_get[_optional]_enabled() helpers

[Uwe Kleine-König]

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

Hello Guenter,

On Sat, Mar 04, 2023 at 08:56:52AM -0800, Guenter Roeck wrote:
> The devm_clk_get[_optional]_enabled() helpers:
>     - call devm_clk_get[_optional]()
>     - call clk_prepare_enable() and register what is needed in order to
>       call clk_disable_unprepare() when needed, as a managed resource.
> 
> This simplifies the code and avoids the calls to clk_disable_unprepare().
> 
> While at it, use dev_err_probe consistently, and use its return value
> to return the error code.
> 
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 1/2] watchdog: s3c2410_wdt: Use Use devm_clk_get[_optional]_enabled() helpers

[Uwe Kleine-König]

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

Hello,

On Mon, Mar 06, 2023 at 10:10:48AM +0100, Uwe Kleine-König wrote:
> On Sat, Mar 04, 2023 at 08:56:52AM -0800, Guenter Roeck wrote:
> > The devm_clk_get[_optional]_enabled() helpers:
> >     - call devm_clk_get[_optional]()
> >     - call clk_prepare_enable() and register what is needed in order to
> >       call clk_disable_unprepare() when needed, as a managed resource.
> > 
> > This simplifies the code and avoids the calls to clk_disable_unprepare().
> > 
> > While at it, use dev_err_probe consistently, and use its return value
> > to return the error code.
> > 
> > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> 
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

This patch is in next now as b05a2e58c16c47f3d752b7db1714ef077e5b82d9.
My name occurs twice in the tag area, once it is mangled as

	Uwe Kleine-K=F6nig

I would welcome fixing that (i.e. s/=F6/ö/). When this commit is
touched, you can also do s/Use Use/Use/ in the Subject.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 1/2] watchdog: s3c2410_wdt: Use Use devm_clk_get[_optional]_enabled() helpers

[Wim Van Sebroeck]

Hi Uwe,

> Hello,
> 
> On Mon, Mar 06, 2023 at 10:10:48AM +0100, Uwe Kleine-König wrote:
> > On Sat, Mar 04, 2023 at 08:56:52AM -0800, Guenter Roeck wrote:
> > > The devm_clk_get[_optional]_enabled() helpers:
> > >     - call devm_clk_get[_optional]()
> > >     - call clk_prepare_enable() and register what is needed in order to
> > >       call clk_disable_unprepare() when needed, as a managed resource.
> > > 
> > > This simplifies the code and avoids the calls to clk_disable_unprepare().
> > > 
> > > While at it, use dev_err_probe consistently, and use its return value
> > > to return the error code.
> > > 
> > > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > 
> > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> This patch is in next now as b05a2e58c16c47f3d752b7db1714ef077e5b82d9.
> My name occurs twice in the tag area, once it is mangled as
> 
> 	Uwe Kleine-K=F6nig
> 
> I would welcome fixing that (i.e. s/=F6/ö/). When this commit is
> touched, you can also do s/Use Use/Use/ in the Subject.

Fixed.

Kind regards,
wim.

Re: [PATCH 1/2] watchdog: s3c2410_wdt: Use Use devm_clk_get[_optional]_enabled() helpers

[Uwe Kleine-König]

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

On Sat, Apr 22, 2023 at 01:22:29PM +0200, Wim Van Sebroeck wrote:
> Hi Uwe,
> 
> > Hello,
> > 
> > On Mon, Mar 06, 2023 at 10:10:48AM +0100, Uwe Kleine-König wrote:
> > > On Sat, Mar 04, 2023 at 08:56:52AM -0800, Guenter Roeck wrote:
> > > > The devm_clk_get[_optional]_enabled() helpers:
> > > >     - call devm_clk_get[_optional]()
> > > >     - call clk_prepare_enable() and register what is needed in order to
> > > >       call clk_disable_unprepare() when needed, as a managed resource.
> > > > 
> > > > This simplifies the code and avoids the calls to clk_disable_unprepare().
> > > > 
> > > > While at it, use dev_err_probe consistently, and use its return value
> > > > to return the error code.
> > > > 
> > > > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > 
> > > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > This patch is in next now as b05a2e58c16c47f3d752b7db1714ef077e5b82d9.
> > My name occurs twice in the tag area, once it is mangled as
> > 
> > 	Uwe Kleine-K=F6nig
> > 
> > I would welcome fixing that (i.e. s/=F6/ö/). When this commit is
> > touched, you can also do s/Use Use/Use/ in the Subject.
> 
> Fixed.

Looking at the output of

	git range-diff b05a2e58c16c47f3d752b7db1714ef077e5b82d9...9b31b1ea125ca2e734ae89badc0c3073b4445842

it looks good to me now.

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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