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

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

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