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

[Uwe Kleine-König]

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.
 
Best regards
Uwe

[1] A few posts that I found quickly:
    https://lore.kernel.org/lkml/YH2k5xnD%2F+CKnMBQ@xxxxxxxxxxxxxxxxxxxx/
    https://lore.kernel.org/lkml/CAPDyKFpg1qJD0r54sBC3hCoFey_+gwAL1n2o-aGwnAzAan5p7w@xxxxxxxxxxxxxx/
    https://lore.kernel.org/lkml/CAMpxmJW770v6JLdveEe1hkgNEJByVyArhorSyUZBYOyFiVyOeg@xxxxxxxxxxxxxx/
    https://lore.kernel.org/linux-can/fe0a8a9b-35c6-8f23-5968-0b14abb6078d@xxxxxxxxxxxxxx/
    https://lore.kernel.org/netdev/20190327084422.4209-16-maxime.chevallier@xxxxxxxxxxx/

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature