Replace the custom frequency and phase sysfs attributes with IIO channels
using read_raw()/write_raw() callbacks, as well as removing the dds.h
header.

Changes:
- Add iio_chan_spec definitions for 2 frequency and 4 phase channels.
- Implement read_raw/write_raw for IIO_CHAN_INFO_FREQUENCY/PHASE.
- Cache frequency and phase values in driver state for readback.
- Remove dependency on dds.h macros for sysfs.
- Use guard(mutex) for cleaner locking.
- Add input validation and consistent error messages.

NOTE: This changes the userspace ABI, see cover letter.
Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>
---
 drivers/staging/iio/frequency/ad9832.c | 232 ++++++++++++++++++-------
 1 file changed, 168 insertions(+), 64 deletions(-)

diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
index e2ad3e5a7a..79d26009d1 100644
--- a/drivers/staging/iio/frequency/ad9832.c
+++ b/drivers/staging/iio/frequency/ad9832.c
@@ -9,6 +9,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/bits.h>
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/err.h>
@@ -23,10 +24,7 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
-#include "dds.h"
-
 /* Registers */
-
 #define AD9832_FREQ0LL		0x0
 #define AD9832_FREQ0HL		0x1
 #define AD9832_FREQ0LM		0x2
@@ -50,7 +48,6 @@
 #define AD9832_OUTPUT_EN	0x13
 
 /* Command Control Bits */
-
 #define AD9832_CMD_PHA8BITSW	0x1
 #define AD9832_CMD_PHA16BITSW	0x0
 #define AD9832_CMD_FRE8BITSW	0x3
@@ -79,6 +76,8 @@
  * @ctrl_fp:		cached frequency/phase control word
  * @ctrl_ss:		cached sync/selsrc control word
  * @ctrl_src:		cached sleep/reset/clr word
+ * @freq:		cached frequencies
+ * @phase:		cached phases
  * @xfer:		default spi transfer
  * @msg:		default spi message
  * @freq_xfer:		tuning word spi transfer
@@ -90,13 +89,14 @@
  * @phase_data:		tuning word spi transmit buffer
  * @freq_data:		tuning word spi transmit buffer
  */
-
 struct ad9832_state {
 	struct spi_device		*spi;
 	struct clk			*mclk;
 	unsigned short			ctrl_fp;
 	unsigned short			ctrl_ss;
 	unsigned short			ctrl_src;
+	u32				freq[2];
+	u32				phase[4];
 	struct spi_transfer		xfer;
 	struct spi_message		msg;
 	struct spi_transfer		freq_xfer[4];
@@ -115,7 +115,7 @@ struct ad9832_state {
 	} __aligned(IIO_DMA_MINALIGN);
 };
 
-static unsigned long ad9832_calc_freqreg(unsigned long mclk, unsigned long fout)
+static unsigned long ad9832_calc_freqreg(unsigned long mclk, u32 fout)
 {
 	unsigned long long freqreg = (u64)fout *
 				     (u64)((u64)1L << AD9832_FREQ_BITS);
@@ -124,12 +124,24 @@ static unsigned long ad9832_calc_freqreg(unsigned long mclk, unsigned long fout)
 }
 
 static int ad9832_write_frequency(struct ad9832_state *st,
-				  unsigned int addr, unsigned long fout)
+				  unsigned int addr, u32 fout)
 {
 	unsigned long clk_freq;
 	unsigned long regval;
 	u8 regval_bytes[4];
 	u16 freq_cmd;
+	int ret, idx;
+
+	switch (addr) {
+	case AD9832_FREQ0HM:
+		idx = 0;
+		break;
+	case AD9832_FREQ1HM:
+		idx = 1;
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	clk_freq = clk_get_rate(st->mclk);
 
@@ -147,14 +159,37 @@ static int ad9832_write_frequency(struct ad9832_state *st,
 			FIELD_PREP(AD9832_DAT_MSK, regval_bytes[i]));
 	}
 
-	return spi_sync(st->spi, &st->freq_msg);
+	ret = spi_sync(st->spi, &st->freq_msg);
+	if (ret)
+		return ret;
+
+	st->freq[idx] = fout;
+	return 0;
 }
 
 static int ad9832_write_phase(struct ad9832_state *st,
-			      unsigned long addr, unsigned long phase)
+			      unsigned long addr, u32 phase)
 {
 	u8 phase_bytes[2];
 	u16 phase_cmd;
+	int ret, idx;
+
+	switch (addr) {
+	case AD9832_PHASE0H:
+		idx = 0;
+		break;
+	case AD9832_PHASE1H:
+		idx = 1;
+		break;
+	case AD9832_PHASE2H:
+		idx = 2;
+		break;
+	case AD9832_PHASE3H:
+		idx = 3;
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	if (phase >= BIT(AD9832_PHASE_BITS))
 		return -EINVAL;
@@ -169,10 +204,77 @@ static int ad9832_write_phase(struct ad9832_state *st,
 			FIELD_PREP(AD9832_DAT_MSK, phase_bytes[i]));
 	}
 
-	return spi_sync(st->spi, &st->phase_msg);
+	ret = spi_sync(st->spi, &st->phase_msg);
+	if (ret)
+		return ret;
+
+	st->phase[idx] = phase;
+	return 0;
 }
 
