From: Thierry Reding <thierry.reding@gmail.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Fenglin Wu <fenglinw@codeaurora.org>,
linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
Lee Jones <lee.jones@linaro.org>,
linux-pwm@vger.kernel.org, subbaram@codeaurora.org,
collinsd@codeaurora.org, aghayal@codeaurora.org
Subject: Re: [PATCH 2/2] pwm: pwm-qcom: add driver for PWM modules in QCOM PMICs
Date: Wed, 28 Apr 2021 19:46:56 +0200 [thread overview]
Message-ID: <YImfkM/ll1nCmopq@orome.fritz.box> (raw)
In-Reply-To: <20210427170748.wglupc6zwrndalxs@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 2348 bytes --]
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2021-04-28 17:46 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20210427102247.822-1-fenglinw@codeaurora.org>
2021-04-27 10:22 ` [PATCH 1/2] dt-bindings: pwm: add bindings for PWM modules inside QCOM PMICs Fenglin Wu
2021-04-27 12:57 ` Rob Herring
2021-04-28 10:54 ` fenglinw
2021-04-28 17:38 ` Thierry Reding
2021-04-27 10:22 ` [PATCH 2/2] pwm: pwm-qcom: add driver for PWM modules in " Fenglin Wu
2021-04-27 17:07 ` Uwe Kleine-König
2021-04-28 12:42 ` fenglinw
2021-04-28 15:40 ` Uwe Kleine-König
2021-04-28 17:46 ` Thierry Reding [this message]
2021-04-29 6:52 ` Uwe Kleine-König
2021-04-29 7:06 ` Lee Jones
2021-04-29 10:18 ` Thierry Reding
2021-04-29 11:04 ` Lee Jones
2021-04-29 10:15 ` Thierry Reding
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YImfkM/ll1nCmopq@orome.fritz.box \
--to=thierry.reding@gmail.com \
--cc=aghayal@codeaurora.org \
--cc=collinsd@codeaurora.org \
--cc=fenglinw@codeaurora.org \
--cc=lee.jones@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=subbaram@codeaurora.org \
--cc=u.kleine-koenig@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).