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