-static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
+static int ad9832_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	struct ad9832_state *st = iio_priv(indio_dev);
+
+	if (val < 0)
+		return -EINVAL;
+
+	guard(mutex)(&st->lock);
+	switch (mask) {
+	case IIO_CHAN_INFO_FREQUENCY:
+		return ad9832_write_frequency(st, chan->address, val);
+	case IIO_CHAN_INFO_PHASE:
+		return ad9832_write_phase(st, chan->address, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad9832_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct ad9832_state *st = iio_priv(indio_dev);
+
+	guard(mutex)(&st->lock);
+	switch (mask) {
+	case IIO_CHAN_INFO_FREQUENCY:
+		switch (chan->address) {
+		case AD9832_FREQ0HM:
+			*val = st->freq[0];
+			return IIO_VAL_INT;
+		case AD9832_FREQ1HM:
+			*val = st->freq[1];
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_PHASE:
+		switch (chan->address) {
+		case AD9832_PHASE0H:
+			*val = st->phase[0];
+			return IIO_VAL_INT;
+		case AD9832_PHASE1H:
+			*val = st->phase[1];
+			return IIO_VAL_INT;
+		case AD9832_PHASE2H:
+			*val = st->phase[2];
+			return IIO_VAL_INT;
+		case AD9832_PHASE3H:
+			*val = st->phase[3];
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t ad9832_store(struct device *dev,
+			    struct device_attribute *attr,
 			    const char *buf, size_t len)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
@@ -183,20 +285,10 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 
 	ret = kstrtoul(buf, 10, &val);
 	if (ret)
-		goto error_ret;
+		return ret;
 
-	mutex_lock(&st->lock);
-	switch ((u32)this_attr->address) {
-	case AD9832_FREQ0HM:
-	case AD9832_FREQ1HM:
-		ret = ad9832_write_frequency(st, this_attr->address, val);
-		break;
-	case AD9832_PHASE0H:
-	case AD9832_PHASE1H:
-	case AD9832_PHASE2H:
-	case AD9832_PHASE3H:
-		ret = ad9832_write_phase(st, this_attr->address, val);
-		break;
+	guard(mutex)(&st->lock);
+	switch (this_attr->address) {
 	case AD9832_PINCTRL_EN:
 		st->ctrl_ss &= ~AD9832_SELSRC;
 		st->ctrl_ss |= FIELD_PREP(AD9832_SELSRC, val ? 0 : 1);
@@ -206,13 +298,13 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 		ret = spi_sync(st->spi, &st->msg);
 		break;
 	case AD9832_FREQ_SYM:
-		if (val == 1 || val == 0) {
-			st->ctrl_fp &= ~AD9832_FREQ;
-			st->ctrl_fp |= FIELD_PREP(AD9832_FREQ, val ? 1 : 0);
-		} else {
+		if (val != 1 && val != 0) {
 			ret = -EINVAL;
 			break;
 		}
+
+		st->ctrl_fp &= ~AD9832_FREQ;
+		st->ctrl_fp |= FIELD_PREP(AD9832_FREQ, val);
 		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_FPSELECT) |
 						  st->ctrl_fp);
 		ret = spi_sync(st->spi, &st->msg);
@@ -243,47 +335,56 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 	default:
 		ret = -ENODEV;
 	}
-	mutex_unlock(&st->lock);
 
-error_ret:
 	return ret ? ret : len;
 }
 
-/*
- * see dds.h for further information
- */
+#define AD9832_CHAN_FREQ(_channel, _addr) {			\
+	.type = IIO_ALTVOLTAGE,					\
+	.output = 1,						\
+	.indexed = 1,						\
+	.channel = _channel,					\
+	.address = _addr,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY),	\
+}
 
-static IIO_DEV_ATTR_FREQ(0, 0, 0200, NULL, ad9832_write, AD9832_FREQ0HM);
-static IIO_DEV_ATTR_FREQ(0, 1, 0200, NULL, ad9832_write, AD9832_FREQ1HM);
-static IIO_DEV_ATTR_FREQSYMBOL(0, 0200, NULL, ad9832_write, AD9832_FREQ_SYM);
-static IIO_CONST_ATTR_FREQ_SCALE(0, "1"); /* 1Hz */
+#define AD9832_CHAN_PHASE(_channel, _addr) {			\
+	.type = IIO_ALTVOLTAGE,					\
+	.output = 1,						\
+	.indexed = 1,						\
+	.channel = _channel,					\
+	.address = _addr,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_PHASE),		\
+}
+
+static const struct iio_chan_spec ad9832_channels[] = {
+	AD9832_CHAN_FREQ(0, AD9832_FREQ0HM),
+	AD9832_CHAN_FREQ(1, AD9832_FREQ1HM),
+	AD9832_CHAN_PHASE(2, AD9832_PHASE0H),
+	AD9832_CHAN_PHASE(3, AD9832_PHASE1H),
+	AD9832_CHAN_PHASE(4, AD9832_PHASE2H),
+	AD9832_CHAN_PHASE(5, AD9832_PHASE3H),
+};
 
