[PATCH v4 0/2] iio: adc: mcp320x: bitfield refactoring

[PATCH v4 0/2] iio: adc: mcp320x: bitfield refactoring

[Gustavo Pagnotta Faria]

This patch series refactors the mcp320x driver to use the standard
Linux bitfield API and sorts the headers alphabetically as requested.

Changes in v4:
 - Separated the patch into a series (Patch 1: sort headers,
Patch 2: bitfield refactor).
 - Fixed style of MCP3550 comment
 - Avoided variable reassignments (foo = bar(foo)) and used explicit
temporary variables.
 - Replaced boolean casting (!differential) with ternary operator.
 - Fixed line length limit warning in the mcp3301 case by introducing
temporary variables.
 - Fixed placement of RX data extraction in the MCP3550 block to
strictly follow the descriptive comment, as requested.
 - Link to v3: https://lore.kernel.org/linux-iio/20260422030846.45317-1-gustavo.pagnotta@ime.usp.br/

Changes in v3:
 - Removed wildcard 'X' from all macros (including MCP355X -> MCP3550),
using lowest part number instead.
 - Separated Tx channel masks for 4-channel (MCP3004) and
8-channel (MCP3008).
 - Added comment explaining MCP3550 Data and Status bits usage.
 - Replaced be16_to_cpup() and be32_to_cpup() with get_unaligned_be16()
and get_unaligned_be32(), moving them inside the relevant switch cases.
 - Fixed accidental removal of '~' operator in MCP3550 overrange clearing.
 - Link to v2: https://lore.kernel.org/linux-iio/20260420215021.14112-1-gustavo.pagnotta@ime.usp.br/

Changes in v2:
 - Sent previously as an unversioned reply to the v1 thread.
 - Fixed typo in author's email address.
 - Link to v1: https://lore.kernel.org/linux-iio/20260420212405.11789-1-gustavo.pagnotta@ime.usp.br/

Gustavo Pagnotta Faria (2):
  iio: adc: mcp320x: sort headers alphabetically
  iio: adc: mcp320x: refactor driver to use bitfield API

 drivers/iio/adc/mcp320x.c | 89 ++++++++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 24 deletions(-)

-- 
2.43.0

[PATCH v4 1/2] iio: adc: mcp320x: sort headers alphabetically

[Gustavo Pagnotta Faria]

Sort include directives alphabetically to improve long-term maintenance.

Signed-off-by: Gustavo Pagnotta Faria <gustavo.pagnotta@ime.usp.br>
Co-developed-by: Eduardo Augusto <eduardoaugustoabc@ime.usp.br>
Signed-off-by: Eduardo Augusto <eduardoaugustoabc@ime.usp.br>
Co-developed-by: Christian Barry <christian.barry@ime.usp.br>
Signed-off-by: Christian Barry <christian.barry@ime.usp.br>
---
 drivers/iio/adc/mcp320x.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
index 57cff3772ebe..d011f1732bd7 100644
--- a/drivers/iio/adc/mcp320x.c
+++ b/drivers/iio/adc/mcp320x.c
@@ -37,13 +37,13 @@
  * http://ww1.microchip.com/downloads/en/DeviceDoc/21950D.pdf  mcp3550/1/3
  */
 
-#include <linux/err.h>
 #include <linux/delay.h>
-#include <linux/spi/spi.h>
-#include <linux/module.h>
-#include <linux/mod_devicetable.h>
+#include <linux/err.h>
 #include <linux/iio/iio.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
 #include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
 
 enum {
 	mcp3001,
-- 
2.43.0

Re: [PATCH v4 1/2] iio: adc: mcp320x: sort headers alphabetically

[Andy Shevchenko]

On Wed, Apr 22, 2026 at 07:14:12PM -0300, Gustavo Pagnotta Faria wrote:
> Sort include directives alphabetically to improve long-term maintenance.

> Signed-off-by: Gustavo Pagnotta Faria <gustavo.pagnotta@ime.usp.br>
> Co-developed-by: Eduardo Augusto <eduardoaugustoabc@ime.usp.br>
> Signed-off-by: Eduardo Augusto <eduardoaugustoabc@ime.usp.br>
> Co-developed-by: Christian Barry <christian.barry@ime.usp.br>
> Signed-off-by: Christian Barry <christian.barry@ime.usp.br>

Is this SoB chain for real? If you want to give credits to mentors or so,
it should be done differently.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/2] iio: adc: mcp320x: sort headers alphabetically

