* [PATCH] pwm: atmel: Make use of dev_err_probe() @ 2020-08-12 8:02 Uwe Kleine-König 2020-08-12 8:20 ` Lee Jones 0 siblings, 1 reply; 9+ messages in thread From: Uwe Kleine-König @ 2020-08-12 8:02 UTC (permalink / raw) To: Claudiu Beznea, Thierry Reding, Lee Jones, Nicolas Ferre, Alexandre Belloni, Ludovic Desroches Cc: linux-pwm, kernel, linux-arm-kernel Add an error message for failure points that currently lack a message and convert dev_err to dev_err_probe() which does the right thing for -EPROBE_DEFER. Also slightly simplify the error handling. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/pwm/pwm-atmel.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c index 6161e7e3e9ac..aa0b36695dc7 100644 --- a/drivers/pwm/pwm-atmel.c +++ b/drivers/pwm/pwm-atmel.c @@ -415,17 +415,18 @@ static int atmel_pwm_probe(struct platform_device *pdev) res = platform_get_resource(pdev, IORESOURCE_MEM, 0); atmel_pwm->base = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(atmel_pwm->base)) - return PTR_ERR(atmel_pwm->base); + return dev_err_probe(&pdev->dev, PTR_ERR(atmel_pwm->base), + "Failed to remap register space\n"); atmel_pwm->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(atmel_pwm->clk)) - return PTR_ERR(atmel_pwm->clk); + return dev_err_probe(&pdev->dev, PTR_ERR(atmel_pwm->clk), + "Failed to get clock\n"); ret = clk_prepare(atmel_pwm->clk); - if (ret) { - dev_err(&pdev->dev, "failed to prepare PWM clock\n"); - return ret; - } + if (ret) + return dev_err_probe(&pdev->dev, ret, + "Failed to prepare PWM clock\n"); atmel_pwm->chip.dev = &pdev->dev; atmel_pwm->chip.ops = &atmel_pwm_ops; @@ -436,17 +437,14 @@ static int atmel_pwm_probe(struct platform_device *pdev) ret = pwmchip_add(&atmel_pwm->chip); if (ret < 0) { - dev_err(&pdev->dev, "failed to add PWM chip %d\n", ret); - goto unprepare_clk; + clk_unprepare(atmel_pwm->clk); + return dev_err_probe(&pdev->dev, ret, + "Failed to add PWM chip\n"); } platform_set_drvdata(pdev, atmel_pwm); - return ret; - -unprepare_clk: - clk_unprepare(atmel_pwm->clk); - return ret; + return 0; } static int atmel_pwm_remove(struct platform_device *pdev) -- 2.27.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] pwm: atmel: Make use of dev_err_probe() 2020-08-12 8:02 [PATCH] pwm: atmel: Make use of dev_err_probe() Uwe Kleine-König @ 2020-08-12 8:20 ` Lee Jones 2020-08-12 8:32 ` Uwe Kleine-König 0 siblings, 1 reply; 9+ messages in thread From: Lee Jones @ 2020-08-12 8:20 UTC (permalink / raw) To: Uwe Kleine-König Cc: linux-pwm, Alexandre Belloni, Ludovic Desroches, Thierry Reding, kernel, Claudiu Beznea, linux-arm-kernel On Wed, 12 Aug 2020, Uwe Kleine-König wrote: > Add an error message for failure points that currently lack a message > and convert dev_err to dev_err_probe() which does the right thing for > -EPROBE_DEFER. Also slightly simplify the error handling. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/pwm/pwm-atmel.c | 24 +++++++++++------------- > 1 file changed, 11 insertions(+), 13 deletions(-) > > diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c > index 6161e7e3e9ac..aa0b36695dc7 100644 > --- a/drivers/pwm/pwm-atmel.c > +++ b/drivers/pwm/pwm-atmel.c > @@ -415,17 +415,18 @@ static int atmel_pwm_probe(struct platform_device *pdev) > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > atmel_pwm->base = devm_ioremap_resource(&pdev->dev, res); > if (IS_ERR(atmel_pwm->base)) > - return PTR_ERR(atmel_pwm->base); > + return dev_err_probe(&pdev->dev, PTR_ERR(atmel_pwm->base), > + "Failed to remap register space\n"); This is a regression. devm_ioremap_resource() already emits and error message for !-ENOMEM. -ENOMEM cases should fail silently. > atmel_pwm->clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(atmel_pwm->clk)) > - return PTR_ERR(atmel_pwm->clk); > + return dev_err_probe(&pdev->dev, PTR_ERR(atmel_pwm->clk), > + "Failed to get clock\n"); Isn't dev_err_probe() only useful for drivers handling -EPROBE_DEFER? > ret = clk_prepare(atmel_pwm->clk); > - if (ret) { > - dev_err(&pdev->dev, "failed to prepare PWM clock\n"); > - return ret; > - } > + if (ret) > + return dev_err_probe(&pdev->dev, ret, > + "Failed to prepare PWM clock\n"); As above. > atmel_pwm->chip.dev = &pdev->dev; > atmel_pwm->chip.ops = &atmel_pwm_ops; > @@ -436,17 +437,14 @@ static int atmel_pwm_probe(struct platform_device *pdev) > > ret = pwmchip_add(&atmel_pwm->chip); > if (ret < 0) { > - dev_err(&pdev->dev, "failed to add PWM chip %d\n", ret); > - goto unprepare_clk; > + clk_unprepare(atmel_pwm->clk); > + return dev_err_probe(&pdev->dev, ret, > + "Failed to add PWM chip\n"); > } > > platform_set_drvdata(pdev, atmel_pwm); > > - return ret; > - > -unprepare_clk: > - clk_unprepare(atmel_pwm->clk); > - return ret; > + return 0; > } > > static int atmel_pwm_remove(struct platform_device *pdev) -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] pwm: atmel: Make use of dev_err_probe() 2020-08-12 8:20 ` Lee Jones @ 2020-08-12 8:32 ` Uwe Kleine-König 2020-08-12 8:47 ` Alexandre Belloni 2020-08-12 8:47 ` Lee Jones 0 siblings, 2 replies; 9+ messages in thread From: Uwe Kleine-König @ 2020-08-12 8:32 UTC (permalink / raw) To: Lee Jones Cc: linux-pwm, Alexandre Belloni, Ludovic Desroches, Thierry Reding, kernel, Claudiu Beznea, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 2293 bytes --] On Wed, Aug 12, 2020 at 09:20:02AM +0100, Lee Jones wrote: > On Wed, 12 Aug 2020, Uwe Kleine-König wrote: > > > Add an error message for failure points that currently lack a message > > and convert dev_err to dev_err_probe() which does the right thing for > > -EPROBE_DEFER. Also slightly simplify the error handling. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > drivers/pwm/pwm-atmel.c | 24 +++++++++++------------- > > 1 file changed, 11 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c > > index 6161e7e3e9ac..aa0b36695dc7 100644 > > --- a/drivers/pwm/pwm-atmel.c > > +++ b/drivers/pwm/pwm-atmel.c > > @@ -415,17 +415,18 @@ static int atmel_pwm_probe(struct platform_device *pdev) > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > atmel_pwm->base = devm_ioremap_resource(&pdev->dev, res); > > if (IS_ERR(atmel_pwm->base)) > > - return PTR_ERR(atmel_pwm->base); > > + return dev_err_probe(&pdev->dev, PTR_ERR(atmel_pwm->base), > > + "Failed to remap register space\n"); > > This is a regression. > > devm_ioremap_resource() already emits and error message for !-ENOMEM. > > -ENOMEM cases should fail silently. ah right. Maybe dev_err_probe() should do this right, too? > > atmel_pwm->clk = devm_clk_get(&pdev->dev, NULL); > > if (IS_ERR(atmel_pwm->clk)) > > - return PTR_ERR(atmel_pwm->clk); > > + return dev_err_probe(&pdev->dev, PTR_ERR(atmel_pwm->clk), > > + "Failed to get clock\n"); > > Isn't dev_err_probe() only useful for drivers handling -EPROBE_DEFER? devm_clk_get() might return -EPROBE_DEFER. > > ret = clk_prepare(atmel_pwm->clk); > > - if (ret) { > > - dev_err(&pdev->dev, "failed to prepare PWM clock\n"); > > - return ret; > > - } > > + if (ret) > > + return dev_err_probe(&pdev->dev, ret, > > + "Failed to prepare PWM clock\n"); > > As above. I only checked quickly and didn't find an instance where clk_prepare can return -EPROBE_DEFER, but even if it doesn't it works fine. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] pwm: atmel: Make use of dev_err_probe() 2020-08-12 8:32 ` Uwe Kleine-König @ 2020-08-12 8:47 ` Alexandre Belloni 2020-08-12 9:25 ` Uwe Kleine-König 2020-08-12 8:47 ` Lee Jones 1 sibling, 1 reply; 9+ messages in thread From: Alexandre Belloni @ 2020-08-12 8:47 UTC (permalink / raw) To: Uwe Kleine-König Cc: linux-pwm, Ludovic Desroches, Thierry Reding, kernel, Lee Jones, Claudiu Beznea, linux-arm-kernel On 12/08/2020 10:32:04+0200, Uwe Kleine-König wrote: > On Wed, Aug 12, 2020 at 09:20:02AM +0100, Lee Jones wrote: > > On Wed, 12 Aug 2020, Uwe Kleine-König wrote: > > > > > Add an error message for failure points that currently lack a message > > > and convert dev_err to dev_err_probe() which does the right thing for > > > -EPROBE_DEFER. Also slightly simplify the error handling. > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > --- > > > drivers/pwm/pwm-atmel.c | 24 +++++++++++------------- > > > 1 file changed, 11 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c > > > index 6161e7e3e9ac..aa0b36695dc7 100644 > > > --- a/drivers/pwm/pwm-atmel.c > > > +++ b/drivers/pwm/pwm-atmel.c > > > @@ -415,17 +415,18 @@ static int atmel_pwm_probe(struct platform_device *pdev) > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > atmel_pwm->base = devm_ioremap_resource(&pdev->dev, res); > > > if (IS_ERR(atmel_pwm->base)) > > > - return PTR_ERR(atmel_pwm->base); > > > + return dev_err_probe(&pdev->dev, PTR_ERR(atmel_pwm->base), > > > + "Failed to remap register space\n"); > > > > This is a regression. > > > > devm_ioremap_resource() already emits and error message for !-ENOMEM. > > > > -ENOMEM cases should fail silently. > > ah right. Maybe dev_err_probe() should do this right, too? > > > > atmel_pwm->clk = devm_clk_get(&pdev->dev, NULL); > > > if (IS_ERR(atmel_pwm->clk)) > > > - return PTR_ERR(atmel_pwm->clk); > > > + return dev_err_probe(&pdev->dev, PTR_ERR(atmel_pwm->clk), > > > + "Failed to get clock\n"); > > > > Isn't dev_err_probe() only useful for drivers handling -EPROBE_DEFER? > > devm_clk_get() might return -EPROBE_DEFER. > If it did, you wouldn't be able to print this message. I' not sure it is worth adding so many checks for errors that will never happen. > > > ret = clk_prepare(atmel_pwm->clk); > > > - if (ret) { > > > - dev_err(&pdev->dev, "failed to prepare PWM clock\n"); > > > - return ret; > > > - } > > > + if (ret) > > > + return dev_err_probe(&pdev->dev, ret, > > > + "Failed to prepare PWM clock\n"); > > > > As above. > > I only checked quickly and didn't find an instance where clk_prepare can > return -EPROBE_DEFER, but even if it doesn't it works fine. Same here, clk_prepare will never fail but there is a check and a string that will never be used. In my opinion, this is simply a waste of space... -- Alexandre Belloni, Bootlin Embedded Linux and Kernel engineering https://bootlin.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] pwm: atmel: Make use of dev_err_probe() 2020-08-12 8:47 ` Alexandre Belloni @ 2020-08-12 9:25 ` Uwe Kleine-König 2020-08-12 9:34 ` Alexandre Belloni 2020-08-12 9:36 ` Lee Jones 0 siblings, 2 replies; 9+ messages in thread From: Uwe Kleine-König @ 2020-08-12 9:25 UTC (permalink / raw) To: Alexandre Belloni Cc: linux-pwm, Ludovic Desroches, Thierry Reding, kernel, Lee Jones, Claudiu Beznea, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 1297 bytes --] Hello Alexandre, On Wed, Aug 12, 2020 at 10:47:28AM +0200, Alexandre Belloni wrote: > On 12/08/2020 10:32:04+0200, Uwe Kleine-König wrote: > > On Wed, Aug 12, 2020 at 09:20:02AM +0100, Lee Jones wrote: > > > On Wed, 12 Aug 2020, Uwe Kleine-König wrote: > > > > atmel_pwm->clk = devm_clk_get(&pdev->dev, NULL); > > > > if (IS_ERR(atmel_pwm->clk)) > > > > - return PTR_ERR(atmel_pwm->clk); > > > > + return dev_err_probe(&pdev->dev, PTR_ERR(atmel_pwm->clk), > > > > + "Failed to get clock\n"); > > > > > > Isn't dev_err_probe() only useful for drivers handling -EPROBE_DEFER? > > > > devm_clk_get() might return -EPROBE_DEFER. > > If it did, you wouldn't be able to print this message. Why that? It probably won't make it to the console immediately, but once the clk is available the log buffer should be pushed out, shouldn't it? > I' not sure it is worth adding so many checks for errors that will > never happen. I'm sure this train of thought is unsustainable. And people will copy this code to platforms where this assumption might even be more wrong than on at91. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] pwm: atmel: Make use of dev_err_probe() 2020-08-12 9:25 ` Uwe Kleine-König @ 2020-08-12 9:34 ` Alexandre Belloni 2020-08-12 9:36 ` Lee Jones 1 sibling, 0 replies; 9+ messages in thread From: Alexandre Belloni @ 2020-08-12 9:34 UTC (permalink / raw) To: Uwe Kleine-König Cc: linux-pwm, Ludovic Desroches, Thierry Reding, kernel, Lee Jones, Claudiu Beznea, linux-arm-kernel On 12/08/2020 11:25:39+0200, Uwe Kleine-König wrote: > Hello Alexandre, > > On Wed, Aug 12, 2020 at 10:47:28AM +0200, Alexandre Belloni wrote: > > On 12/08/2020 10:32:04+0200, Uwe Kleine-König wrote: > > > On Wed, Aug 12, 2020 at 09:20:02AM +0100, Lee Jones wrote: > > > > On Wed, 12 Aug 2020, Uwe Kleine-König wrote: > > > > > atmel_pwm->clk = devm_clk_get(&pdev->dev, NULL); > > > > > if (IS_ERR(atmel_pwm->clk)) > > > > > - return PTR_ERR(atmel_pwm->clk); > > > > > + return dev_err_probe(&pdev->dev, PTR_ERR(atmel_pwm->clk), > > > > > + "Failed to get clock\n"); > > > > > > > > Isn't dev_err_probe() only useful for drivers handling -EPROBE_DEFER? > > > > > > devm_clk_get() might return -EPROBE_DEFER. > > > > If it did, you wouldn't be able to print this message. > > Why that? It probably won't make it to the console immediately, but once > the clk is available the log buffer should be pushed out, shouldn't it? > No because that would mean that there is not timer clock and te system would not be booting to that point at all. > > I' not sure it is worth adding so many checks for errors that will > > never happen. > > I'm sure this train of thought is unsustainable. And people will copy > this code to platforms where this assumption might even be more wrong > than on at91. Well my point is that if you keep the error checks, you can remove the strings as they will never be printed. -- Alexandre Belloni, Bootlin Embedded Linux and Kernel engineering https://bootlin.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] pwm: atmel: Make use of dev_err_probe() 2020-08-12 9:25 ` Uwe Kleine-König 2020-08-12 9:34 ` Alexandre Belloni @ 2020-08-12 9:36 ` Lee Jones 1 sibling, 0 replies; 9+ messages in thread From: Lee Jones @ 2020-08-12 9:36 UTC (permalink / raw) To: Uwe Kleine-König Cc: linux-pwm, Alexandre Belloni, Ludovic Desroches, Thierry Reding, kernel, Claudiu Beznea, linux-arm-kernel On Wed, 12 Aug 2020, Uwe Kleine-König wrote: > Hello Alexandre, > > On Wed, Aug 12, 2020 at 10:47:28AM +0200, Alexandre Belloni wrote: > > On 12/08/2020 10:32:04+0200, Uwe Kleine-König wrote: > > > On Wed, Aug 12, 2020 at 09:20:02AM +0100, Lee Jones wrote: > > > > On Wed, 12 Aug 2020, Uwe Kleine-König wrote: > > > > > atmel_pwm->clk = devm_clk_get(&pdev->dev, NULL); > > > > > if (IS_ERR(atmel_pwm->clk)) > > > > > - return PTR_ERR(atmel_pwm->clk); > > > > > + return dev_err_probe(&pdev->dev, PTR_ERR(atmel_pwm->clk), > > > > > + "Failed to get clock\n"); > > > > > > > > Isn't dev_err_probe() only useful for drivers handling -EPROBE_DEFER? > > > > > > devm_clk_get() might return -EPROBE_DEFER. > > > > If it did, you wouldn't be able to print this message. > > Why that? It probably won't make it to the console immediately, but once > the clk is available the log buffer should be pushed out, shouldn't it? > > > I' not sure it is worth adding so many checks for errors that will > > never happen. > > I'm sure this train of thought is unsustainable. And people will copy > this code to platforms where this assumption might even be more wrong > than on at91. This driver was authored (and reviewed) with intricate knowledge of the H/W. The current code is optimal for this device. This patch regresses certain aspects such as binary size and code complexity. Sorry, but it's a NACK from me, and sounds like one from Alexandre too. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] pwm: atmel: Make use of dev_err_probe() 2020-08-12 8:32 ` Uwe Kleine-König 2020-08-12 8:47 ` Alexandre Belloni @ 2020-08-12 8:47 ` Lee Jones 2020-08-12 9:40 ` Uwe Kleine-König 1 sibling, 1 reply; 9+ messages in thread From: Lee Jones @ 2020-08-12 8:47 UTC (permalink / raw) To: Uwe Kleine-König Cc: linux-pwm, Alexandre Belloni, Ludovic Desroches, Thierry Reding, kernel, Claudiu Beznea, linux-arm-kernel On Wed, 12 Aug 2020, Uwe Kleine-König wrote: > On Wed, Aug 12, 2020 at 09:20:02AM +0100, Lee Jones wrote: > > On Wed, 12 Aug 2020, Uwe Kleine-König wrote: > > > > > Add an error message for failure points that currently lack a message > > > and convert dev_err to dev_err_probe() which does the right thing for > > > -EPROBE_DEFER. Also slightly simplify the error handling. > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > --- > > > drivers/pwm/pwm-atmel.c | 24 +++++++++++------------- > > > 1 file changed, 11 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c > > > index 6161e7e3e9ac..aa0b36695dc7 100644 > > > --- a/drivers/pwm/pwm-atmel.c > > > +++ b/drivers/pwm/pwm-atmel.c > > > @@ -415,17 +415,18 @@ static int atmel_pwm_probe(struct platform_device *pdev) > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > atmel_pwm->base = devm_ioremap_resource(&pdev->dev, res); > > > if (IS_ERR(atmel_pwm->base)) > > > - return PTR_ERR(atmel_pwm->base); > > > + return dev_err_probe(&pdev->dev, PTR_ERR(atmel_pwm->base), > > > + "Failed to remap register space\n"); > > > > This is a regression. > > > > devm_ioremap_resource() already emits and error message for !-ENOMEM. > > > > -ENOMEM cases should fail silently. > > ah right. Maybe dev_err_probe() should do this right, too? Maybe, but you're still adding an unnecessary string to the kernel's binary. There was a bit push a little while back to cut down on these. > > > atmel_pwm->clk = devm_clk_get(&pdev->dev, NULL); > > > if (IS_ERR(atmel_pwm->clk)) > > > - return PTR_ERR(atmel_pwm->clk); > > > + return dev_err_probe(&pdev->dev, PTR_ERR(atmel_pwm->clk), > > > + "Failed to get clock\n"); > > > > Isn't dev_err_probe() only useful for drivers handling -EPROBE_DEFER? > > devm_clk_get() might return -EPROBE_DEFER. Perhaps, but the author has chosen not to handle it specifically. It's my understanding that dev_err_probe() was designed to simplify error handling in .probe() whereas this patch seems to do the polar opposite. > > > ret = clk_prepare(atmel_pwm->clk); > > > - if (ret) { > > > - dev_err(&pdev->dev, "failed to prepare PWM clock\n"); > > > - return ret; > > > - } > > > + if (ret) > > > + return dev_err_probe(&pdev->dev, ret, > > > + "Failed to prepare PWM clock\n"); > > > > As above. > > I only checked quickly and didn't find an instance where clk_prepare can > return -EPROBE_DEFER, but even if it doesn't it works fine. It's still misleading IMHO. dev_err_probe()'s header details its reason for existence. This seems to be a square peg in a round hole scenario. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] pwm: atmel: Make use of dev_err_probe() 2020-08-12 8:47 ` Lee Jones @ 2020-08-12 9:40 ` Uwe Kleine-König 0 siblings, 0 replies; 9+ messages in thread From: Uwe Kleine-König @ 2020-08-12 9:40 UTC (permalink / raw) To: Lee Jones Cc: linux-pwm, Alexandre Belloni, Ludovic Desroches, Thierry Reding, kernel, Claudiu Beznea, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 4023 bytes --] Hello Lee, On Wed, Aug 12, 2020 at 09:47:51AM +0100, Lee Jones wrote: > On Wed, 12 Aug 2020, Uwe Kleine-König wrote: > > On Wed, Aug 12, 2020 at 09:20:02AM +0100, Lee Jones wrote: > > > On Wed, 12 Aug 2020, Uwe Kleine-König wrote: > > > > > > > Add an error message for failure points that currently lack a message > > > > and convert dev_err to dev_err_probe() which does the right thing for > > > > -EPROBE_DEFER. Also slightly simplify the error handling. > > > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > --- > > > > drivers/pwm/pwm-atmel.c | 24 +++++++++++------------- > > > > 1 file changed, 11 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c > > > > index 6161e7e3e9ac..aa0b36695dc7 100644 > > > > --- a/drivers/pwm/pwm-atmel.c > > > > +++ b/drivers/pwm/pwm-atmel.c > > > > @@ -415,17 +415,18 @@ static int atmel_pwm_probe(struct platform_device *pdev) > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > atmel_pwm->base = devm_ioremap_resource(&pdev->dev, res); > > > > if (IS_ERR(atmel_pwm->base)) > > > > - return PTR_ERR(atmel_pwm->base); > > > > + return dev_err_probe(&pdev->dev, PTR_ERR(atmel_pwm->base), > > > > + "Failed to remap register space\n"); > > > > > > This is a regression. > > > > > > devm_ioremap_resource() already emits and error message for !-ENOMEM. > > > > > > -ENOMEM cases should fail silently. > > > > ah right. Maybe dev_err_probe() should do this right, too? > > Maybe, but you're still adding an unnecessary string to the kernel's > binary. There was a bit push a little while back to cut down on > these. Note I agreed to dropping the error message here. Letting dev_err_probe() ignore -ENOMEM is an orthogonal thing. > > > > atmel_pwm->clk = devm_clk_get(&pdev->dev, NULL); > > > > if (IS_ERR(atmel_pwm->clk)) > > > > - return PTR_ERR(atmel_pwm->clk); > > > > + return dev_err_probe(&pdev->dev, PTR_ERR(atmel_pwm->clk), > > > > + "Failed to get clock\n"); > > > > > > Isn't dev_err_probe() only useful for drivers handling -EPROBE_DEFER? > > > > devm_clk_get() might return -EPROBE_DEFER. > > Perhaps, but the author has chosen not to handle it specifically. Perhaps, in my book having a probe call fail without an error message is always annoying. So the main advantage of this commit is not the use of dev_err_probe(), but that error paths yield some output with an indication about the reason. > It's my understanding that dev_err_probe() was designed to simplify > error handling in .probe() whereas this patch seems to do the polar > opposite. Yes, it doesn't get simpler, but it gains a feature and for that uses dev_err_probe() because open coding it would be still more complex. > > > > ret = clk_prepare(atmel_pwm->clk); > > > > - if (ret) { > > > > - dev_err(&pdev->dev, "failed to prepare PWM clock\n"); > > > > - return ret; > > > > - } > > > > + if (ret) > > > > + return dev_err_probe(&pdev->dev, ret, > > > > + "Failed to prepare PWM clock\n"); > > > > > > As above. > > > > I only checked quickly and didn't find an instance where clk_prepare can > > return -EPROBE_DEFER, but even if it doesn't it works fine. > > It's still misleading IMHO. dev_err_probe()'s header details its > reason for existence. This seems to be a square peg in a round hole > scenario. clk_prepare might not benefit from the EPROBE_DEFER logic in dev_err_probe(), that's true. But it replaces two statements by a single one and adds the error code to the error message. So the driver benefits only for two reasons from the conversion to dev_err_probe() while others might have three. IMHO still good enough to justify the patch. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-08-12 9:42 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-08-12 8:02 [PATCH] pwm: atmel: Make use of dev_err_probe() Uwe Kleine-König 2020-08-12 8:20 ` Lee Jones 2020-08-12 8:32 ` Uwe Kleine-König 2020-08-12 8:47 ` Alexandre Belloni 2020-08-12 9:25 ` Uwe Kleine-König 2020-08-12 9:34 ` Alexandre Belloni 2020-08-12 9:36 ` Lee Jones 2020-08-12 8:47 ` Lee Jones 2020-08-12 9:40 ` Uwe Kleine-König
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).