Re: [PATCH v13 2/2] Add PWM fan controller driver for LGM SoC

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3792 bytes --]

On Thu, Sep 24, 2020 at 04:16:59PM +0200, Uwe Kleine-König wrote:
> On Thu, Sep 24, 2020 at 04:23:34PM +0300, Andy Shevchenko wrote:
> > On Thu, Sep 24, 2020 at 08:55:34AM +0200, Uwe Kleine-König wrote:
> > > On Tue, Sep 15, 2020 at 04:23:37PM +0800, Rahul Tanwar wrote:
> > 
> > ...
> > 
> > > > +	ret = lgm_clk_enable(dev, pc);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "failed to enable clock\n");
> > > 
> > > You used dev_err_probe four times for six error paths. I wonder why you
> > > didn't use it here (and below for a failing pwmchip_add()).
> > 
> > dev_err_probe() makes sense when we might experience deferred probe. In neither
> > of mentioned function this can be the case.
> > 
> > > > +		return ret;
> > > > +	}
> > 
> > ...
> > 
> > > > +	ret = lgm_reset_control_deassert(dev, pc);
> > > > +	if (ret)
> > > > +		return dev_err_probe(dev, ret, "cannot deassert reset control\n");
> > > 
> > > After lgm_reset_control_deassert is called pc->rst is unused. So there
> > > is no need to have this member in struct lgm_pwm_chip. The same applies
> > > to ->clk. (You have to pass rst (or clk) to devm_add_action_or_reset for
> > > that to work. Looks like a nice idea anyhow.)
> > 
> > True. And above dev_err_probe() is not needed.
> 
> You argue that dev_err_probe() gives no benefit as
> lgm_reset_control_deassert won't return -EPROBE_DEFER, right?
> 
> Still I consider it a useful function because
> 
>  a) I (as an author or as a reviewer) don't need to think if the
>     failing function might return -EPROBE_DEFER now or in the future.
>     dev_err_probe does the right thing even for functions that don't
>     return -EPROBE_DEFER.
> 
>  b) With dev_err_probe() I can accomplish things in a single line that
>     need two lines when open coding it.
> 
>  c) dev_err_probe() emits the symbolic error name without having to
>     resort to %pe + ERR_PTR.
> 
>  d) Using dev_err_probe() for all error paths gives a consistency that I
>     like with a maintainer's hat on.

That would perhaps be true if all error paths did use dev_err_probe().
And even if that were the case, dev_err_probe() doesn't guarantee that
error messages will actually be consistent because developers can still
provide whatever format string they like.

Also, the format of the messages that dev_err_probe() prints is unlike
anything that I've seen, so introducing dev_err_probe() actually makes
things more inconsistent, in my opinion.

I have in fact been advocating for people to use error messages of the
form:

	"failed to ...: %d\n", err

or:

	"unable to ...: %d\n", err

Or some other similar form because that's the most common type that I
have come across in the kernel. I think it's also easier to read those
error messages because they contain the important data (i.e. the
description, which tells you what went wrong) first and then are
followed by the error code (which tells you how it failed).

Now I suspect the current format was chosen because we need to have the
constant part first, because otherwise the arbitrary format string could
be something that doesn't lend itself to have an error code appended.

The current format is arguably also something that's easier to parse
from some script because the format is in a somewhat standard format. On
the other hand, I think this is a bit misguided because we already have
structured log messages, so I wonder if it might have been better to
make the error code part of structured log messages to make them truly
machine readable but leave the formatting up to developers so that they
can use whatever is consistent within the driver or whatever fits best
without actually adding a standard string to the log messages.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]