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

[Uwe Kleine-König]

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 at 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/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20200812/8b1b9c58/attachment.sig>