Re: [PATCH] Replace manual bitfield manipulations with field_get

[Jonathan Cameron <jic23@kernel.org>]

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

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