From: Jonathan Cameron <jic23@kernel.org>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>,
Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
Rob Herring <robh+dt@kernel.org>,
Dmitry Torokhov <dmitry.torokhov@gmail.com>,
Lee Jones <lee.jones@linaro.org>,
bcousson@baylibre.com, Tony Lindgren <tony@atomide.com>,
linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
linux-input@vger.kernel.org, linux-omap@vger.kernel.org,
Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
Vignesh Raghavendra <vigneshr@ti.com>,
Lokesh Vutla <lokeshvutla@ti.com>,
Tero Kristo <kristo@kernel.org>,
Ryan Barnett <ryan.barnett@collins.com>,
Grygorii Strashko <grygorii.strashko@ti.com>,
Jason Reeder <jreeder@ti.com>,
Jonathan Cameron <Jonathan.Cameron@huawei.com>
Subject: Re: [PATCH v2 26/46] mfd: ti_am335x_tscadc: Use FIELD_PREP() when relevant in the header
Date: Sun, 19 Sep 2021 16:49:33 +0100 [thread overview]
Message-ID: <20210919164933.77d9a097@jic23-huawei> (raw)
In-Reply-To: <20210915094847.6bb5e8d1@xps13>
On Wed, 15 Sep 2021 09:48:47 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> Hi Jonathan,
>
> jic23@kernel.org wrote on Sun, 5 Sep 2021 14:13:32 +0100:
>
> > On Thu, 2 Sep 2021 23:51:24 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > > Use the FIELD_PREP() macro when relevant. Sometimes reorder the lines to
> > > be able to use the relevant bitmask.
> > >
> > > Mind the s/%d/%ld/ change in a log due to the type change following the
> > > use of FIELD_PREP() in the header.
> > >
> > > Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > What you have here is fine, but I would take it a step further and push the
> > FIELD_PREP() calls directly inline in the code. There is little advantage
> > beyond saving a few characters in
> >
> > STEPENB(3)
> >
> > rather than
> >
> > FIELD_PREP(STEPENB_MASK, 3)
> >
> > If you don't want to go that far this is still an improvement so
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > > ---
> > > drivers/iio/adc/ti_am335x_adc.c | 2 +-
> > > include/linux/mfd/ti_am335x_tscadc.h | 31 ++++++++++++++--------------
> > > 2 files changed, 17 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> > > index e946903b0993..54c410b6ca07 100644
> > > --- a/drivers/iio/adc/ti_am335x_adc.c
> > > +++ b/drivers/iio/adc/ti_am335x_adc.c
> > > @@ -126,7 +126,7 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
> > > chan = adc_dev->channel_line[i];
> > >
> > > if (adc_dev->step_avg[i] > STEPCONFIG_AVG_16) {
> > > - dev_warn(dev, "chan %d step_avg truncating to %d\n",
> > > + dev_warn(dev, "chan %d step_avg truncating to %ld\n",
> > > chan, STEPCONFIG_AVG_16);
> > > adc_dev->step_avg[i] = STEPCONFIG_AVG_16;
> > > }
> > > diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> > > index 4de16fc3d74f..29095c0dc6d8 100644
> > > --- a/include/linux/mfd/ti_am335x_tscadc.h
> > > +++ b/include/linux/mfd/ti_am335x_tscadc.h
> > > @@ -8,6 +8,7 @@
> > > #ifndef __LINUX_TI_AM335X_TSCADC_MFD_H
> > > #define __LINUX_TI_AM335X_TSCADC_MFD_H
> > >
> > > +#include <linux/bitfield.h>
> > > #include <linux/mfd/core.h>
> > >
> > > #define REG_RAWIRQSTATUS 0x024
> > > @@ -40,7 +41,7 @@
> > >
> > > /* Step Enable */
> > > #define STEPENB_MASK GENMASK(16, 0)
> > > -#define STEPENB(val) ((val) << 0)
> > > +#define STEPENB(val) FIELD_PREP(STEPENB_MASK, (val))
> >
> > My inclination with these is to drop the macro entirely and
> > just use FIELD_PREP(STEPENB_MASK, xxx) inline directly.
> > There is no loss of clarity in doing so and it gets you to a state
> > where you only have one macro per field which is nice for checking
> > purposes.
>
> Actually I don't like too much leaking FIELD_PREP()s directly in the
> code because (and this is really a personal feeling) I found them
> rather less straightforward to understand than a good old BIT() for
> example. Moreover, I've already hacked in a lot of different
> places and moving these definitions in the code directly would
> produce a big diff, overall hardening possible future git-blames.
>
> However, I rather agree on the fact that there is no point in defining
> a mask and then using that mask in another macro unless that mask is
> reused somewhere else. So if it's fine for you, I'll provide a new
> version with a reduced number of defines in this file. Would this work
> for you?
That will be fine.
Thanks,
Jonathan
>
> >
> > > #define ENB(val) BIT(val)
> > > #define STPENB_STEPENB STEPENB(GENMASK(16, 0))
> > > #define STPENB_STEPENB_TC STEPENB(GENMASK(12, 0))
> > > @@ -58,11 +59,11 @@
> > >
> > > /* Step Configuration */
> > > #define STEPCONFIG_MODE_MASK GENMASK(1, 0)
> > > -#define STEPCONFIG_MODE(val) ((val) << 0)
> > > +#define STEPCONFIG_MODE(val) FIELD_PREP(STEPCONFIG_MODE_MASK, (val))
> > > #define STEPCONFIG_MODE_SWCNT STEPCONFIG_MODE(1)
> > > #define STEPCONFIG_MODE_HWSYNC STEPCONFIG_MODE(2)
> > > #define STEPCONFIG_AVG_MASK GENMASK(4, 2)
> > > -#define STEPCONFIG_AVG(val) ((val) << 2)
> > > +#define STEPCONFIG_AVG(val) FIELD_PREP(STEPCONFIG_AVG_MASK, (val))
> > > #define STEPCONFIG_AVG_16 STEPCONFIG_AVG(4)
> > > #define STEPCONFIG_XPP BIT(5)
> > > #define STEPCONFIG_XNN BIT(6)
> > > @@ -70,43 +71,43 @@
> > > #define STEPCONFIG_YNN BIT(8)
> > > #define STEPCONFIG_XNP BIT(9)
> > > #define STEPCONFIG_YPN BIT(10)
> > > -#define STEPCONFIG_RFP(val) ((val) << 12)
> > > #define STEPCONFIG_RFP_VREFP GENMASK(13, 12)
> > > +#define STEPCONFIG_RFP(val) FIELD_PREP(STEPCONFIG_RFP_VREFP, (val))
> > > #define STEPCONFIG_INM_MASK GENMASK(18, 15)
> > > -#define STEPCONFIG_INM(val) ((val) << 15)
> > > +#define STEPCONFIG_INM(val) FIELD_PREP(STEPCONFIG_INM_MASK, (val))
> > > #define STEPCONFIG_INM_ADCREFM STEPCONFIG_INM(8)
> > > #define STEPCONFIG_INP_MASK GENMASK(22, 19)
> > > -#define STEPCONFIG_INP(val) ((val) << 19)
> > > +#define STEPCONFIG_INP(val) FIELD_PREP(STEPCONFIG_INP_MASK, (val))
> > > #define STEPCONFIG_INP_AN4 STEPCONFIG_INP(4)
> > > #define STEPCONFIG_INP_ADCREFM STEPCONFIG_INP(8)
> > > #define STEPCONFIG_FIFO1 BIT(26)
> > > -#define STEPCONFIG_RFM(val) ((val) << 23)
> > > #define STEPCONFIG_RFM_VREFN GENMASK(24, 23)
> > > +#define STEPCONFIG_RFM(val) FIELD_PREP(STEPCONFIG_RFM_VREFN, (val))
> > >
> > > /* Delay register */
> > > #define STEPDELAY_OPEN_MASK GENMASK(17, 0)
> > > -#define STEPDELAY_OPEN(val) ((val) << 0)
> > > +#define STEPDELAY_OPEN(val) FIELD_PREP(STEPDELAY_OPEN_MASK, (val))
> > > #define STEPCONFIG_OPENDLY STEPDELAY_OPEN(0x098)
> > > #define STEPDELAY_SAMPLE_MASK GENMASK(31, 24)
> > > -#define STEPDELAY_SAMPLE(val) ((val) << 24)
> > > +#define STEPDELAY_SAMPLE(val) FIELD_PREP(STEPDELAY_SAMPLE_MASK, (val))
> > > #define STEPCONFIG_SAMPLEDLY STEPDELAY_SAMPLE(0)
> > >
> > > /* Charge Config */
> > > #define STEPCHARGE_RFP_MASK GENMASK(14, 12)
> > > -#define STEPCHARGE_RFP(val) ((val) << 12)
> > > +#define STEPCHARGE_RFP(val) FIELD_PREP(STEPCHARGE_RFP_MASK, (val))
> > > #define STEPCHARGE_RFP_XPUL STEPCHARGE_RFP(1)
> > > #define STEPCHARGE_INM_MASK GENMASK(18, 15)
> > > -#define STEPCHARGE_INM(val) ((val) << 15)
> > > +#define STEPCHARGE_INM(val) FIELD_PREP(STEPCHARGE_INM_MASK, (val))
> > > #define STEPCHARGE_INM_AN1 STEPCHARGE_INM(1)
> > > #define STEPCHARGE_INP_MASK GENMASK(22, 19)
> > > -#define STEPCHARGE_INP(val) ((val) << 19)
> > > +#define STEPCHARGE_INP(val) FIELD_PREP(STEPCHARGE_INP_MASK, (val))
> > > #define STEPCHARGE_RFM_MASK GENMASK(24, 23)
> > > -#define STEPCHARGE_RFM(val) ((val) << 23)
> > > +#define STEPCHARGE_RFM(val) FIELD_PREP(STEPCHARGE_RFM_MASK, (val))
> > > #define STEPCHARGE_RFM_XNUR STEPCHARGE_RFM(1)
> > >
> > > /* Charge delay */
> > > #define CHARGEDLY_OPEN_MASK GENMASK(17, 0)
> > > -#define CHARGEDLY_OPEN(val) ((val) << 0)
> > > +#define CHARGEDLY_OPEN(val) FIELD_PREP(CHARGEDLY_OPEN_MASK, (val))
> > > #define CHARGEDLY_OPENDLY CHARGEDLY_OPEN(0x400)
> > >
> > > /* Control register */
> > > @@ -115,7 +116,7 @@
> > > #define CNTRLREG_STEPCONFIGWRT BIT(2)
> > > #define CNTRLREG_POWERDOWN BIT(4)
> > > #define CNTRLREG_AFE_CTRL_MASK GENMASK(6, 5)
> > > -#define CNTRLREG_AFE_CTRL(val) ((val) << 5)
> > > +#define CNTRLREG_AFE_CTRL(val) FIELD_PREP(CNTRLREG_AFE_CTRL_MASK, (val))
> > > #define CNTRLREG_4WIRE CNTRLREG_AFE_CTRL(1)
> > > #define CNTRLREG_5WIRE CNTRLREG_AFE_CTRL(2)
> > > #define CNTRLREG_8WIRE CNTRLREG_AFE_CTRL(3)
> >
>
> Thanks,
> Miquèl
next prev parent reply other threads:[~2021-09-19 15:46 UTC|newest]
Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-02 21:50 [PATCH v2 00/46] TI AM437X ADC1 Miquel Raynal
2021-09-02 21:50 ` [PATCH v2 01/46] clk: ti: am43xx: Add clkctrl data for am43xx ADC1 Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 02/46] dt-bindings: mfd: ti,am3359-tscadc: Add a yaml description for this MFD Miquel Raynal
2021-09-03 16:05 ` Rob Herring
2021-09-05 11:57 ` Jonathan Cameron
2021-09-02 21:51 ` [PATCH v2 03/46] dt-bindings: touchscreen: ti,am3359-tsc: New yaml description Miquel Raynal
2021-09-03 16:06 ` Rob Herring
2021-09-05 11:59 ` Jonathan Cameron
2021-09-02 21:51 ` [PATCH v2 04/46] dt-bindings: iio: adc: ti,am3359-adc: " Miquel Raynal
2021-09-03 16:07 ` Rob Herring
2021-09-02 21:51 ` [PATCH v2 05/46] dt-bindings: touchscreen: ti,am3359-tsc: Remove deprecated text file Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 06/46] dt-bindings: mfd: ti,am3359-tscadc: Describe am4372 MFD compatible Miquel Raynal
2021-09-05 12:01 ` Jonathan Cameron
2021-09-07 19:04 ` Rob Herring
2021-09-02 21:51 ` [PATCH v2 07/46] dt-bindings: iio: adc: ti,am3359-adc: Describe am4372 ADC compatible Miquel Raynal
2021-09-07 19:05 ` Rob Herring
2021-09-02 21:51 ` [PATCH v2 08/46] mfd: ti_am335x_tscadc: Replace license text with SPDX tag Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 09/46] mfd: ti_am335x_tscadc: Fix style Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 10/46] mfd: ti_am335x_tscadc: Drop extra spacing when declaring stack variables Miquel Raynal
2021-09-05 12:03 ` Jonathan Cameron
2021-09-02 21:51 ` [PATCH v2 11/46] mfd: ti_am335x_tscadc: Ensure a balanced number of node get/put Miquel Raynal
2021-09-05 12:06 ` Jonathan Cameron
2021-09-02 21:51 ` [PATCH v2 12/46] mfd: ti_am335x_tscadc: Get rid of useless gotos Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 13/46] mfd: ti_am335x_tscadc: Reword the comment explaining the dividers Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 14/46] mfd: ti_am335x_tscadc: Don't search the tree for our clock Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 15/46] mfd: ti_am335x_tscadc: Simplify divisor calculation Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 16/46] mfd: ti_am335x_tscadc: Move the driver structure allocation earlier Miquel Raynal
2021-09-05 12:09 ` Jonathan Cameron
2021-09-02 21:51 ` [PATCH v2 17/46] mfd: ti_am335x_tscadc: Use driver data Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 18/46] mfd: ti_am335x_tscadc: Mimic the probe from resume() Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 19/46] mfd: ti_am335x_tscadc: Drop useless variables from the driver structure Miquel Raynal
2021-09-05 12:11 ` Jonathan Cameron
2021-09-02 21:51 ` [PATCH v2 20/46] mfd: ti_am335x_tscadc: Always provide an idle configuration Miquel Raynal
2021-09-05 12:13 ` Jonathan Cameron
2021-09-02 21:51 ` [PATCH v2 21/46] mfd: ti_am335x_tscadc: Reorder the initialization steps Miquel Raynal
2021-09-05 12:18 ` Jonathan Cameron
2021-09-02 21:51 ` [PATCH v2 22/46] mfd: ti_am335x_tscadc: Gather the ctrl register logic in one place Miquel Raynal
2021-09-05 12:20 ` Jonathan Cameron
2021-09-02 21:51 ` [PATCH v2 23/46] mfd: ti_am335x_tscadc: Replace the header license text with SPDX tag Miquel Raynal
2021-09-05 12:21 ` Jonathan Cameron
2021-09-02 21:51 ` [PATCH v2 24/46] mfd: ti_am335x_tscadc: Fix header spacing Miquel Raynal
2021-09-05 12:22 ` Jonathan Cameron
2021-09-02 21:51 ` [PATCH v2 25/46] mfd: ti_am335x_tscadc: Use BIT() and GENMASK() when relevant in the header Miquel Raynal
2021-09-05 13:09 ` Jonathan Cameron
2021-09-02 21:51 ` [PATCH v2 26/46] mfd: ti_am335x_tscadc: Use FIELD_PREP() " Miquel Raynal
2021-09-05 13:13 ` Jonathan Cameron
2021-09-15 7:48 ` Miquel Raynal
2021-09-19 15:49 ` Jonathan Cameron [this message]
2021-09-02 21:51 ` [PATCH v2 27/46] mfd: ti_am335x_tscadc: Rename the subsystem enable macro Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 28/46] mfd: ti_am335x_tscadc: Add TSC prefix in certain macros Miquel Raynal
2021-09-05 13:15 ` Jonathan Cameron
2021-09-02 21:51 ` [PATCH v2 29/46] mfd: ti_am335x_tscadc: Rename a variable Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 30/46] mfd: ti_am335x_tscadc: Fix an error message Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 31/46] mfd: ti_am335x_tscadc: Add a boolean to clarify the presence of a touchscreen Miquel Raynal
2021-09-05 13:17 ` Jonathan Cameron
2021-09-02 21:51 ` [PATCH v2 32/46] mfd: ti_am335x_tscadc: Introduce has_tsc Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 33/46] mfd: ti_am335x_tscadc: Move control register configuration Miquel Raynal
2021-09-03 13:24 ` Grygorii Strashko
2021-09-05 13:18 ` Jonathan Cameron
2021-09-05 13:22 ` Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 34/46] mfd: ti_am335x_tscadc: Add ADC1/magnetic reader support Miquel Raynal
2021-09-05 13:23 ` Jonathan Cameron
2021-09-14 18:25 ` Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 35/46] mfd: ti_am335x_tscadc: Support the correctly spelled DT property Miquel Raynal
2021-09-05 13:24 ` Jonathan Cameron
2021-09-02 21:51 ` [PATCH v2 36/46] iio: adc: ti_am335x_adc: Wait the idle state to avoid stalls Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 37/46] iio: adc: ti_am335x_adc: Replace license text with SPDX tag Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 38/46] iio: adc: ti_am335x_adc: Fix style Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 39/46] iio: adc: ti_am335x_adc: Get rid of useless gotos Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 40/46] iio: adc: ti_am335x_adc: Gather the checks on the delays Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 41/46] iio: adc: ti_am335x_adc: Add a unit to the timeout delay Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 42/46] iio: adc: ti_am335x_adc: Add the scale information Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 43/46] iio: adc: ti_am335x_adc: Add the am437x compatible Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 44/46] ARM: dts: am437x-cm-t43: Use a correctly spelled DT property Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 45/46] ARM: dts: am43xx: Describe the magnetic reader/ADC1 hardware module Miquel Raynal
2021-09-02 22:17 ` Miquel Raynal
2021-09-02 21:51 ` [PATCH v2 46/46] ARM: dts: am437x-gp-evm: enable ADC1 Miquel Raynal
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=20210919164933.77d9a097@jic23-huawei \
--to=jic23@kernel.org \
--cc=Jonathan.Cameron@huawei.com \
--cc=bcousson@baylibre.com \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.torokhov@gmail.com \
--cc=grygorii.strashko@ti.com \
--cc=jreeder@ti.com \
--cc=kristo@kernel.org \
--cc=lars@metafoo.de \
--cc=lee.jones@linaro.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=lokeshvutla@ti.com \
--cc=miquel.raynal@bootlin.com \
--cc=pmeerw@pmeerw.net \
--cc=robh+dt@kernel.org \
--cc=ryan.barnett@collins.com \
--cc=thomas.petazzoni@bootlin.com \
--cc=tony@atomide.com \
--cc=vigneshr@ti.com \
/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 NNTP newsgroup(s).