Re: [PATCH v6 1/6] iio: dac: ad5686: refactor command/data macros

On Sun, 05 Jul 2026 12:38:56 +0100
Rodrigo Alencar via B4 Relay <[email protected]> wrote:

> From: Rodrigo Alencar <[email protected]>
> 
> Replace usage of bit shifting macros for FIELD_PREP(), which would not
> ignore bit masking when preparing SPI/I2C commands.
> 
> Signed-off-by: Rodrigo Alencar <[email protected]>

...

> diff --git a/drivers/iio/dac/ad5686.h b/drivers/iio/dac/ad5686.h
> index c424720f8f72..0d1bbf110926 100644
> --- a/drivers/iio/dac/ad5686.h
> +++ b/drivers/iio/dac/ad5686.h
> @@ -14,13 +14,6 @@
>  
>  #include <linux/iio/iio.h>
>  
> -#define AD5310_CMD(x)				((x) << 12)
> -
> -#define AD5683_DATA(x)				((x) << 4)
> -
> -#define AD5686_ADDR(x)				((x) << 16)
> -#define AD5686_CMD(x)				((x) << 20)
> -
>  #define AD5686_ADDR_DAC(chan)			(0x1 << (chan))
>  #define AD5686_ADDR_ALL_DAC			0xF
>  
> @@ -38,12 +31,18 @@
>  #define AD5686_CMD_CONTROL_REG			0x4
>  #define AD5686_CMD_READBACK_ENABLE_V2		0x5
>  
> +#define AD5310_CMD_MSK				GENMASK(15, 12)
> +#define AD5310_DATA_MSK				GENMASK(11, 0)

There is a sashiko bug report on the final patch that got me looking at
these - it's a false positive but took some time to figure out why
it was getting confused.  I think we need to do something to make it
more obvious what is going on..

>  #define AD5310_REF_BIT_MSK			BIT(8)

This is bit 8 of DATA - here DATA is 0 aligned so no mismatch.

>  #define AD5310_PD_MSK				GENMASK(10, 9)
>  
> +#define AD5683_DATA_MSK				GENMASK(19, 4)
>  #define AD5683_REF_BIT_MSK			BIT(12)

and this is bit 12 of AD5683_DATA rather than BIT(12) of the the whole thing
(given the 4 don't care bits.)  

Could either rename things to
#define AD5683_DATA_REF_BIT_MSK() or add a comment on the bit positions
in the overall message - which will then align with the ones of the datasheet.
where ref is DB16.

>  #define AD5683_PD_MSK				GENMASK(14, 13)
>  
> +#define AD5686_CMD_MSK				GENMASK(23, 20)
> +#define AD5686_ADDR_MSK				GENMASK(19, 16)
> +#define AD5686_DATA_MSK				GENMASK(15, 0)
>  #define AD5686_REF_BIT_MSK			BIT(0)
>  #define AD5686_PD_MSK				GENMASK(1, 0)
>