[PATCH] Replace manual bitfield manipulations with field_get

[PATCH] Replace manual bitfield manipulations with field_get

[Rafael Lopes Santana]

From: Rafael Lopes Santana <santanarl@usp.br>

Signed-off-by: Rafael Lopes Santana <santanarl@usp.br>
---
 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)) |
+			      ((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;
 
-- 
2.53.0

Re: [PATCH] Replace manual bitfield manipulations with field_get

[Andy Shevchenko]

On Fri, May 1, 2026 at 4:16 AM Rafael Lopes Santana <santanarl@usp.br> wrote:
> From: Rafael Lopes Santana <santanarl@usp.br>
>
> Signed-off-by: Rafael Lopes Santana <santanarl@usp.br>

NAK.
I'm tired of replying to each and every 101 issues with your patches.
Talk to your group / mentors / read previous reviews to learn first.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] Replace manual bitfield manipulations with field_get

[Joshua Crofts]

On Fri, 1 May 2026 at 03:17, Rafael Lopes Santana <santanarl@usp.br> wrote:
>
> From: Rafael Lopes Santana <santanarl@usp.br>
>
> Signed-off-by: Rafael Lopes Santana <santanarl@usp.br>

To expand on Andy's review:
1. fix the title of the patch - you must add the subsystem + driver
that you're contributing to (in your case "iio: proximity: aw96103:
replace manual bitfield manipulations with field_get").
2. you're missing the body of the patch message - do not send
patches without a description of the changes you're trying to make.
You should describe what you're doing, how you're doing it and what
difference will the patch make in the future.
3. the macro used is FIELD_GET(), therefore all mentions of it in the
title or message body should be the same (no field_get in the title).
It's also preferred to add parentheses to the macro name to imply
that it takes a parameter (in this case, FIELD_GET() does take a
parameter).

The first two problems are things that will automatically get your
patch rejected (no matter how good the code is). Also, wait at
least 24 hours before sending a v2.

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

Is this code more readable than the original? I would argue
that the original was better.

-- 
Kind regards

CJD

Re: [PATCH] Replace manual bitfield manipulations with field_get

[Joshua Crofts]

On Fri, 1 May 2026 at 16:34, Joshua Crofts <joshua.crofts1@gmail.com> wrote:
>
> On Fri, 1 May 2026 at 03:17, Rafael Lopes Santana <santanarl@usp.br> wrote:
> >
> > From: Rafael Lopes Santana <santanarl@usp.br>
> >
> > Signed-off-by: Rafael Lopes Santana <santanarl@usp.br>
>
> To expand on Andy's review:
> 1. fix the title of the patch - you must add the subsystem + driver
> that you're contributing to (in your case "iio: proximity: aw96103:
> replace manual bitfield manipulations with field_get").
> 2. you're missing the body of the patch message - do not send
> patches without a description of the changes you're trying to make.
> You should describe what you're doing, how you're doing it and what
> difference will the patch make in the future.
> 3. the macro used is FIELD_GET(), therefore all mentions of it in the
> title or message body should be the same (no field_get in the title).
> It's also preferred to add parentheses to the macro name to imply
> that it takes a parameter (in this case, FIELD_GET() does take a
> parameter).

I made a mistake - you're using field_get() (since you're not passing
compile-time constants), so lower case is fine. However, adding
parentheses is still the norm.

-- 
Kind regards

CJD

Re: [PATCH] Replace manual bitfield manipulations with field_get

[Andy Shevchenko]

On Fri, May 1, 2026 at 5:38 PM Joshua Crofts <joshua.crofts1@gmail.com> wrote:
> On Fri, 1 May 2026 at 16:34, Joshua Crofts <joshua.crofts1@gmail.com> wrote:
> > On Fri, 1 May 2026 at 03:17, Rafael Lopes Santana <santanarl@usp.br> wrote:

...

> > 2. you're missing the body of the patch message - do not send
> > patches without a description of the changes you're trying to make.
> > You should describe what you're doing, how you're doing it and what
> > difference will the patch make in the future.

The main point is to describe "why?" the patch is done, what is the
benefit of applying it vs. not applying.

> > 3. the macro used is FIELD_GET(), therefore all mentions of it in the
> > title or message body should be the same (no field_get in the title).
> > It's also preferred to add parentheses to the macro name to imply
> > that it takes a parameter (in this case, FIELD_GET() does take a
> > parameter).
>
> I made a mistake - you're using field_get() (since you're not passing
> compile-time constants), so lower case is fine. However, adding
> parentheses is still the norm.

I am not sure I follow this. Too many parentheses is not a C language
style. It's not LISP, and we only do unneeded parentheses for the
cases where it's not immediately obvious by operator precedence.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] Replace manual bitfield manipulations with field_get

[Joshua Crofts]

On Fri, 1 May 2026 at 20:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> I am not sure I follow this. Too many parentheses is not a C language
> style. It's not LISP, and we only do unneeded parentheses for the
> cases where it's not immediately obvious by operator precedence.

This is not a case of operator precedence, I meant it as if you
mention a macro that takes a parameter in the commit message,
it should be styled as MACRO_NAME(), at least that's what I
interpreted from previous code reviews. (same goes for functions -
function_name()). More of a nitpick though.

-- 
Kind regards

CJD

Re: [PATCH] Replace manual bitfield manipulations with field_get

[Andy Shevchenko]

On Fri, May 1, 2026 at 9:41 PM Joshua Crofts <joshua.crofts1@gmail.com> wrote:
>
> On Fri, 1 May 2026 at 20:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > I am not sure I follow this. Too many parentheses is not a C language
> > style. It's not LISP, and we only do unneeded parentheses for the
> > cases where it's not immediately obvious by operator precedence.
>
> This is not a case of operator precedence, I meant it as if you
> mention a macro that takes a parameter in the commit message,
> it should be styled as MACRO_NAME(), at least that's what I
> interpreted from previous code reviews. (same goes for functions -
> function_name()). More of a nitpick though.

I see, I meant the parentheses in the real code. There are too many of
them after conversion to field_get().


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] Replace manual bitfield manipulations with field_get

[Jonathan Cameron]

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

Re: [PATCH] Replace manual bitfield manipulations with field_get

[David Lechner]

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

Re: [PATCH] Replace manual bitfield manipulations with field_get

[Jonathan Cameron]

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