[PATCH v3 0/2] iio: adc: mcp3422: apply bit manipulation macros

[PATCH v3 0/2] iio: adc: mcp3422: apply bit manipulation macros

[Marcelo Machado Lage]

This patch set rewrites several open-coded bit manipulation parts of
the MCP3421/2/3/4/5/6/7/8 driver code using bits.h and bitfield.h
macros.

Marcelo Machado Lage (2):
  iio: adc: mcp3422: rewrite mask macros using bits.h macros
  iio: adc: mcp3422: write bit operations with bitfield.h macros

 drivers/iio/adc/mcp3422.c | 63 ++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 37 deletions(-)

-- 
2.34.1

[PATCH v3 1/2] iio: adc: mcp3422: rewrite mask macros using bits.h macros

[Marcelo Machado Lage]

Rewrite MCP3422_CHANNEL_MASK, MCP3422_PGA_MASK, MCP3422_PGA_MASK
and MCP3422_CONT_SAMPLING using GENMASK() and BIT() macros from
bits.h.

The other macros MCP3422_SRATE_{240, 60, 15, 3} were not changed
because they are also used as array indices.

Signed-off-by: Marcelo Machado Lage <marcelomlage@usp.br>
Co-developed-by: Vinicius Lira <vinilira@usp.br>
Signed-off-by: Vinicius Lira <vinilira@usp.br>
---

v1 -> v2:
 - Split this change from the rest of the patch, following a request by Andy Shevchenko
v2 -> v3:
 - Sort marks in logical order, following a request by David Lechner
 - Delete the "/* Masks */" comment to avoid confusion, following a request by Jonathan Cameron

v1: https://lore.kernel.org/linux-iio/20260417005041.484742-1-marcelomlage@usp.br/
v2: https://lore.kernel.org/linux-iio/20260417165747.507487-1-marcelomlage@usp.br/

 drivers/iio/adc/mcp3422.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
index 50834fdcf738..0cd0e7d39e39 100644
--- a/drivers/iio/adc/mcp3422.c
+++ b/drivers/iio/adc/mcp3422.c
@@ -13,6 +13,7 @@
  * voltage unit is nV.
  */
 
+#include <linux/bits.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
@@ -24,10 +25,10 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
-/* Masks */
-#define MCP3422_CHANNEL_MASK	0x60
-#define MCP3422_PGA_MASK	0x03
-#define MCP3422_SRATE_MASK	0x0C
+#define MCP3422_CHANNEL_MASK	GENMASK(6, 5)
+#define MCP3422_SRATE_MASK	GENMASK(3, 2)
+#define MCP3422_PGA_MASK	GENMASK(1, 0)
+
 #define MCP3422_SRATE_240	0x0
 #define MCP3422_SRATE_60	0x1
 #define MCP3422_SRATE_15	0x2
@@ -36,7 +37,7 @@
 #define MCP3422_PGA_2	1
 #define MCP3422_PGA_4	2
 #define MCP3422_PGA_8	3
-#define MCP3422_CONT_SAMPLING	0x10
+#define MCP3422_CONT_SAMPLING	BIT(4)
 
 #define MCP3422_CHANNEL(config)	(((config) & MCP3422_CHANNEL_MASK) >> 5)
 #define MCP3422_PGA(config)	((config) & MCP3422_PGA_MASK)
-- 
2.34.1

Re: [PATCH v3 1/2] iio: adc: mcp3422: rewrite mask macros using bits.h macros

[Jonathan Cameron]

On Mon, 20 Apr 2026 16:14:54 -0300
Marcelo Machado Lage <marcelomlage@usp.br> wrote:

> Rewrite MCP3422_CHANNEL_MASK, MCP3422_PGA_MASK, MCP3422_PGA_MASK
> and MCP3422_CONT_SAMPLING using GENMASK() and BIT() macros from
> bits.h.
https://sashiko.dev/#/patchset/20260420191455.529923-1-marcelomlage%40usp.br
pointed out repetition of PGA_MASK.

