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

[Lee Jones]

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.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog