Re: [PATCH v2] iio: frequency: ad9832: Update bit manipulation macros to use FIELD_PREP and GENMASK

[Jonathan Cameron]

On Thu, 27 Mar 2025 01:50:07 +0530
Siddharth Menon <simeddon@xxxxxxxxx> wrote:

> Update AD9832_PHASE and RES_MASK to use FIELD_PREP and GENMASK for
> clean bitmask generation and improved maintainability.
> 
> Suggested-by: Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx>
> Signed-off-by: Siddharth Menon <simeddon@xxxxxxxxx>

Hi Siddharth,
> ---
>  The previous patch would not apply cleanly as I was not working on a
>  clean branch
>  v1->v2:
>  Resolve previous patch application issues
>  
>  drivers/staging/iio/frequency/ad9832.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> index 140ee4f9c137..6e463488462a 100644
> --- a/drivers/staging/iio/frequency/ad9832.c
> +++ b/drivers/staging/iio/frequency/ad9832.c
> @@ -59,7 +59,7 @@
>  #define AD9832_CMD_SLEEPRESCLR	0xC
>  
>  #define AD9832_FREQ		BIT(11)
> -#define AD9832_PHASE(x)		(((x) & 3) << 9)
> +#define AD9832_PHASE(x)	FIELD_PREP(GENMASK(10, 9), x)
This code is mixing two styles, the sort of 'function' to set
case here and the providing of direct masks that
are suitable for inline FIELD_PREP / FIELD_GET.

I'd prefer this was changed to the second style
#define AD9832_PHASE_MASK	GENMASK(10, 9)

Then where it used inline you can use the mask directly to clear
the bits + an inline FIELD_PREP() to set them to the desired
value.  That's cleaner than current situation where the value 3 is
passed to this macro to generate the mask.

There are other cases in the code where we have the pattern:
		if (val)
			st->ctrl_ss &= ~AD9832_SELSRC;
		else
			st->ctrl_ss |= AD9832_SELSRC;
which could be rewritten as:


		st->ctrl_ss &= ~AD932_SELSRC;
		st->ctrl_ss |= FIELD_PREP(AD932_SELSRC, !!val);

the !! is needed to ensure we get 0, 1.  Could also do
val ? 1 : 0 if you prefer.

So overall a good cleanup would be to move to consistent use
of masks (including single bit ones) and FIELD_PREP to write
the values.

Jonathan


>  #define AD9832_SYNC		BIT(13)
>  #define AD9832_SELSRC		BIT(12)
>  #define AD9832_SLEEP		BIT(13)
> @@ -69,7 +69,7 @@
>  #define ADD_SHIFT		8
>  #define AD9832_FREQ_BITS	32
>  #define AD9832_PHASE_BITS	12
> -#define RES_MASK(bits)		((1 << (bits)) - 1)
> +#define RES_MASK(bits)	GENMASK((bits) - 1, 0)
>  
>  /**
>   * struct ad9832_state - driver instance specific data