[PATCH] pwm: atmel: Make use of dev_err_probe()

[PATCH] pwm: atmel: Make use of dev_err_probe()

[Uwe Kleine-König]

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

[PATCH] pwm: atmel: Make use of dev_err_probe()

[Uwe Kleine-König]

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

Re: [PATCH] pwm: atmel: Make use of dev_err_probe()

[Lee Jones]

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

Re: [PATCH] pwm: atmel: Make use of dev_err_probe()

[Lee Jones]

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

Re: [PATCH] pwm: atmel: Make use of dev_err_probe()

[Uwe Kleine-König]

[-- Attachment #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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] pwm: atmel: Make use of dev_err_probe()

[Uwe Kleine-König]

[-- 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

Re: [PATCH] pwm: atmel: Make use of dev_err_probe()

[Alexandre Belloni]

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

Re: [PATCH] pwm: atmel: Make use of dev_err_probe()

[Alexandre Belloni]

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

Re: [PATCH] pwm: atmel: Make use of dev_err_probe()

[Lee Jones]

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

Re: [PATCH] pwm: atmel: Make use of dev_err_probe()

[Lee Jones]

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

Re: [PATCH] pwm: atmel: Make use of dev_err_probe()

[Uwe Kleine-König]

[-- Attachment #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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] pwm: atmel: Make use of dev_err_probe()

[Uwe Kleine-König]

[-- 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

Re: [PATCH] pwm: atmel: Make use of dev_err_probe()

[Alexandre Belloni]

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

Re: [PATCH] pwm: atmel: Make use of dev_err_probe()

[Alexandre Belloni]

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

Re: [PATCH] pwm: atmel: Make use of dev_err_probe()

[Lee Jones]

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

Re: [PATCH] pwm: atmel: Make use of dev_err_probe()

[Lee Jones]

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

Re: [PATCH] pwm: atmel: Make use of dev_err_probe()

[Uwe Kleine-König]

[-- Attachment #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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] pwm: atmel: Make use of dev_err_probe()

[Uwe Kleine-König]

[-- 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