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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 29 Apr 2021 12:18:37 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        Fenglin Wu <fenglinw@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-pwm@...r.kernel.org, subbaram@...eaurora.org,
        collinsd@...eaurora.org, aghayal@...eaurora.org
Subject: Re: [PATCH 2/2] pwm: pwm-qcom: add driver for PWM modules in QCOM
 PMICs

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. But there's absolutely no reason
to enforce it for register definitions or local variables because the
symbols will never show up anywhere.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)