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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 20 Apr 2026 18:49:18 -0300
Gustavo Pagnotta Faria <gustavo.pagnotta@ime.usp.br> wrote:

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

Even though you replied to v1 with why this was being sent, this
should still be clearly marked as [PATCH V2] in the patch title to
reduce the risk of the wrong version getting picked up.

> 
> 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 | 58 +++++++++++++++++++++++++++------------
>  1 file changed, 41 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
> index 57cff3772ebe..b7523e5ea903 100644
> --- a/drivers/iio/adc/mcp320x.c
> +++ b/drivers/iio/adc/mcp320x.c
> @@ -44,6 +44,29 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/iio/iio.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +
> +/* MCP3x02/04/08 Tx Data configuration */
> +#define MCP3X02_START_BIT	BIT(4)

I'm not sure how this driver got in with a wild card in the name but let's
not make it worse.   Manufacturers are far too inconsistent on naming
for them to be 'safe' so just name these after the lowest numbered part
they are relevant to.  Same applies to all wild cards in here.


> +#define MCP3X02_SGL_DIFF	BIT(3)
> +#define MCP3X02_CH_MASK		BIT(2)
> +
> +#define MCP3X04_08_START_BIT	BIT(6)
> +#define MCP3X04_08_SGL_DIFF	BIT(5)
> +#define MCP3X04_08_CH_MASK	GENMASK(4, 2)

As noted below. This is only really the mask for the 8 channel
parts.  They should be split.

> +
> +/* MCP Rx Data extraction masks */
> +#define MCP3001_DATA_MASK	GENMASK(12, 3)
> +#define MCP300X_DATA_MASK	GENMASK(15, 6)

Here's an immediate example of why wild cards are inappropriate.
That clearly includes 3001 and I only have to go up one line
to see it doesn't.

> +#define MCP3201_DATA_MASK	GENMASK(12, 1)
> +#define MCP320X_DATA_MASK	GENMASK(15, 4)
> +#define MCP3301_DATA_MASK	GENMASK(12, 0)
> +
> +/* MCP355X Data and Status bits */
> +#define MCP355X_DATA_MASK	GENMASK(31, 8)

Not sure why this wasn't done as a 24 bit transfer. Ah well,
not something to touch in this patch (or without hardware to test
on!)

> +#define MCP355X_SGN		BIT(23)
> +#define MCP355X_OVR		BIT(22)

These are odd enough that I'd add something short to reference
that there are more details on when these are or aren't part of the data
mask as in the comment in the mcp320x_adc_conversion function.

>  
>  enum {
>  	mcp3001,
> @@ -99,19 +122,19 @@ 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(MCP3X02_START_BIT, 1) |
> +		       FIELD_PREP(MCP3X02_SGL_DIFF, !differential) |
> +		       FIELD_PREP(MCP3X02_CH_MASK, channel);
>  	case mcp3004:
>  	case mcp3204:
>  	case mcp3008:
>  	case mcp3208:
> -		return ((start_bit << 6) | (!differential << 5) |
> -							(channel << 2));
> +		return FIELD_PREP(MCP3X04_08_START_BIT, 1) |
> +		       FIELD_PREP(MCP3X04_08_SGL_DIFF, !differential) |
> +		       FIELD_PREP(MCP3X04_08_CH_MASK, channel);

Given you are using an explicit mask rather simply shifting, I'd break appart
the 4 channel and 8 channel devices.

>  	default:
>  		return -EINVAL;
>  	}
> @@ -140,26 +163,27 @@ static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel,
>  	if (ret < 0)
>  		return ret;
>  
> +	u16 raw16 = be16_to_cpup((__be16 *)adc->rx_buf);

This only applies to some of the case statements. Whilst
it might seem like a lot of duplication I'd move it into the
ones where it applies.

> +
>  	switch (device_index) {
>  	case mcp3001:
> -		*val = (adc->rx_buf[0] << 5 | adc->rx_buf[1] >> 3);
> +		*val = FIELD_GET(MCP3001_DATA_MASK, raw16);
>  		return 0;
>  	case mcp3002:
>  	case mcp3004:
>  	case mcp3008:
> -		*val = (adc->rx_buf[0] << 2 | adc->rx_buf[1] >> 6);
> +		*val = FIELD_GET(MCP300X_DATA_MASK, raw16);
>  		return 0;
>  	case mcp3201:
> -		*val = (adc->rx_buf[0] << 7 | adc->rx_buf[1] >> 1);
> +		*val = FIELD_GET(MCP3201_DATA_MASK, raw16);
>  		return 0;
>  	case mcp3202:
>  	case mcp3204:
>  	case mcp3208:
> -		*val = (adc->rx_buf[0] << 4 | adc->rx_buf[1] >> 4);
> +		*val = FIELD_GET(MCP320X_DATA_MASK, raw16);
>  		return 0;
>  	case mcp3301:
> -		*val = sign_extend32((adc->rx_buf[0] & 0x1f) << 8
> -				    | adc->rx_buf[1], 12);
> +		*val = sign_extend32(FIELD_GET(MCP3301_DATA_MASK, raw16), 12);
>  		return 0;
>  	case mcp3550_50:
>  	case mcp3550_60:
> @@ -170,17 +194,17 @@ static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel,
>  		if (!(adc->spi->mode & SPI_CPOL))
>  			raw <<= 1; /* strip Data Ready bit in SPI mode 0,0 */
>  
> +		raw = FIELD_GET(MCP355X_DATA_MASK, raw);
>  		/*
>  		 * 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))
> +		if ((raw & MCP355X_OVR) && (raw & MCP355X_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 & MCP355X_OVR)
> +			raw &= MCP355X_OVR; /* overrange */
> +		else if ((raw & MCP355X_SGN) || (raw & BIT(21)))
>  			raw |= GENMASK(31, 22); /* underrange or negative */
>  
>  		*val = (s32)raw;