Re: [PATCH] Replace manual bitfield manipulations with field_get

[David Lechner <dlechner@baylibre.com>]

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.

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