[Gustavo Pagnotta Faria]

On Thu, Apr 23, 2026 at 20:46:26 +0300, Andy Shevchenko wrote:
> Is this SoB chain for real? If you want to give credits to mentors or so,
> it should be done differently.

Hi Andy,

Yes, the chain is real. Eduardo, Christian, and I are classmates at the
University of São Paulo (IME-USP), and we are working on this driver
together as a group assignment.

We are doing mob programming on the same machine, which is why we
included the Co-developed-by tags for everyone involved in the changes.
They are not my mentors or professors.

However, I understand that having three authors for such a trivial patch
(like sorting headers) looks highly unusual. I also realize that, as the
sender of the patch, my Signed-off-by should have been placed at the
very bottom of the chain.

For the upcoming v5, would you prefer me to keep the Co-developed-by
tags on the main refactoring patch (Patch 2) and leave only my
Signed-off-by on this trivial header sorting one?

--
Thank you for the review and the guidance,
Gustavo Pagnotta

Re: [PATCH v4 1/2] iio: adc: mcp320x: sort headers alphabetically

[Andy Shevchenko]

On Thu, Apr 23, 2026 at 05:45:08PM -0300, Gustavo Pagnotta Faria wrote:
> On Thu, Apr 23, 2026 at 20:46:26 +0300, Andy Shevchenko wrote:
> > Is this SoB chain for real? If you want to give credits to mentors or so,
> > it should be done differently.
> 
> Yes, the chain is real. Eduardo, Christian, and I are classmates at the
> University of São Paulo (IME-USP), and we are working on this driver
> together as a group assignment.
> 
> We are doing mob programming on the same machine, which is why we
> included the Co-developed-by tags for everyone involved in the changes.
> They are not my mentors or professors.

A-ha, thanks for elaboration on this. See below how I suggest to address this
case.

> However, I understand that having three authors for such a trivial patch
> (like sorting headers) looks highly unusual. I also realize that, as the
> sender of the patch, my Signed-off-by should have been placed at the
> very bottom of the chain.

We use terms: Author, Committer, Submitter. The latter one is probably what
you are referring to.

> For the upcoming v5, would you prefer me to keep the Co-developed-by
> tags on the main refactoring patch (Patch 2) and leave only my
> Signed-off-by on this trivial header sorting one?

It's up to maintainers. Just put the information in the cover letter to explain
these rather long SoB chains. If maintainer is happy, they will take change as
is, otherwise they will ask for modifications.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/2] iio: adc: mcp320x: sort headers alphabetically

[Jonathan Cameron]

On Fri, 24 Apr 2026 11:01:19 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Thu, Apr 23, 2026 at 05:45:08PM -0300, Gustavo Pagnotta Faria wrote:
> > On Thu, Apr 23, 2026 at 20:46:26 +0300, Andy Shevchenko wrote:  
> > > Is this SoB chain for real? If you want to give credits to mentors or so,
> > > it should be done differently.  
> > 
> > Yes, the chain is real. Eduardo, Christian, and I are classmates at the
> > University of São Paulo (IME-USP), and we are working on this driver
> > together as a group assignment.
> > 
> > We are doing mob programming on the same machine, which is why we
> > included the Co-developed-by tags for everyone involved in the changes.
> > They are not my mentors or professors.  
> 
> A-ha, thanks for elaboration on this. See below how I suggest to address this
> case.
> 
> > However, I understand that having three authors for such a trivial patch
> > (like sorting headers) looks highly unusual. I also realize that, as the
> > sender of the patch, my Signed-off-by should have been placed at the
> > very bottom of the chain.  
> 
> We use terms: Author, Committer, Submitter. The latter one is probably what
> you are referring to.
> 
> > For the upcoming v5, would you prefer me to keep the Co-developed-by
> > tags on the main refactoring patch (Patch 2) and leave only my
> > Signed-off-by on this trivial header sorting one?  
> 
> It's up to maintainers. Just put the information in the cover letter to explain
> these rather long SoB chains. If maintainer is happy, they will take change as
> is, otherwise they will ask for modifications.
I'm fine with a bunch of co-devs for IIO (and had a good idea
what was going on in this case!)

