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

[Thierry Reding]

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

Attachment: signature.asc
Description: PGP signature