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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Apr 20, 2026 at 06:49:18PM -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.

...

>  #include <linux/mod_devicetable.h>
>  #include <linux/iio/iio.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>

Add a patch to sort headers first, it will help maintenance in a long term.

...

>  	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) |

!differential is boolean, you want integer, while the code generation will be
the same, I prefer to see that explicitly, id est

	differential ? 0 : 1

Ditto for other similar cases.

> +		       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);
>  	default:
>  		return -EINVAL;
>  	}

...

> +	u16 raw16 = be16_to_cpup((__be16 *)adc->rx_buf);

First of all, we don't allow free variable definitions (only few exceptions,
and this is none of them). Second, the casting doesn't sound right. You
probably wanted get_unaligned_be16() instead.

...

> +		raw = FIELD_GET(MCP355X_DATA_MASK, raw);

Besides wrong placement (please, leave it before if and after comment), this is
bad code for reading and understanding. Use another temporary variable for the
purpose or refactor to avoid foo = bar(foo); cases.

>  		/*
>  		 * 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 */

-- 
With Best Regards,
Andy Shevchenko