| Message ID | 20230304165653.2179835-1-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9b31b1ea125ca2e734ae89badc0c3073b4445842 |
| Headers | show |
| Series | [1/2] watchdog: s3c2410_wdt: Use Use devm_clk_get[_optional]_enabled() helpers | expand |
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; > } >
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
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
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
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
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
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
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
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.
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
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; }
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(-)