-static IIO_DEV_ATTR_PHASE(0, 0, 0200, NULL, ad9832_write, AD9832_PHASE0H);
-static IIO_DEV_ATTR_PHASE(0, 1, 0200, NULL, ad9832_write, AD9832_PHASE1H);
-static IIO_DEV_ATTR_PHASE(0, 2, 0200, NULL, ad9832_write, AD9832_PHASE2H);
-static IIO_DEV_ATTR_PHASE(0, 3, 0200, NULL, ad9832_write, AD9832_PHASE3H);
-static IIO_DEV_ATTR_PHASESYMBOL(0, 0200, NULL,
-				ad9832_write, AD9832_PHASE_SYM);
-static IIO_CONST_ATTR_PHASE_SCALE(0, "0.0015339808"); /* 2PI/2^12 rad*/
+static IIO_CONST_ATTR(out_altvoltage_frequency_scale, "1");	   /* 1Hz */
+static IIO_CONST_ATTR(out_altvoltage_phase_scale, "0.0015339808"); /* 2PI / 2^12 rad */
 
-static IIO_DEV_ATTR_PINCONTROL_EN(0, 0200, NULL,
-				ad9832_write, AD9832_PINCTRL_EN);
-static IIO_DEV_ATTR_OUT_ENABLE(0, 0200, NULL,
-				ad9832_write, AD9832_OUTPUT_EN);
+static IIO_DEVICE_ATTR(out_altvoltage_frequencysymbol, 0200, NULL,
+		       ad9832_store, AD9832_FREQ_SYM);
+static IIO_DEVICE_ATTR(out_altvoltage_phasesymbol, 0200, NULL,
+		       ad9832_store, AD9832_PHASE_SYM);
+static IIO_DEVICE_ATTR(out_altvoltage_out_enable, 0200, NULL,
+		       ad9832_store, AD9832_OUTPUT_EN);
+static IIO_DEVICE_ATTR(out_altvoltage_pincontrol_en, 0200, NULL,
+		       ad9832_store, AD9832_PINCTRL_EN);
 
 static struct attribute *ad9832_attributes[] = {
-	&iio_dev_attr_out_altvoltage0_frequency0.dev_attr.attr,
-	&iio_dev_attr_out_altvoltage0_frequency1.dev_attr.attr,
-	&iio_const_attr_out_altvoltage0_frequency_scale.dev_attr.attr,
-	&iio_dev_attr_out_altvoltage0_phase0.dev_attr.attr,
-	&iio_dev_attr_out_altvoltage0_phase1.dev_attr.attr,
-	&iio_dev_attr_out_altvoltage0_phase2.dev_attr.attr,
-	&iio_dev_attr_out_altvoltage0_phase3.dev_attr.attr,
-	&iio_const_attr_out_altvoltage0_phase_scale.dev_attr.attr,
-	&iio_dev_attr_out_altvoltage0_pincontrol_en.dev_attr.attr,
-	&iio_dev_attr_out_altvoltage0_frequencysymbol.dev_attr.attr,
-	&iio_dev_attr_out_altvoltage0_phasesymbol.dev_attr.attr,
-	&iio_dev_attr_out_altvoltage0_out_enable.dev_attr.attr,
+	&iio_const_attr_out_altvoltage_frequency_scale.dev_attr.attr,
+	&iio_const_attr_out_altvoltage_phase_scale.dev_attr.attr,
+	&iio_dev_attr_out_altvoltage_frequencysymbol.dev_attr.attr,
+	&iio_dev_attr_out_altvoltage_phasesymbol.dev_attr.attr,
+	&iio_dev_attr_out_altvoltage_out_enable.dev_attr.attr,
+	&iio_dev_attr_out_altvoltage_pincontrol_en.dev_attr.attr,
 	NULL,
 };
 
