Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 28 Apr 2021 19:46:56 +0200
From:   Thierry Reding <thierry.reding@gmail.com>
To:     Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= <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
Message-ID: <YImfkM/ll1nCmopq@orome.fritz.box>
References: <20210427102247.822-1-fenglinw@codeaurora.org>
 <20210427102247.822-3-fenglinw@codeaurora.org>
 <20210427170748.wglupc6zwrndalxs@pengutronix.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="ecWP3/I3yNFzHkoj"
Content-Disposition: inline
In-Reply-To: <20210427170748.wglupc6zwrndalxs@pengutronix.de>
User-Agent: Mutt/2.0.6 (98f8cb83) (2021-03-06)
Precedence: bulk


--ecWP3/I3yNFzHkoj
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 27, 2021 at 07:07:48PM +0200, Uwe Kleine-K=C3=B6nig wrote:
> Hello,
>=20
> 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)
>=20
> 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:
>=20
> 	#define QCOM_PWM_PWM_SIZE_CLK		0x41
> 	#define QCOM_PWM_PWM_SIZE_CLK_FREQ_SEL 		GENMASK(1, 0)
>=20
> 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] =3D {6, 9};
> > +static const int clk_freq_hz[NUM_PWM_CLK] =3D {1024, 32768, 19200000};
> > +static const int clk_prediv[NUM_CLK_PREDIV] =3D {1, 3, 5, 6};
> > +static const int pwm_exponent[NUM_PWM_EXP] =3D {0, 1, 2, 3, 4, 5, 6, 7=
};
>=20
> 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

--ecWP3/I3yNFzHkoj
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmCJn5AACgkQ3SOs138+
s6EanA/+KxtrprYC/29QQIirySzAAgLCoW++tjUw/uvLAB0/mCqC0u0XGHuDf1co
WO2yw8nD9RWkOLdVzLbCiw/NVxjg3O9VDdPq+XMHc6mqwmRApt4i7Sq4zmYex4AR
ldflsnuV6wp5ZJcHrT9UPR6Sd/UGBWzAindxVPvqpCwQJwJ6seHc1U7PaM7dVLdh
7Ee0GYgA5HjmT6Xa/jfn6OBieoJT2VWfN3mdrYCXHCiEnjUjyiRKX50jwKxWiERI
l2+HiKrQ46vumvQqV1+HJrUGxLYZfCGjYGvHc2sd0uWTMxaH9LteNByFFjDPrLl9
QY6Ox95dWp6qys1fIkqPp5NWJyqNB8v6XCTcqx7TlxTe/KmZcywnKmFK+jnHfOWw
aFl7m7ylYQjtsbCl1OnWs48G6dH6LfcReHKCaCxeVywu6+3SfRSfebryC7B3Vfxs
oB0PTlkr7sy3EiPUwfRQa72xn/NFqzPCn9XzwM9oGmOFmFBHyn2olCPqzAcu23cY
6T+YOtD2QYa7W0PYYJ/kjMTXalgoMfLRaC4bhNkaVhwzd3iStBrIcrJJrGeAOAlo
26NH7f2gODb3sGqANRGC7905EV2goOCLVKQh5FstZeFqMiXqqSyBEcYU8QLXYUVt
BK97V+s7RtVIr8wgDV7HqiUlAJmVQvUEj1M0U1TqxaZs5IQJVpo=
=PQGL
-----END PGP SIGNATURE-----

--ecWP3/I3yNFzHkoj--