| Message ID | 20230304165653.2179835-1-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Accepted |
| 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 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(-)