@@ -292,6 +393,8 @@ static const struct attribute_group ad9832_attribute_group = {
 };
 
 static const struct iio_info ad9832_info = {
+	.write_raw = ad9832_write_raw,
+	.read_raw = ad9832_read_raw,
 	.attrs = &ad9832_attribute_group,
 };
 
@@ -309,15 +412,15 @@ static int ad9832_probe(struct spi_device *spi)
 
 	ret = devm_regulator_get_enable(&spi->dev, "avdd");
 	if (ret)
-		return dev_err_probe(&spi->dev, ret, "failed to enable specified AVDD voltage\n");
+		return dev_err_probe(&spi->dev, ret, "failed to enable AVDD supply\n");
 
 	ret = devm_regulator_get_enable(&spi->dev, "dvdd");
 	if (ret)
-		return dev_err_probe(&spi->dev, ret, "Failed to enable specified DVDD supply\n");
+		return dev_err_probe(&spi->dev, ret, "failed to enable DVDD supply\n");
 
 	st->mclk = devm_clk_get_enabled(&spi->dev, "mclk");
 	if (IS_ERR(st->mclk))
-		return PTR_ERR(st->mclk);
+		return dev_err_probe(&spi->dev, PTR_ERR(st->mclk), "failed to enable MCLK\n");
 
 	st->spi = spi;
 	mutex_init(&st->lock);
@@ -325,9 +428,10 @@ static int ad9832_probe(struct spi_device *spi)
 	indio_dev->name = spi_get_device_id(spi)->name;
 	indio_dev->info = &ad9832_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = ad9832_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ad9832_channels);
 
 	/* Setup default messages */
-
 	st->xfer.tx_buf = &st->data;
 	st->xfer.len = 2;
 
-- 
2.43.0

On Fri,  5 Dec 2025 17:27:42 -0300
Tomas Borquez <tomasborquez13@gmail.com> wrote:

> Replace the custom frequency and phase sysfs attributes with IIO channels
> using read_raw()/write_raw() callbacks, as well as removing the dds.h
> header.
> 
> Changes:
> - Add iio_chan_spec definitions for 2 frequency and 4 phase channels.
> - Implement read_raw/write_raw for IIO_CHAN_INFO_FREQUENCY/PHASE.
> - Cache frequency and phase values in driver state for readback.
> - Remove dependency on dds.h macros for sysfs.
> - Use guard(mutex) for cleaner locking.
> - Add input validation and consistent error messages.
> 
Hi Tomas,