The other comments are a mixed bag but it's worth taking a look to see
if you think any of them matter.  They are all really about the existin
code.


> 
> The other macros MCP3422_SRATE_{240, 60, 15, 3} were not changed
> because they are also used as array indices.
> 
> Signed-off-by: Marcelo Machado Lage <marcelomlage@usp.br>
> Co-developed-by: Vinicius Lira <vinilira@usp.br>
> Signed-off-by: Vinicius Lira <vinilira@usp.br>

Re: [PATCH v3 1/2] iio: adc: mcp3422: rewrite mask macros using bits.h macros

[Marcelo Machado Lage]

Dear Jonathan,
Thank you for your comments. We will correct the mistake in the commit
message in the next version of the patch.
Regarding the comments at
https://sashiko.dev/#/patchset/20260420191455.529923-1-marcelomlage%40usp.br,
we appreciate the opportunity to learn about this resource. However,
after going through the content there, we do not feel confident to
address other pre-existing issues in the code at the moment.
Best regards,
Marcelo and Vinícius


Em ter., 21 de abr. de 2026 às 12:55, Jonathan Cameron
<jic23@kernel.org> escreveu:
>
> On Mon, 20 Apr 2026 16:14:54 -0300
> Marcelo Machado Lage <marcelomlage@usp.br> wrote:
>
> > Rewrite MCP3422_CHANNEL_MASK, MCP3422_PGA_MASK, MCP3422_PGA_MASK
> > and MCP3422_CONT_SAMPLING using GENMASK() and BIT() macros from
> > bits.h.
> https://sashiko.dev/#/patchset/20260420191455.529923-1-marcelomlage%40usp.br
> pointed out repetition of PGA_MASK.
>
> The other comments are a mixed bag but it's worth taking a look to see
> if you think any of them matter.  They are all really about the existin
> code.
>
>
> >
> > The other macros MCP3422_SRATE_{240, 60, 15, 3} were not changed
> > because they are also used as array indices.
> >
> > Signed-off-by: Marcelo Machado Lage <marcelomlage@usp.br>
> > Co-developed-by: Vinicius Lira <vinilira@usp.br>
> > Signed-off-by: Vinicius Lira <vinilira@usp.br>

Re: [PATCH v3 1/2] iio: adc: mcp3422: rewrite mask macros using bits.h macros

[Andy Shevchenko]

On Thu, Apr 23, 2026 at 04:15:54PM -0300, Marcelo Machado Lage wrote:

Please, stop top-posting! It's not how we do in Linux kernel mailing lists.


> Thank you for your comments. We will correct the mistake in the commit
> message in the next version of the patch.
> Regarding the comments at
> https://sashiko.dev/#/patchset/20260420191455.529923-1-marcelomlage%40usp.br,
> we appreciate the opportunity to learn about this resource. However,
> after going through the content there, we do not feel confident to
> address other pre-existing issues in the code at the moment.

Also drop the second 'macros' in the Subject or rephrase it.
Exempli gratia: 'rewrite mask macros with help of bits.h APIs'.

> Em ter., 21 de abr. de 2026 às 12:55, Jonathan Cameron
> <jic23@kernel.org> escreveu:
> >
> > On Mon, 20 Apr 2026 16:14:54 -0300
> > Marcelo Machado Lage <marcelomlage@usp.br> wrote:
> >
> > > Rewrite MCP3422_CHANNEL_MASK, MCP3422_PGA_MASK, MCP3422_PGA_MASK
> > > and MCP3422_CONT_SAMPLING using GENMASK() and BIT() macros from
> > > bits.h.
> > https://sashiko.dev/#/patchset/20260420191455.529923-1-marcelomlage%40usp.br
> > pointed out repetition of PGA_MASK.
> >
> > The other comments are a mixed bag but it's worth taking a look to see
> > if you think any of them matter.  They are all really about the existin
> > code.

> > > The other macros MCP3422_SRATE_{240, 60, 15, 3} were not changed
> > > because they are also used as array indices.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 2/2] iio: adc: mcp3422: write bit operations with bitfield.h macros

