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