> NOTE: This changes the userspace ABI, see cover letter.
Given I responded there on ABI, I'll just ignore that aspect for this review.

Code is pretty clean, but there are a few things that belong in other
patches rather than being mixed in here.
For kernel code we are pretty strict on one patch, one type of change.

> Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>
> ---
>  drivers/staging/iio/frequency/ad9832.c | 232 ++++++++++++++++++-------
>  1 file changed, 168 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> index e2ad3e5a7a..79d26009d1 100644
> --- a/drivers/staging/iio/frequency/ad9832.c
> +++ b/drivers/staging/iio/frequency/ad9832.c
> @@ -9,6 +9,7 @@
>  
>  #include <linux/bitfield.h>
>  #include <linux/bits.h>
> +#include <linux/cleanup.h>
>  #include <linux/clk.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> @@ -23,10 +24,7 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  
> -#include "dds.h"
> -
>  /* Registers */
> -
Trivial but don't make unrelated white space changes in a patch
doing anything else - they add noise and hurt reviewer speed.
Put them in a patch on their own.

>  #define AD9832_FREQ0LL		0x0
>  #define AD9832_FREQ0HL		0x1
>  #define AD9832_FREQ0LM		0x2
> @@ -50,7 +48,6 @@
>  #define AD9832_OUTPUT_EN	0x13
>  
>  /* Command Control Bits */
> -

>  	} __aligned(IIO_DMA_MINALIGN);
>  };
>  
> -static unsigned long ad9832_calc_freqreg(unsigned long mclk, unsigned long fout)
> +static unsigned long ad9832_calc_freqreg(unsigned long mclk, u32 fout)
>  {
>  	unsigned long long freqreg = (u64)fout *
>  				     (u64)((u64)1L << AD9832_FREQ_BITS);
> @@ -124,12 +124,24 @@ static unsigned long ad9832_calc_freqreg(unsigned long mclk, unsigned long fout)
>  }
>  
>  static int ad9832_write_frequency(struct ad9832_state *st,
> -				  unsigned int addr, unsigned long fout)
> +				  unsigned int addr, u32 fout)
>  {
>  	unsigned long clk_freq;
>  	unsigned long regval;
>  	u8 regval_bytes[4];
>  	u16 freq_cmd;
> +	int ret, idx;
> +
> +	switch (addr) {
> +	case AD9832_FREQ0HM:
> +		idx = 0;
> +		break;
> +	case AD9832_FREQ1HM:
> +		idx = 1;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
>  
>  	clk_freq = clk_get_rate(st->mclk);
>  
> @@ -147,14 +159,37 @@ static int ad9832_write_frequency(struct ad9832_state *st,
>  			FIELD_PREP(AD9832_DAT_MSK, regval_bytes[i]));
>  	}
>  
> -	return spi_sync(st->spi, &st->freq_msg);
> +	ret = spi_sync(st->spi, &st->freq_msg);
> +	if (ret)
> +		return ret;
> +
> +	st->freq[idx] = fout;

I'd put a blank line here.  Generally slightly helps readability before
simple return statements like this.

> +	return 0;
>  }
>  
>  static int ad9832_write_phase(struct ad9832_state *st,
> -			      unsigned long addr, unsigned long phase)
> +			      unsigned long addr, u32 phase)
>  {
>  	u8 phase_bytes[2];
>  	u16 phase_cmd;
> +	int ret, idx;
> +
> +	switch (addr) {
> +	case AD9832_PHASE0H:
> +		idx = 0;
> +		break;
> +	case AD9832_PHASE1H:
> +		idx = 1;
> +		break;
> +	case AD9832_PHASE2H:
> +		idx = 2;
> +		break;
> +	case AD9832_PHASE3H:
> +		idx = 3;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
>  
>  	if (phase >= BIT(AD9832_PHASE_BITS))
>  		return -EINVAL;
> @@ -169,10 +204,77 @@ static int ad9832_write_phase(struct ad9832_state *st,
>  			FIELD_PREP(AD9832_DAT_MSK, phase_bytes[i]));
>  	}
>  
> -	return spi_sync(st->spi, &st->phase_msg);
> +	ret = spi_sync(st->spi, &st->phase_msg);
> +	if (ret)
> +		return ret;
> +
> +	st->phase[idx] = phase;
> +	return 0;
>  }
>  
> -static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
> +static int ad9832_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct ad9832_state *st = iio_priv(indio_dev);
> +
> +	if (val < 0)
> +		return -EINVAL;