[Marcelo Machado Lage]

Replace manual bit manipulations with FIELD_GET(), FIELD_PREP() and
FIELD_MODIFY() calls. The resulting code is more readable and
maintainable, and 6 macros previously defined in the header are not
needed anymore.

Signed-off-by: Marcelo Machado Lage <marcelomlage@usp.br>
Co-developed-by: Vinicius Lira <vinilira@usp.br>
Signed-off-by: Vinicius Lira <vinilira@usp.br>
---

v1 -> v2:
 - Remove 3 further macros still present in v1 and replace their usages by inline calls to FIELD_GET(), following a request by Jonathan Comeron
v2 -> v3:
 - Apply stylistic changes to the definition block inside mcp3422_read_raw(), following a request by Andy Shevchenko

v1: https://lore.kernel.org/linux-iio/20260417005041.484742-1-marcelomlage@usp.br/
v2: https://lore.kernel.org/linux-iio/20260417165747.507487-1-marcelomlage@usp.br/

 drivers/iio/adc/mcp3422.c | 52 +++++++++++++++------------------------
 1 file changed, 20 insertions(+), 32 deletions(-)

diff --git a/drivers/iio/adc/mcp3422.c b/drivers/iio/adc/mcp3422.c
index 0cd0e7d39e39..24906f152c56 100644
--- a/drivers/iio/adc/mcp3422.c
+++ b/drivers/iio/adc/mcp3422.c
@@ -13,6 +13,7 @@
  * voltage unit is nV.
  */
 
+#include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
@@ -39,14 +40,6 @@
 #define MCP3422_PGA_8	3
 #define MCP3422_CONT_SAMPLING	BIT(4)
 
