| Message ID | 20250326204901.44337-1-simeddon@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] iio: frequency: ad9832: Update bit manipulation macros to use FIELD_PREP and GENMASK | expand |
On Thu, 27 Mar 2025 01:50:07 +0530 Siddharth Menon <simeddon@gmail.com> 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@gmail.com> > Signed-off-by: Siddharth Menon <simeddon@gmail.com> 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
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) #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
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@gmail.com> Signed-off-by: Siddharth Menon <simeddon@gmail.com> --- 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(-)