Given I'm getting a lot of patches from IME-USP folk this year
I would encourage you all to do a bit of cross review.  Some of the
patches are tackling similar problems in different drivers, so
it would be helpful perhaps to 'spread' the feedback by pointing
out where similar improvements apply to your fellow students work.

I guess that might well be going on behind the scenes, but feel
free to do it in public too.   A large part of getting involved in
the upstream kernel is in helping to review!

Thanks,

Jonathan
 
> 

[PATCH v4 2/2] iio: adc: mcp320x: refactor driver to use bitfield API

[Gustavo Pagnotta Faria]

Update the mcp320x driver to use the standard Linux
bitfield API (<linux/bitfield.h>) instead of manual
bitwise shifts and masks.

This replaces the hardcoded shift operations in the TX
data preparation (mcp320x_channel_to_tx_data) with FIELD_PREP()
and replaces the manual masking in the RX data extraction
(mcp320x_adc_conversion) with FIELD_GET(). Explicit masks using
GENMASK() and BIT() were also introduced for both transmit
configurations and receive extractions.

Signed-off-by: Gustavo Pagnotta Faria <gustavo.pagnotta@ime.usp.br>
Co-developed-by: Eduardo Augusto <eduardoaugustoabc@ime.usp.br>
Signed-off-by: Eduardo Augusto <eduardoaugustoabc@ime.usp.br>
Co-developed-by: Christian Barry <christian.barry@ime.usp.br>
Signed-off-by: Christian Barry <christian.barry@ime.usp.br>
---
 drivers/iio/adc/mcp320x.c | 81 +++++++++++++++++++++++++++++----------
 1 file changed, 61 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
index d011f1732bd7..454494a6edd2 100644
--- a/drivers/iio/adc/mcp320x.c
+++ b/drivers/iio/adc/mcp320x.c
@@ -37,6 +37,8 @@
  * http://ww1.microchip.com/downloads/en/DeviceDoc/21950D.pdf  mcp3550/1/3
  */
 
+#include <linux/bitfield.h>
+#include <linux/bits.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/iio/iio.h>
@@ -44,6 +46,37 @@
 #include <linux/module.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
