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

[Lee Jones <lee.jones@linaro.org>]

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