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

[Thierry Reding]

On Thu, Apr 29, 2021 at 08:52:13AM +0200, 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.

I've said this elsewhere, and I specifically didn't comment on that
request, that I think namespacing for static functions does make sense
because they may show up in backtraces.

Register definitions, however, are never going to show up in a
backtrace. The only place where you will ever see them is within the
source file where the context is already plenty clear. The same goes
for locally scoped variables.

>                                   It is sensible because tools like
> ctags, etags and cscope work better when names are unique.

But those tools are primarily useful to find cross-references of
symbols. If you have symbols that are local to a single file, then it's
much easier to open that file in an editor than run the tools.

>                                                            It is
> sensible because it's harder than necessary to spot the error in
> 
> 	writel(PWM_EN_GLITCH_REMOVAL_MASK, base + REG_ENABLE_CONTROL);

I don't think it's sensible to rely on naming to detect errors in this
kind of construct. Either you write it correctly and the code does what
it's supposed to, or it isn't correct and the code will be broken.

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

Yes, sure, if you consider everyone incapable of making reasonable
decisions, then by all means, let's make it as easy as possible. Maybe
while at it you want to go and propagate those rules across the entire
kernel. Good luck with that.

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

Again, functions are an exception where the prefix makes sense.

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

Well, I'm sure I could find plenty of examples of maintainers *not*
requesting common prefixes because they disagree with your opinion.
Maybe you should think about why it's called a "bubble".

This is totally subjective and there aren't any clear rules. As such I
don't think we should enforce it. The one exception that we all seem to
agree on is static functions because they can show up in traces, but for
the rest I'm not going to enforce the common prefix.

Thierry

Attachment: signature.asc
Description: PGP signature