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:   Thu, 29 Apr 2021 12:18:37 +0200
From:   Thierry Reding <thierry.reding@gmail.com>
To:     Lee Jones <lee.jones@linaro.org>
Cc:     Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= 
        <u.kleine-koenig@pengutronix.de>,
        Fenglin Wu <fenglinw@codeaurora.org>,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-pwm@vger.kernel.org, subbaram@codeaurora.org,
        collinsd@codeaurora.org, aghayal@codeaurora.org
Subject: Re: [PATCH 2/2] pwm: pwm-qcom: add driver for PWM modules in QCOM
 PMICs
Message-ID: <YIqH/eimua/gwDW8@orome.fritz.box>
References: <20210427102247.822-1-fenglinw@codeaurora.org>
 <20210427102247.822-3-fenglinw@codeaurora.org>
 <20210427170748.wglupc6zwrndalxs@pengutronix.de>
 <YImfkM/ll1nCmopq@orome.fritz.box>
 <20210429065213.inajpznvfxa2xsld@pengutronix.de>
 <20210429070653.GJ6446@dell>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="J6I+xEPlKn2g3FMY"
Content-Disposition: inline
In-Reply-To: <20210429070653.GJ6446@dell>
User-Agent: Mutt/2.0.6 (98f8cb83) (2021-03-06)
Precedence: bulk


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

On Thu, Apr 29, 2021 at 08:06:53AM +0100, Lee Jones wrote:
> On Thu, 29 Apr 2021, Uwe Kleine-K=C3=B6nig wrote:
>=20
> > Hello,
> >=20
> > On Wed, Apr 28, 2021 at 07:46:56PM +0200, Thierry Reding wrote:
> > > On Tue, Apr 27, 2021 at 07:07:48PM +0200, Uwe Kleine-K=C3=B6nig wrote:
> > > > I would like to see the register definition to use a common prefix =
(like
> > > > QCOM_PWM_) and that the names of bit fields include the register na=
me.
> > > > So something like:
> > > >=20
> > > > 	#define QCOM_PWM_PWM_SIZE_CLK		0x41
> > > > 	#define QCOM_PWM_PWM_SIZE_CLK_FREQ_SEL 		GENMASK(1, 0)
> > > >=20
> > > > even if the names are quite long, its usage is less error prone. Ma=
ybe
> > > > it makes sense to drop the duplicated PWM (but only if all or no
> > > > register contains PWM in its name according to the reference manual=
).
> > > > Also maybe QCOM_PWM_PWMSIZECLK_FREQSEL might be a good choice. I le=
t you
> > > > judge about the details.
> > >=20
> > > Please stop requesting this. A common prefix is good for namespacing
> > > symbols, but these defines are used only within this file, so there's=
 no
> > > need to namespace them.
> >=20
> > I do consider it important. The goal of my review comments is to improve
> > the drivers according to what I consider sensible even if that might not
> > fit your metrics.=20
> >=20
> > Consistent name(space)ing is sensible because the names of static
> > functions are used in backtraces. It is sensible because tools like
> > ctags, etags and cscope work better when names are unique. It is
> > sensible because it's harder than necessary to spot the error in
> >=20
> > 	writel(PWM_EN_GLITCH_REMOVAL_MASK, base + REG_ENABLE_CONTROL);
> >=20
> > . It is sensible because the rule "Use namespacing for all symbols" is
> > easier than "Use namespacing for symbols that might conflict with
> > (present or future) names in the core or that might appear in user
> > visible messages like backtraces or KASAN reports". It's sensible
> > because then it's obvious when reading a code line that the symbol is
> > driver specific. It is useful to have a common prefix for driver
> > functions because that makes it easier to select them for tracing.
> >=20
> > > Forcing everyone to use a specific prefix is just going to add a bunch
> > > of characters but doesn't actually add any value.
> >=20
> > That's your opinion and I disagree. I do see a value and the "burden" of
> > these additional characters is quite worth its costs. In my bubble most
> > people also see this value. This includes the coworkers I talked to,
> > several other maintainers also insist on common prefixes[1] and it
> > matches what my software engineering professor taught me during my
> > studies. I also agree that longer names are more annoying than short
> > ones, but that doesn't outweigh the advantages in my eyes and a good
> > editor helps here.
>=20
> FWIW, I'm +1 for proper namespacing for the purposes of; tracing,
> logging and future proofing, even if it does add a few more chars.
> Less of a problem now the 80-char rule is waning.

I've mentioned this in other threads before, but in retrospect I suppose
I could've been more specific. For function names, even static ones,
yes, I agree a common prefix is better. But there's absolutely no reason
to enforce it for register definitions or local variables because the
symbols will never show up anywhere.

Thierry

--J6I+xEPlKn2g3FMY
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmCKh/0ACgkQ3SOs138+
s6Gd4Q//Tx9VahXAUfB9cMzMbyK+8akZj8xJgwRhL5iCR9XHAiacw67IXUcIUK4H
jSN1zuwjMFQc5K5qWZ1f8U/E7tsQOD/xxYJqDnsMtOxrCDO2/pitCGOcvQOSvKp2
fR9aQGI2Yj23uwOMZvgKF5dn0qrmDOIrrFvE2evZCncQIcHF8wf4i4VYBrebMlew
9pZmFAe+GTWvc/Pyto3g2oDw9cakRcTixuURAs7Es+HyvASMtUUgtT28AVkoMlNi
s/rF54/U4oJ6XBSbJMv+3FoXaLf6uhmp9ciH70piEW62eD9M9Ar50l+JfzokI04J
kC4jozCaxgvLLRmwiHcHMLlOC7joWJab9XhOxZX1nGemuoKWSKgnVJnHnKaxGwCh
bAh1jx4dDFwhpj+v/pCH3XHUcatpmfzPo9KTuIM+KmqecTGjYcKRDO0rwSmv4cLG
+98Sxpl5w6AlcpyZXXSc3F87K0qnpTjq+AA3lQ8g2dhjreCgxKrQcS4mOpfgi8k8
5NDoGv5wWI624GfUhos0fOqzoORNqHpKKvtrvEV/kA6PPa7LUJp954LDRcm+7hW2
g4QeV/BqjKyU8kM9MFGJMsKxiFetn6slzXgk3pUjAQxy3D9LEpmfL19El5wJcCrs
wvUKt1Kxbx/JD+oaHwggZmCvSbTja5mmg1xQP8wM0wNMzXxar5k=
=HT+U
-----END PGP SIGNATURE-----

--J6I+xEPlKn2g3FMY--