Re: [PATCH] Replace manual bitfield manipulations with field_get

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 5 May 2026 08:52:44 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 5/5/26 7:16 AM, Jonathan Cameron wrote:
> > On Thu, 30 Apr 2026 22:15:46 -0300
> > Rafael Lopes Santana <santanarl@usp.br> wrote:
> >   
> >> From: Rafael Lopes Santana <santanarl@usp.br>
> >>
> >> Signed-off-by: Rafael Lopes Santana <santanarl@usp.br>  
> > Hi Rafael,
> > 
> > Additional comments inline.
> > 
> > Given this is packing code that is using shifts in one direction even
> > in your new version I'm not seeing a clear advantage to this change.
> >   
> >> ---
> >>  drivers/iio/proximity/aw96103.c | 10 +++++-----
> >>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/iio/proximity/aw96103.c b/drivers/iio/proximity/aw96103.c
> >> index 3472a2c36e44..a8a6ae02438a 100644
> >> --- a/drivers/iio/proximity/aw96103.c
> >> +++ b/drivers/iio/proximity/aw96103.c
> >> @@ -591,7 +591,7 @@ static void aw96103_cfg_update(const struct firmware *fw, void *data)
> >>  	}
> >>  
> >>  	for (i = 0; i < aw96103->max_channels; i++) {
> >> -		if ((aw96103->chan_en >> i) & 0x01)
> >> +		if ((field_get(BIT(i), aw96103->chan_en)))
> >>  			aw96103->channels_arr[i].used = true;
> >>  		else
> >>  			aw96103->channels_arr[i].used = false;
> >> @@ -643,10 +643,10 @@ static irqreturn_t aw96103_irq(int irq, void *data)
> >>  		if (!aw96103->channels_arr[i].used)
> >>  			continue;
> >>  
> >> -		curr_status = (((curr_status_val >> (24 + i)) & 0x1)) |
> >> -			      (((curr_status_val >> (16 + i)) & 0x1) << 1) |
> >> -			      (((curr_status_val >> (8 + i)) & 0x1) << 2) |
> >> -			      (((curr_status_val >> i) & 0x1) << 3);
> >> +		curr_status = (field_get(BIT(24+i), curr_status_val)) |  
> > 
> > Look at coding style for the kernel. You are missing some white space here.
> > 
> > I don't like this but if you were to do it for consistency it would be
> > 
> > 		curr_status = FIELD_PREP(BIT(0), field_get(BIT(24 + i), cur_status_val) |
> > 			      FIELD_PREP(BIT(1), field_get(BIT(16 + i), cur_status_val) |
> > 			      FIELD_PREP(BIT(2), field_get(BIT(8 + i), cur_status_val) |
> > 			      FIELD_PREP(BIT(3), field_get(BIT(i), cur_status_val);  
> 
> I actually find this much quicker to understand the intention of the code.
> 
Ok. That's a clear vote in favour, please respin it like this for v2, remembering
to deal with all the process stuff others have highlighted.

> > 
> > The benefit of that is slightly more than what you have but it's still ugly enough
> > I'm not sure it's worth doing.  Note FIELD_PREP() in this direction as the mask is constant
> > 
> > Given the bit smashing going on here is always going to be ugly I'm not sure
> > any of these are better than the original though I'm open to hearing what others
> > think of this more complete version.
> > 
> >   
> >> +			      ((field_get(BIT(16+i), curr_status_val)) << 1) |
> >> +			      ((field_get(BIT(8+i), curr_status_val)) << 2) |
> >> +			      ((field_get(BIT(i), curr_status_val)) << 3);
> >>  		if (aw96103->channels_arr[i].old_irq_status == curr_status)
> >>  			continue;
> >>    
> >   
> 
>