+#include <linux/unaligned.h>
+
+/* MCP3002 Tx Data configuration */
+#define MCP3002_START_BIT	BIT(4)
+#define MCP3002_SGL_DIFF	BIT(3)
+#define MCP3002_CH_MASK		BIT(2)
+
+/* MCP3004 Tx Data configuration */
+#define MCP3004_START_BIT	BIT(6)
+#define MCP3004_SGL_DIFF	BIT(5)
+#define MCP3004_CH_MASK		GENMASK(3, 2)
+
+/* MCP3008 Tx Data configuration */
+#define MCP3008_START_BIT	BIT(6)
+#define MCP3008_SGL_DIFF	BIT(5)
+#define MCP3008_CH_MASK		GENMASK(4, 2)
+
+/* MCP Rx Data extraction masks */
+#define MCP3001_DATA_MASK	GENMASK(12, 3)
+#define MCP3002_DATA_MASK	GENMASK(15, 6)
+#define MCP3201_DATA_MASK	GENMASK(12, 1)
+#define MCP3202_DATA_MASK	GENMASK(15, 4)
+#define MCP3301_DATA_MASK	GENMASK(12, 0)
+
+/*
+ * MCP3550 Data and Status bits
+ * See mcp320x_adc_conversion() for details on when these are used.
+ */
+#define MCP3550_DATA_MASK	GENMASK(31, 8)
+#define MCP3550_SGN		BIT(23)
+#define MCP3550_OVR		BIT(22)
 
 enum {
 	mcp3001,
@@ -99,19 +132,22 @@ struct mcp320x {
 static int mcp320x_channel_to_tx_data(int device_index,
 			const unsigned int channel, bool differential)
 {
-	int start_bit = 1;
-
 	switch (device_index) {
 	case mcp3002:
 	case mcp3202:
-		return ((start_bit << 4) | (!differential << 3) |
-							(channel << 2));
+		return FIELD_PREP(MCP3002_START_BIT, 1) |
+		       FIELD_PREP(MCP3002_SGL_DIFF, differential ? 0 : 1) |
+		       FIELD_PREP(MCP3002_CH_MASK, channel);
 	case mcp3004:
 	case mcp3204:
+		return FIELD_PREP(MCP3004_START_BIT, 1) |
+		       FIELD_PREP(MCP3004_SGL_DIFF, differential ? 0 : 1) |
+		       FIELD_PREP(MCP3004_CH_MASK, channel);
 	case mcp3008:
 	case mcp3208:
-		return ((start_bit << 6) | (!differential << 5) |
-							(channel << 2));
+		return FIELD_PREP(MCP3008_START_BIT, 1) |
+		       FIELD_PREP(MCP3008_SGL_DIFF, differential ? 0 : 1) |
+		       FIELD_PREP(MCP3008_CH_MASK, channel);
 	default:
 		return -EINVAL;
 	}
@@ -142,45 +178,50 @@ static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel,
 
 	switch (device_index) {
 	case mcp3001:
-		*val = (adc->rx_buf[0] << 5 | adc->rx_buf[1] >> 3);
+		*val = FIELD_GET(MCP3001_DATA_MASK, get_unaligned_be16(adc->rx_buf));
 		return 0;
 	case mcp3002:
 	case mcp3004:
 	case mcp3008:
-		*val = (adc->rx_buf[0] << 2 | adc->rx_buf[1] >> 6);
+		*val = FIELD_GET(MCP3002_DATA_MASK, get_unaligned_be16(adc->rx_buf));
 		return 0;
 	case mcp3201:
-		*val = (adc->rx_buf[0] << 7 | adc->rx_buf[1] >> 1);
+		*val = FIELD_GET(MCP3201_DATA_MASK, get_unaligned_be16(adc->rx_buf));
 		return 0;
 	case mcp3202:
 	case mcp3204:
 	case mcp3208:
-		*val = (adc->rx_buf[0] << 4 | adc->rx_buf[1] >> 4);
+		*val = FIELD_GET(MCP3202_DATA_MASK, get_unaligned_be16(adc->rx_buf));
 		return 0;
-	case mcp3301:
-		*val = sign_extend32((adc->rx_buf[0] & 0x1f) << 8
-				    | adc->rx_buf[1], 12);
+	case mcp3301: {
+		u16 rx_val = get_unaligned_be16(adc->rx_buf);
+		u16 raw = FIELD_GET(MCP3301_DATA_MASK, rx_val);
+
+		*val = sign_extend32(raw, 12);
 		return 0;
+		}
 	case mcp3550_50:
 	case mcp3550_60:
 	case mcp3551:
 	case mcp3553: {
-		u32 raw = be32_to_cpup((__be32 *)adc->rx_buf);
+		u32 rx_val = get_unaligned_be32(adc->rx_buf);
+		u32 raw;
 
 		if (!(adc->spi->mode & SPI_CPOL))
-			raw <<= 1; /* strip Data Ready bit in SPI mode 0,0 */
+			rx_val <<= 1; /* strip Data Ready bit in SPI mode 0,0 */
 
 		/*
 		 * If the input is within -vref and vref, bit 21 is the sign.
 		 * Up to 12% overrange or underrange are allowed, in which case
 		 * bit 23 is the sign and bit 0 to 21 is the value.
 		 */
-		raw >>= 8;
-		if (raw & BIT(22) && raw & BIT(23))
+		raw = FIELD_GET(MCP3550_DATA_MASK, rx_val);
+
+		if ((raw & MCP3550_OVR) && (raw & MCP3550_SGN))
 			return -EIO; /* cannot have overrange AND underrange */
-		else if (raw & BIT(22))
-			raw &= ~BIT(22); /* overrange */
-		else if (raw & BIT(23) || raw & BIT(21))
+		else if (raw & MCP3550_OVR)
+			raw &= ~MCP3550_OVR; /* overrange */
+		else if ((raw & MCP3550_SGN) || (raw & BIT(21)))
 			raw |= GENMASK(31, 22); /* underrange or negative */
 
 		*val = (s32)raw;
-- 
2.43.0

Re: [PATCH v4 2/2] iio: adc: mcp320x: refactor driver to use bitfield API

[Andy Shevchenko]

On Wed, Apr 22, 2026 at 07:14:13PM -0300, Gustavo Pagnotta Faria wrote:
> Update the mcp320x driver to use the standard Linux
> bitfield API (<linux/bitfield.h>) instead of manual
> bitwise shifts and masks.

> This replaces the hardcoded shift operations in the TX
> data preparation (mcp320x_channel_to_tx_data) with FIELD_PREP()
> and replaces the manual masking in the RX data extraction
> (mcp320x_adc_conversion) with FIELD_GET(). Explicit masks using
> GENMASK() and BIT() were also introduced for both transmit
> configurations and receive extractions.

I believe somebody told to you (or to the similar patch *) that this paragraph
is an unneeded detail.

*) I'm already too much lost in the flood of the patches to IIO this spring...
I dunno what's going on, but from last few days when seeing something that does
ring a bell, I suggest contributors to start actually reviewing others' patches
in the mailing list for this (IIO) subsystem and read other reviews to learn
from them. It will save a lot of time for everybody.

...

> +	case mcp3301: {
> +		u16 rx_val = get_unaligned_be16(adc->rx_buf);

This is used in all cases (or almost all), can you make this assignment and
conversion be done outside switch-case?

> +		u16 raw = FIELD_GET(MCP3301_DATA_MASK, rx_val);
> +
> +		*val = sign_extend32(raw, 12);
>  		return 0;
> +		}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/2] iio: adc: mcp320x: refactor driver to use bitfield API

[Jonathan Cameron]

On Fri, 24 Apr 2026 11:05:49 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Wed, Apr 22, 2026 at 07:14:13PM -0300, Gustavo Pagnotta Faria wrote:
> > Update the mcp320x driver to use the standard Linux
> > bitfield API (<linux/bitfield.h>) instead of manual
> > bitwise shifts and masks.  
> 
> > This replaces the hardcoded shift operations in the TX
> > data preparation (mcp320x_channel_to_tx_data) with FIELD_PREP()
> > and replaces the manual masking in the RX data extraction
> > (mcp320x_adc_conversion) with FIELD_GET(). Explicit masks using
> > GENMASK() and BIT() were also introduced for both transmit
> > configurations and receive extractions.  
> 
> I believe somebody told to you (or to the similar patch *) that this paragraph
> is an unneeded detail.
> 
> *) I'm already too much lost in the flood of the patches to IIO this spring...
> I dunno what's going on, but from last few days when seeing something that does
> ring a bell, I suggest contributors to start actually reviewing others' patches
> in the mailing list for this (IIO) subsystem and read other reviews to learn
> from them. It will save a lot of time for everybody.

Lol. I just replied to patch 1 with the same message. I should have read on.

> 
> ...
> 
> > +	case mcp3301: {
> > +		u16 rx_val = get_unaligned_be16(adc->rx_buf);  
> 
> This is used in all cases (or almost all), can you make this assignment and
> conversion be done outside switch-case?

I asked for it to be moved in as it is invalid in a couple of cases and
I don't like having variables taking nonsensical values even if they
are then not used.

> 
> > +		u16 raw = FIELD_GET(MCP3301_DATA_MASK, rx_val);
> > +
> > +		*val = sign_extend32(raw, 12);

Can make it less painful though by doing the FIELD_GET inline or maybe
even using the proposed FIELD_GET_SIGNED().

https://lore.kernel.org/all/20260417173621.368914-1-ynorov@nvidia.com/

I guess we can't assume that will land that soon (though please keep
an eye out for it doing so) hence in meantime.

		*val = sign_extend32(FIELD_GET(MCGP3301_DATA_MASK, rx_val), 12);

Thanks,

Jonathan


		
> >  		return 0;
> > +		}  
>