Re: [PATCH 2/2] pwm: pwm-qcom: add driver for PWM modules in QCOM PMICs

[Thierry Reding]

On Thu, Apr 29, 2021 at 08:06:53AM +0100, Lee Jones wrote:
> On Thu, 29 Apr 2021, Uwe Kleine-König wrote:
> 
> > Hello,
> > 
> > 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önig 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 name.
> > > > So something like:
> > > > 
> > > > 	#define QCOM_PWM_PWM_SIZE_CLK		0x41
> > > > 	#define QCOM_PWM_PWM_SIZE_CLK_FREQ_SEL 		GENMASK(1, 0)
> > > > 
> > > > even if the names are quite long, its usage is less error prone. Maybe
> > > > 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 let you
> > > > judge about the details.
> > > 
> > > 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.
> > 
> > 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. 
> > 
> > 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
> > 
> > 	writel(PWM_EN_GLITCH_REMOVAL_MASK, base + REG_ENABLE_CONTROL);
> > 
> > . 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.
> > 
> > > Forcing everyone to use a specific prefix is just going to add a bunch
> > > of characters but doesn't actually add any value.
> > 
> > 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.
> 
> 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

Attachment: signature.asc
Description: PGP signature