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

[Jonathan Cameron <jic23@kernel.org>]

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;
> > +		}  
>