-#define MCP3422_CHANNEL(config)	(((config) & MCP3422_CHANNEL_MASK) >> 5)
-#define MCP3422_PGA(config)	((config) & MCP3422_PGA_MASK)
-#define MCP3422_SAMPLE_RATE(config)	(((config) & MCP3422_SRATE_MASK) >> 2)
-
-#define MCP3422_CHANNEL_VALUE(value) (((value) << 5) & MCP3422_CHANNEL_MASK)
-#define MCP3422_PGA_VALUE(value) ((value) & MCP3422_PGA_MASK)
-#define MCP3422_SAMPLE_RATE_VALUE(value) ((value << 2) & MCP3422_SRATE_MASK)
-
 #define MCP3422_CHAN(_index) \
 	{ \
 		.type = IIO_VOLTAGE, \
@@ -109,7 +102,7 @@ static int mcp3422_update_config(struct mcp3422 *adc, u8 newconfig)
 static int mcp3422_read(struct mcp3422 *adc, int *value, u8 *config)
 {
 	int ret = 0;
-	u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config);
+	u8 sample_rate = FIELD_GET(MCP3422_SRATE_MASK, adc->config);
 	u8 buf[4] = {0, 0, 0, 0};
 	u32 temp;
 
@@ -137,18 +130,18 @@ static int mcp3422_read_channel(struct mcp3422 *adc,
 
 	mutex_lock(&adc->lock);
 
-	if (req_channel != MCP3422_CHANNEL(adc->config)) {
+	if (req_channel != FIELD_GET(MCP3422_CHANNEL_MASK, adc->config)) {
 		config = adc->config;
-		config &= ~MCP3422_CHANNEL_MASK;
-		config |= MCP3422_CHANNEL_VALUE(req_channel);
-		config &= ~MCP3422_PGA_MASK;
-		config |= MCP3422_PGA_VALUE(adc->pga[req_channel]);
+
+		FIELD_MODIFY(MCP3422_CHANNEL_MASK, &config, req_channel);
+		FIELD_MODIFY(MCP3422_PGA_MASK, &config, adc->pga[req_channel]);
+
 		ret = mcp3422_update_config(adc, config);
 		if (ret < 0) {
 			mutex_unlock(&adc->lock);
 			return ret;
 		}
-		msleep(mcp3422_read_times[MCP3422_SAMPLE_RATE(adc->config)]);
+		msleep(mcp3422_read_times[FIELD_GET(MCP3422_SRATE_MASK, adc->config)]);
 	}
 
 	ret = mcp3422_read(adc, value, &config);
@@ -164,9 +157,8 @@ static int mcp3422_read_raw(struct iio_dev *iio,
 {
 	struct mcp3422 *adc = iio_priv(iio);
 	int err;
-
-	u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config);
-	u8 pga		 = MCP3422_PGA(adc->config);
+	u8 sample_rate = FIELD_GET(MCP3422_SRATE_MASK, adc->config);
+	u8 pga = FIELD_GET(MCP3422_PGA_MASK, adc->config);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -182,7 +174,7 @@ static int mcp3422_read_raw(struct iio_dev *iio,
 		return IIO_VAL_INT_PLUS_NANO;
 
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		*val1 = mcp3422_sample_rates[MCP3422_SAMPLE_RATE(adc->config)];
+		*val1 = mcp3422_sample_rates[FIELD_GET(MCP3422_SRATE_MASK, adc->config)];
 		return IIO_VAL_INT;
 
 	default:
@@ -200,7 +192,7 @@ static int mcp3422_write_raw(struct iio_dev *iio,
 	u8 temp;
 	u8 config = adc->config;
 	u8 req_channel = channel->channel;
-	u8 sample_rate = MCP3422_SAMPLE_RATE(config);
+	u8 sample_rate = FIELD_GET(MCP3422_SRATE_MASK, config);
 	u8 i;
 
 	switch (mask) {
@@ -212,10 +204,8 @@ static int mcp3422_write_raw(struct iio_dev *iio,
 			if (val2 == mcp3422_scales[sample_rate][i]) {
 				adc->pga[req_channel] = i;
 
-				config &= ~MCP3422_CHANNEL_MASK;
-				config |= MCP3422_CHANNEL_VALUE(req_channel);
-				config &= ~MCP3422_PGA_MASK;
-				config |= MCP3422_PGA_VALUE(adc->pga[req_channel]);
+				FIELD_MODIFY(MCP3422_CHANNEL_MASK, &config, req_channel);
+				FIELD_MODIFY(MCP3422_PGA_MASK, &config, adc->pga[req_channel]);
 
 				return mcp3422_update_config(adc, config);
 			}
@@ -242,10 +232,8 @@ static int mcp3422_write_raw(struct iio_dev *iio,
 			return -EINVAL;
 		}
 
-		config &= ~MCP3422_CHANNEL_MASK;
-		config |= MCP3422_CHANNEL_VALUE(req_channel);
-		config &= ~MCP3422_SRATE_MASK;
-		config |= MCP3422_SAMPLE_RATE_VALUE(temp);
+		FIELD_MODIFY(MCP3422_CHANNEL_MASK, &config, req_channel);
+		FIELD_MODIFY(MCP3422_SRATE_MASK, &config, temp);
 
 		return mcp3422_update_config(adc, config);
 
@@ -284,7 +272,7 @@ static ssize_t mcp3422_show_scales(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct mcp3422 *adc = iio_priv(dev_to_iio_dev(dev));
-	u8 sample_rate = MCP3422_SAMPLE_RATE(adc->config);
+	u8 sample_rate = FIELD_GET(MCP3422_SRATE_MASK, adc->config);
 
 	return sprintf(buf, "0.%09u 0.%09u 0.%09u 0.%09u\n",
 		mcp3422_scales[sample_rate][0],
@@ -378,9 +366,9 @@ static int mcp3422_probe(struct i2c_client *client)
 
 	/* meaningful default configuration */
 	config = (MCP3422_CONT_SAMPLING
-		| MCP3422_CHANNEL_VALUE(0)
-		| MCP3422_PGA_VALUE(MCP3422_PGA_1)
-		| MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
+		| FIELD_PREP(MCP3422_CHANNEL_MASK, 0)
+		| FIELD_PREP(MCP3422_PGA_MASK, MCP3422_PGA_1)
+		| FIELD_PREP(MCP3422_SRATE_MASK, MCP3422_SRATE_240));
 	err = mcp3422_update_config(adc, config);
 	if (err < 0)
 		return err;
-- 
2.34.1

Re: [PATCH v3 2/2] iio: adc: mcp3422: write bit operations with bitfield.h macros

[Andy Shevchenko]

On Mon, Apr 20, 2026 at 04:14:55PM -0300, Marcelo Machado Lage wrote:
> Replace manual bit manipulations with FIELD_GET(), FIELD_PREP() and
> FIELD_MODIFY() calls. The resulting code is more readable and
> maintainable, and 6 macros previously defined in the header are not
> needed anymore.

...

>  	/* meaningful default configuration */
>  	config = (MCP3422_CONT_SAMPLING
> -		| MCP3422_CHANNEL_VALUE(0)
> -		| MCP3422_PGA_VALUE(MCP3422_PGA_1)
> -		| MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
> +		| FIELD_PREP(MCP3422_CHANNEL_MASK, 0)
> +		| FIELD_PREP(MCP3422_PGA_MASK, MCP3422_PGA_1)
> +		| FIELD_PREP(MCP3422_SRATE_MASK, MCP3422_SRATE_240));

While at it, please fix the style by making the operator to be the last on
the respective lines.

	config = MCP3422_CONT_SAMPLING |
		 FIELD_PREP(MCP3422_CHANNEL_MASK, 0) |
		 FIELD_PREP(MCP3422_PGA_MASK, MCP3422_PGA_1) |
		 FIELD_PREP(MCP3422_SRATE_MASK, MCP3422_SRATE_240));

But, to me sounds better if you just resplit it it to

	config = MCP3422_CONT_SAMPLING;
	FIELD_MODIFY(...);
	FIELD_MODIFY(...);
	FIELD_MODIFY(...);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] iio: adc: mcp3422: write bit operations with bitfield.h macros

[Jonathan Cameron]

On Mon, 20 Apr 2026 23:24:51 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Mon, Apr 20, 2026 at 04:14:55PM -0300, Marcelo Machado Lage wrote:
> > Replace manual bit manipulations with FIELD_GET(), FIELD_PREP() and
> > FIELD_MODIFY() calls. The resulting code is more readable and
> > maintainable, and 6 macros previously defined in the header are not
> > needed anymore.  
> 
> ...
> 
> >  	/* meaningful default configuration */
> >  	config = (MCP3422_CONT_SAMPLING
> > -		| MCP3422_CHANNEL_VALUE(0)
> > -		| MCP3422_PGA_VALUE(MCP3422_PGA_1)
> > -		| MCP3422_SAMPLE_RATE_VALUE(MCP3422_SRATE_240));
> > +		| FIELD_PREP(MCP3422_CHANNEL_MASK, 0)
> > +		| FIELD_PREP(MCP3422_PGA_MASK, MCP3422_PGA_1)
> > +		| FIELD_PREP(MCP3422_SRATE_MASK, MCP3422_SRATE_240));  
> 
> While at it, please fix the style by making the operator to be the last on
> the respective lines.
> 
> 	config = MCP3422_CONT_SAMPLING |
> 		 FIELD_PREP(MCP3422_CHANNEL_MASK, 0) |
> 		 FIELD_PREP(MCP3422_PGA_MASK, MCP3422_PGA_1) |
> 		 FIELD_PREP(MCP3422_SRATE_MASK, MCP3422_SRATE_240));
> 
> But, to me sounds better if you just resplit it it to
> 
> 	config = MCP3422_CONT_SAMPLING;
> 	FIELD_MODIFY(...);
> 	FIELD_MODIFY(...);
> 	FIELD_MODIFY(...);
> 
To me FIELD_MODIFY() kind of hints it might already be set but in this
case the whole value is being written.  So maybe just reorganize
as per Andy's first suggestion above.