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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 28 Apr 2021 19:46:56 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Fenglin Wu <fenglinw@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Lee Jones <lee.jones@...aro.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 Tue, Apr 27, 2021 at 07:07:48PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Apr 27, 2021 at 06:22:10PM +0800, Fenglin Wu wrote:
[...]
> > diff --git a/drivers/pwm/pwm-qcom.c b/drivers/pwm/pwm-qcom.c
[...]
> > +#define PWM_FREQ_EXPONENT_MASK		GENMASK(2, 0)
> > +
> > +/* REG_PWM_TYPE_CONFIG */
> > +#define PWM_EN_GLITCH_REMOVAL_MASK	BIT(5)
> > +
> > +/* REG_PWM_VALUE */
> > +#define PWM_VALUE_LSB_MASK		GENMASK(7, 0)
> > +#define PWM_VALUE_MSB_MASK		BIT(0)
> > +
> > +/* REG_ENABLE_CONTROL */
> > +#define EN_MODULE_BIT			BIT(7)
> > +
> > +/* REG_PWM_SYNC */
> > +#define PWM_VALUE_SYNC			BIT(0)
> 
> 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. Forcing everyone to use a specific prefix is
just going to add a bunch of characters but doesn't actually add any
value.

> > +/* constant definitions */
> > +#define REG_SIZE_PER_CHANN		0x100
> > +#define NUM_PWM_SIZE			2
> > +#define NUM_PWM_CLK			3
> > +#define NUM_CLK_PREDIV			4
> > +#define NUM_PWM_EXP			8
> > +
> > +static const int pwm_size[NUM_PWM_SIZE] = {6, 9};
> > +static const int clk_freq_hz[NUM_PWM_CLK] = {1024, 32768, 19200000};
> > +static const int clk_prediv[NUM_CLK_PREDIV] = {1, 3, 5, 6};
> > +static const int pwm_exponent[NUM_PWM_EXP] = {0, 1, 2, 3, 4, 5, 6, 7};
> 
> Please also use a driver specific prefix for variables and function
> names.

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.

Thierry

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