Re: [PATCH v5] iio: frequency: ad9832: Use FIELD_PREP macro to set bit fields

Marcelo Schmitt
2025-03-30T15:43:49Z

Hi Siddharth,

Some suggestions in addition to what Jonathan has provided in his review.

On 03/30, Siddharth Menon wrote:
> Use bitfield and bitmask macros to clearly specify AD9832 SPI
> command fields to make register write code more readable.
>
> Suggested-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>
> Signed-off-by: Siddharth Menon <simeddon@gmail.com>
> ---
> v1->v2:
> - remove CMD_SHIFT and ADD_SHIFT
> - use GENMASK
> - store regval in an array and iterate through it
> v2->v3:
> - add missing header
> - refactor code in the previously introduced loops
> v3->v4:
> - update commit message with a better one
> - convert AD9832_PHASE and RES_MASK to masks
> - cleanup a few if else blocks
> v4->v5
> - remove unnecessary inversion (val ? 0 : 1) used
> with AD9832_PHASE_MASK introduced in v4
> - use ARRAY_SIZE instead of fixed integers
> - use reverse xmas tree order
> - align mask macros
> drivers/staging/iio/frequency/ad9832.c | 85 +++++++++++++-------------
> 1 file changed, 44 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> index 140ee4f9c137..e74d085fb4f2 100644
> --- a/drivers/staging/iio/frequency/ad9832.c
> +++ b/drivers/staging/iio/frequency/ad9832.c
> @@ -16,6 +16,9 @@
> #include <linux/slab.h>
> #include <linux/spi/spi.h>
> #include <linux/sysfs.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/unaligned.h>
>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> @@ -59,17 +62,18 @@
> #define AD9832_CMD_SLEEPRESCLR 0xC
>
> #define AD9832_FREQ BIT(11)
> -#define AD9832_PHASE(x) (((x) & 3) << 9)
> +#define AD9832_PHASE_MASK GENMASK(10, 9)
> #define AD9832_SYNC BIT(13)
> #define AD9832_SELSRC BIT(12)
> #define AD9832_SLEEP BIT(13)
> #define AD9832_RESET BIT(12)
> #define AD9832_CLR BIT(11)
> -#define CMD_SHIFT 12
> -#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)
I also don't see RES_MASK being used by ad9832 so just drop it.

> +#define AD9832_CMD_MSK GENMASK(15, 12)
> +#define AD9832_ADD_MSK GENMASK(11, 8)
> +#define AD9832_DAT_MSK GENMASK(7, 0)
>
...
> case AD9832_FREQ_SYM:
> - if (val == 1) {
> - st->ctrl_fp |= AD9832_FREQ;
> - } else if (val == 0) {
> + if (val == 1 || val == 0) {
> st->ctrl_fp &= ~AD9832_FREQ;
> + st->ctrl_fp |= FIELD_PREP(AD9832_FREQ, val ? 0 : 1);
The previous implementation would set ctrl_fp if val == 1 and unset it if val == 0.
This patch seems to be doing the reverse (setting ctrl_fp if val == 0, and
unsetting it if val != 0). Was the previous implementation potentially buggy?
If not, I think we can just do

st->ctrl_fp &= ~AD9832_FREQ;
st->ctrl_fp |= FIELD_PREP(AD9832_FREQ, !!val);

> } else {
> ret = -EINVAL;
> break;
> }
and drop the error path. Slight change in interface but guess no problem with that.

> - st->data = cpu_to_be16((AD9832_CMD_FPSELECT << CMD_SHIFT) |
> + st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_FPSELECT) |
> st->ctrl_fp);
> ret = spi_sync(st->spi, &st->msg);
> break;
> @@ -224,21 +230,18 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
> break;
> }
>
> - st->ctrl_fp &= ~AD9832_PHASE(3);
> - st->ctrl_fp |= AD9832_PHASE(val);
> + st->ctrl_fp &= ~FIELD_PREP(AD9832_PHASE_MASK, 3);
> + st->ctrl_fp |= FIELD_PREP(AD9832_PHASE_MASK, val);

This seems to be a typical modify use case as exemplified in
include/linux/bitfield.h. So,

st->ctrl_fp &= ~AD9832_PHASE_MASK;
st->ctrl_fp |= FIELD_PREP(AD9832_PHASE_MASK, val);

>
> - st->data = cpu_to_be16((AD9832_CMD_FPSELECT << CMD_SHIFT) |
> + st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_FPSELECT) |
> st->ctrl_fp);
> ret = spi_sync(st->spi, &st->msg);
> break;
> case AD9832_OUTPUT_EN:
> - if (val)
> - st->ctrl_src &= ~(AD9832_RESET | AD9832_SLEEP |
> - AD9832_CLR);
> - else
> - st->ctrl_src |= AD9832_RESET;
> + st->ctrl_src &= ~(AD9832_RESET | AD9832_SLEEP | AD9832_CLR);
> + st->ctrl_src |= FIELD_PREP(AD9832_RESET, val ? 0 : 1);
Hmm, this is modifying behavior. AD9832_SLEEP and AD9832_CLR were only being
modified if something other than 0 was written to output enable sysfs file.
Is the patch code mode appropriate than how the driver was before?

>
> - st->data = cpu_to_be16((AD9832_CMD_SLEEPRESCLR << CMD_SHIFT) |
> + st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_SLEEPRESCLR) |
> st->ctrl_src);
> ret = spi_sync(st->spi, &st->msg);
> break;

Regards,
Marcelo