Check val2 as well.  Should be zero.


> +
> +	guard(mutex)(&st->lock);
> +	switch (mask) {
> +	case IIO_CHAN_INFO_FREQUENCY:
> +		return ad9832_write_frequency(st, chan->address, val);
> +	case IIO_CHAN_INFO_PHASE:
> +		return ad9832_write_phase(st, chan->address, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +static ssize_t ad9832_store(struct device *dev,
> +			    struct device_attribute *attr,
>  			    const char *buf, size_t len)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> @@ -183,20 +285,10 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
>  
>  	ret = kstrtoul(buf, 10, &val);
>  	if (ret)
> -		goto error_ret;
> +		return ret;
>  
> -	mutex_lock(&st->lock);
> -	switch ((u32)this_attr->address) {
> -	case AD9832_FREQ0HM:
> -	case AD9832_FREQ1HM:
> -		ret = ad9832_write_frequency(st, this_attr->address, val);
> -		break;
> -	case AD9832_PHASE0H:
> -	case AD9832_PHASE1H:
> -	case AD9832_PHASE2H:
> -	case AD9832_PHASE3H:
> -		ret = ad9832_write_phase(st, this_attr->address, val);
> -		break;
> +	guard(mutex)(&st->lock);

Ideally do guard() changes in a separate patch as seems unrelated to the
other stuff going on here. I'd suggest making that change first.

> +	switch (this_attr->address) {
>  	case AD9832_PINCTRL_EN:
>  		st->ctrl_ss &= ~AD9832_SELSRC;
>  		st->ctrl_ss |= FIELD_PREP(AD9832_SELSRC, val ? 0 : 1);
> @@ -206,13 +298,13 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
>  		ret = spi_sync(st->spi, &st->msg);
>  		break;
>  	case AD9832_FREQ_SYM:
> -		if (val == 1 || val == 0) {
> -			st->ctrl_fp &= ~AD9832_FREQ;
> -			st->ctrl_fp |= FIELD_PREP(AD9832_FREQ, val ? 1 : 0);
> -		} else {
> +		if (val != 1 && val != 0) {
>  			ret = -EINVAL;

With guard, should be able to directly return in error cases
simplifying the flow and helping code readability.  That's one of the
nicest things guard() enables.

>  			break;
>  		}
> +
> +		st->ctrl_fp &= ~AD9832_FREQ;
> +		st->ctrl_fp |= FIELD_PREP(AD9832_FREQ, val);
>  		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_FPSELECT) |
>  						  st->ctrl_fp);
>  		ret = spi_sync(st->spi, &st->msg);
> @@ -243,47 +335,56 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
>  	default:
>  		ret = -ENODEV;
>  	}
> -	mutex_unlock(&st->lock);
>  
> -error_ret:
>  	return ret ? ret : len;
>  }

>  
> @@ -309,15 +412,15 @@ static int ad9832_probe(struct spi_device *spi)
>  
>  	ret = devm_regulator_get_enable(&spi->dev, "avdd");
>  	if (ret)
> -		return dev_err_probe(&spi->dev, ret, "failed to enable specified AVDD voltage\n");
> +		return dev_err_probe(&spi->dev, ret, "failed to enable AVDD supply\n");
>  
>  	ret = devm_regulator_get_enable(&spi->dev, "dvdd");
>  	if (ret)
> -		return dev_err_probe(&spi->dev, ret, "Failed to enable specified DVDD supply\n");
> +		return dev_err_probe(&spi->dev, ret, "failed to enable DVDD supply\n");
>  
>  	st->mclk = devm_clk_get_enabled(&spi->dev, "mclk");
>  	if (IS_ERR(st->mclk))
> -		return PTR_ERR(st->mclk);
> +		return dev_err_probe(&spi->dev, PTR_ERR(st->mclk), "failed to enable MCLK\n");
This is an unrelated change.  Do it in a separate patch for just the dev_err_probe()
usage.

>