Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 28 Sep 2020 08:45:17 +0200
From:   Thierry Reding <thierry.reding@gmail.com>
To:     Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Cc:     Andy Shevchenko <andriy.shevchenko@intel.com>,
        Rahul Tanwar <rahul.tanwar@linux.intel.com>,
        linux-pwm@vger.kernel.org, lee.jones@linaro.org,
        p.zabel@pengutronix.de, robh+dt@kernel.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        songjun.Wu@intel.com, cheol.yong.kim@intel.com,
        qi-ming.wu@intel.com, rahul.tanwar.linux@gmail.com,
        rtanwar@maxlinear.com
Subject: Re: [PATCH v13 2/2] Add PWM fan controller driver for LGM SoC
Message-ID: <20200928064517.GA2837573@ulmo>
References: <cover.1600158087.git.rahul.tanwar@linux.intel.com>
 <befa655d8beb326fc8aa405a25a8b3e62b7e6a4a.1600158087.git.rahul.tanwar@linux.intel.com>
 <20200924065534.e2anwghhtysv63e7@pengutronix.de>
 <20200924132334.GT3956970@smile.fi.intel.com>
 <20200924141659.4wov7w2l2bllpre4@pengutronix.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="xHFwDpU9dbj6ez1V"
Content-Disposition: inline
In-Reply-To: <20200924141659.4wov7w2l2bllpre4@pengutronix.de>
User-Agent: Mutt/1.14.7 (2020-08-29)
Precedence: bulk


--xHFwDpU9dbj6ez1V
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Sep 24, 2020 at 04:16:59PM +0200, Uwe Kleine-K=C3=B6nig 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=C3=B6nig wrote:
> > > On Tue, Sep 15, 2020 at 04:23:37PM +0800, Rahul Tanwar wrote:
> >=20
> > ...
> >=20
> > > > +	ret =3D lgm_clk_enable(dev, pc);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "failed to enable clock\n");
> > >=20
> > > You used dev_err_probe four times for six error paths. I wonder why y=
ou
> > > didn't use it here (and below for a failing pwmchip_add()).
> >=20
> > dev_err_probe() makes sense when we might experience deferred probe. In=
 neither
> > of mentioned function this can be the case.
> >=20
> > > > +		return ret;
> > > > +	}
> >=20
> > ...
> >=20
> > > > +	ret =3D lgm_reset_control_deassert(dev, pc);
> > > > +	if (ret)
> > > > +		return dev_err_probe(dev, ret, "cannot deassert reset control\n"=
);
> > >=20
> > > 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 appli=
es
> > > 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.)
> >=20
> > True. And above dev_err_probe() is not needed.
>=20
> You argue that dev_err_probe() gives no benefit as
> lgm_reset_control_deassert won't return -EPROBE_DEFER, right?
>=20
> Still I consider it a useful function because
>=20
>  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.
>=20
>  b) With dev_err_probe() I can accomplish things in a single line that
>     need two lines when open coding it.
>=20
>  c) dev_err_probe() emits the symbolic error name without having to
>     resort to %pe + ERR_PTR.
>=20
>  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
=66rom 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

--xHFwDpU9dbj6ez1V
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAl9xhnkACgkQ3SOs138+
s6FL3xAAi8SW4eZyqa3DzH2x8PIYh0nU4XdJiwu3FXjnpeZKFhpAw727Pf1Rwae7
PMFnDggq2Q0GJC8Pzo95H0yBYaaVuQa93TLNszX3ofW0GZoZAadGPtGUjQrfFWcZ
p7DySeukbO3BNIEDSIbMRYSUF+9cPq1sC4RAAQXP27Kdk62nbw2Tsr+AG7YXIBlj
Z5N4zdFTv6LQRIIv08vpNyj74FSAb41wb+MR5vQnZKwlGRLJ0ZpVuW77SjFtj3Q7
XMwrHljmSogGLIeqepX3I4McjzVfMXLWcF1sw6quBSX3zJo5DYybcvN6TUnWXQWe
2AKyE+PGBjwBZcUJg9RM90VvGNjCJ3zm2WUQqLvsYRKC0eGubF9p65b36y6j0IIa
A2gQNJQ5JiqW5q9Jm5I4RwTU0lZ/Mi+TwnKdZvPiPcC+YuRiqdUC5dHfXvWPfeCL
eMFhmd7pRI8H7HkRNPo8z/tVufr+zHbYjtQljv2iCCZLXDfBPioX8jeTh9iXbj8Y
wqR7F5vCC4pAWGLMw4o9jg0Zg5ywPRccXpNh8IbsrNONj0Y7MfXbo5lhxeqck10h
ZnQRB67wK7iRbV23oos+R5Run1fU/PD4TD+m+6noGDHcmiOmX3g/ZxecAU5iB9pg
e+V3/apZhQnJ3zwUssmZUfG501sECh3EqmZdUzDT6yrULhEiLtg=
=Td2r
-----END PGP SIGNATURE-----

--xHFwDpU9dbj6ez1V--