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

[Lee Jones <lee.jones@linaro.org>]

On Thu, 29 Apr 2021, Thierry Reding wrote:

> 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.

I think you were very specific:

 "Again, these are local symbols and there's no need for namespacing. The
  only case where this would need to change is if the symbols started
  conflicting with global ones, but until that happens, let's just keep
  the names short and concise."

:)

> But there's absolutely no reason to enforce it for register
> definitions or local variables because the symbols will never show
> up anywhere.

I personally like namespacing defines too since it makes local ones
easily distinguishable from defines pulled in from API's header
files.

But at the end of the day, it's your train-set.

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