[PATCH v2 0/6] staging: ad9832: driver cleanup

[PATCH v2 0/6] staging: ad9832: driver cleanup

[Tomas Borquez]

This series cleans up the ad9832 driver with the goal of (eventually)
graduating it from staging. The main change is converting custom sysfs
attributes to IIO channel interface and adding sysfs documentation.

Changes since v1:
  - Reordered patches: moved dev_err_probe cleanup before guard(mutex)
    conversion (Marcelo)
  - Split channel conversion into two patches: 
      - dds.h removal
      - channel interface conversion (ABI change) (Jonathan)
  - Added devm_mutex_init() conversion as separate patch (Marcelo)
  - Used MICRO macro for phase calculations instead of hardcoded values
    for better readability (Marcelo)
  - Fixed FIELD_PREP usage in pinctrl_en for consistency (Marcelo)
  - Renamed ext_info macro parameter from _channel to _select since
    it represents register selection not channel number (Marcelo)
  - Ensured patches apply cleanly on top of IIO testing branch
  - Made sysfs ABI documentation generic using capital letters for
    channel and symbol indices (Marcelo, Jonathan)
  - Changed attribute naming to hierarchical structure 
    (out_altcurrentY_frequency_symbolZ) (Marcelo)
  - Made descriptions hardware-agnostic and removed chip-specific
    implementation details (Jonathan)
  - Renamed documentation file to sysfs-bus-iio-frequency instead of
    chip-specific name (Jonathan)

Tomas Borquez (6):
  staging: iio: ad9832: cleanup dev_err_probe()
  staging: iio: ad9832: convert to guard(mutex)
  staging: iio: ad9832: convert to devm_mutex_init()
  staging: iio: ad9832: remove dds.h dependency
  staging: iio: ad9832: convert to iio channels and ext_info attrs
  staging: iio: ad9832: add sysfs documentation

 .../iio/Documentation/sysfs-bus-iio-frequency |  40 +++
 drivers/staging/iio/frequency/ad9832.c        | 327 +++++++++++++-----
 2 files changed, 279 insertions(+), 88 deletions(-)
 create mode 100644 drivers/staging/iio/Documentation/sysfs-bus-iio-frequency

-- 
2.43.0

[PATCH v2 1/6] staging: iio: ad9832: cleanup dev_err_probe()

[Tomas Borquez]

Cleanup dev_err_probe() by keeping messages consistent and adding
error message for clock acquisition failure.

Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>
---
 drivers/staging/iio/frequency/ad9832.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
index b87ea1781b27..a5523e2210bf 100644
--- a/drivers/staging/iio/frequency/ad9832.c
+++ b/drivers/staging/iio/frequency/ad9832.c
@@ -293,27 +293,28 @@ static const struct iio_info ad9832_info = {
 
 static int ad9832_probe(struct spi_device *spi)
 {
+	struct device *dev = &spi->dev;
 	struct iio_dev *indio_dev;
 	struct ad9832_state *st;
 	int ret;
 
-	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
 	if (!indio_dev)
 		return -ENOMEM;
 
 	st = iio_priv(indio_dev);
 
-	ret = devm_regulator_get_enable(&spi->dev, "avdd");
+	ret = devm_regulator_get_enable(dev, "avdd");
 	if (ret)
-		return dev_err_probe(&spi->dev, ret, "failed to enable specified AVDD voltage\n");
+		return dev_err_probe(dev, ret, "failed to enable AVDD supply\n");
 
-	ret = devm_regulator_get_enable(&spi->dev, "dvdd");
+	ret = devm_regulator_get_enable(dev, "dvdd");
 	if (ret)
-		return dev_err_probe(&spi->dev, ret, "Failed to enable specified DVDD supply\n");
+		return dev_err_probe(dev, ret, "failed to enable DVDD supply\n");
 
-	st->mclk = devm_clk_get_enabled(&spi->dev, "mclk");
+	st->mclk = devm_clk_get_enabled(dev, "mclk");
 	if (IS_ERR(st->mclk))
-		return PTR_ERR(st->mclk);
+		return dev_err_probe(dev, PTR_ERR(st->mclk), "failed to enable MCLK\n");
 
 	st->spi = spi;
 	mutex_init(&st->lock);
-- 
2.43.0

[PATCH v2 2/6] staging: iio: ad9832: convert to guard(mutex)

[Tomas Borquez]

Use guard(mutex) for cleaner lock handling and simpler error paths.

Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>
---
 drivers/staging/iio/frequency/ad9832.c | 41 +++++++++++---------------
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
index a5523e2210bf..35d8d51d5c2b 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>
@@ -179,20 +180,20 @@ 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);
+	guard(mutex)(&st->lock);
 	switch ((u32)this_attr->address) {
 	case AD9832_FREQ0HM:
 	case AD9832_FREQ1HM:
 		ret = ad9832_write_frequency(st, this_attr->address, val);
-		break;
+		return ret ?: len;
 	case AD9832_PHASE0H:
 	case AD9832_PHASE1H:
 	case AD9832_PHASE2H:
 	case AD9832_PHASE3H:
 		ret = ad9832_write_phase(st, this_attr->address, val);
-		break;
+		return ret ?: len;
 	case AD9832_PINCTRL_EN:
 		st->ctrl_ss &= ~AD9832_SELSRC;
 		st->ctrl_ss |= FIELD_PREP(AD9832_SELSRC, val ? 0 : 1);
@@ -200,24 +201,20 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_SYNCSELSRC) |
 						  st->ctrl_ss);
 		ret = spi_sync(st->spi, &st->msg);
-		break;
+		return ret ?: len;
 	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 {
-			ret = -EINVAL;
-			break;
-		}
+		if (val != 1 && val != 0)
+			return -EINVAL;
+
+		st->ctrl_fp &= ~AD9832_FREQ;
+		st->ctrl_fp |= FIELD_PREP(AD9832_FREQ, val ? 1 : 0);
 		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_FPSELECT) |
 						  st->ctrl_fp);
 		ret = spi_sync(st->spi, &st->msg);
-		break;
+		return ret ?: len;
 	case AD9832_PHASE_SYM:
-		if (val > 3) {
-			ret = -EINVAL;
-			break;
-		}
+		if (val > 3)
+			return -EINVAL;
 
 		st->ctrl_fp &= ~AD9832_PHASE_MASK;
 		st->ctrl_fp |= FIELD_PREP(AD9832_PHASE_MASK, val);
@@ -225,7 +222,7 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_FPSELECT) |
 						  st->ctrl_fp);
 		ret = spi_sync(st->spi, &st->msg);
-		break;
+		return ret ?: len;
 	case AD9832_OUTPUT_EN:
 		if (val)
 			st->ctrl_src &= ~(AD9832_RESET | AD9832_SLEEP | AD9832_CLR);
@@ -235,14 +232,10 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_SLEEPRESCLR) |
 						  st->ctrl_src);
 		ret = spi_sync(st->spi, &st->msg);
-		break;
+		return ret ?: len;
 	default:
-		ret = -ENODEV;
+		return -ENODEV;
 	}
-	mutex_unlock(&st->lock);
-
-error_ret:
-	return ret ? ret : len;
 }
 
 /*
-- 
2.43.0

[PATCH v2 3/6] staging: iio: ad9832: convert to devm_mutex_init()

[Tomas Borquez]

Use devm_mutex_init() for automatic resource cleanup when the device
is removed or probe fails.

Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>
---
 drivers/staging/iio/frequency/ad9832.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
index 35d8d51d5c2b..4bb203a67046 100644
--- a/drivers/staging/iio/frequency/ad9832.c
+++ b/drivers/staging/iio/frequency/ad9832.c
@@ -310,7 +310,9 @@ static int ad9832_probe(struct spi_device *spi)
 		return dev_err_probe(dev, PTR_ERR(st->mclk), "failed to enable MCLK\n");
 
 	st->spi = spi;
-	mutex_init(&st->lock);
+	ret = devm_mutex_init(&spi->dev, &st->lock);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to initialize mutex\n");
 
 	indio_dev->name = spi_get_device_id(spi)->name;
 	indio_dev->info = &ad9832_info;
-- 
2.43.0

[PATCH v2 4/6] staging: iio: ad9832: remove dds.h dependency

[Tomas Borquez]

Remove dependency on dds.h by converting custom macros to standard IIO
attribute declarations.

Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>
---
 drivers/staging/iio/frequency/ad9832.c | 37 +++++++++++---------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
index 4bb203a67046..aa78973c3a3c 100644
--- a/drivers/staging/iio/frequency/ad9832.c
+++ b/drivers/staging/iio/frequency/ad9832.c
@@ -24,8 +24,6 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
-#include "dds.h"
-
 /* Registers */
 #define AD9832_FREQ0LL		0x0
 #define AD9832_FREQ0HL		0x1
@@ -238,27 +236,22 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 	}
 }
 
-/*
- * see dds.h for further information
- */
+static IIO_DEVICE_ATTR(out_altvoltage0_frequency0, 0200, NULL, ad9832_write, AD9832_FREQ0HM);
+static IIO_DEVICE_ATTR(out_altvoltage0_frequency1, 0200, NULL, ad9832_write, AD9832_FREQ1HM);
+
+static IIO_DEVICE_ATTR(out_altvoltage0_frequencysymbol, 0200, NULL, ad9832_write, AD9832_FREQ_SYM);
+static IIO_CONST_ATTR(out_altvoltage0_frequency_scale, "1"); /* 1Hz */
+
+static IIO_DEVICE_ATTR(out_altvoltage0_phase0, 0200, NULL, ad9832_write, AD9832_PHASE0H);
+static IIO_DEVICE_ATTR(out_altvoltage0_phase1, 0200, NULL, ad9832_write, AD9832_PHASE1H);
+static IIO_DEVICE_ATTR(out_altvoltage0_phase2, 0200, NULL, ad9832_write, AD9832_PHASE2H);
+static IIO_DEVICE_ATTR(out_altvoltage0_phase3, 0200, NULL, ad9832_write, AD9832_PHASE3H);
+
+static IIO_DEVICE_ATTR(out_altvoltage0_phasesymbol, 0200, NULL, ad9832_write, AD9832_PHASE_SYM);
+static IIO_CONST_ATTR(out_altvoltage0_phase_scale, "0.0015339808"); /* 2PI/2^12 rad */
 
-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 */
-
-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_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_altvoltage0_pincontrol_en, 0200, NULL, ad9832_write, AD9832_PINCTRL_EN);
+static IIO_DEVICE_ATTR(out_altvoltage0_out_enable, 0200, NULL, ad9832_write, AD9832_OUTPUT_EN);
 
 static struct attribute *ad9832_attributes[] = {
 	&iio_dev_attr_out_altvoltage0_frequency0.dev_attr.attr,
-- 
2.43.0

[PATCH v2 5/6] staging: iio: ad9832: convert to iio channels and ext_info attrs

[Tomas Borquez]

Convert ad9832 from sysfs attributes to standard channel interface
using a single IIO_ALTCURRENT channel with ext_info attributes, as this
device is a current source DAC with one output.

Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>
---
 drivers/staging/iio/frequency/ad9832.c | 278 +++++++++++++++++++------
 1 file changed, 220 insertions(+), 58 deletions(-)

diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
index aa78973c3a3c..47b41b9eb14f 100644
--- a/drivers/staging/iio/frequency/ad9832.c
+++ b/drivers/staging/iio/frequency/ad9832.c
@@ -20,6 +20,7 @@
 #include <linux/spi/spi.h>
 #include <linux/sysfs.h>
 #include <linux/unaligned.h>
+#include <linux/units.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -64,6 +65,7 @@
 #define AD9832_CLR		BIT(11)
 #define AD9832_FREQ_BITS	32
 #define AD9832_PHASE_BITS	12
+#define AD9832_2PI_URAD		6283185UL
 #define AD9832_CMD_MSK		GENMASK(15, 12)
 #define AD9832_ADD_MSK		GENMASK(11, 8)
 #define AD9832_DAT_MSK		GENMASK(7, 0)
@@ -75,6 +77,12 @@
  * @ctrl_fp:		cached frequency/phase control word
  * @ctrl_ss:		cached sync/selsrc control word
  * @ctrl_src:		cached sleep/reset/clr word
+ * @freq:		cached frequencies
+ * @freq_sym:		cached frequency symbol selection
+ * @phase:		cached phases
+ * @phase_sym:		cached phase symbol selection
+ * @output_en:		cached output enable state
+ * @pinctrl_en:		cached pinctrl enable state
  * @xfer:		default spi transfer
  * @msg:		default spi message
  * @freq_xfer:		tuning word spi transfer
@@ -92,6 +100,12 @@ struct ad9832_state {
 	unsigned short			ctrl_fp;
 	unsigned short			ctrl_ss;
 	unsigned short			ctrl_src;
+	u32				freq[2];
+	bool				freq_sym;
+	u32				phase[4];
+	u32				phase_sym;
+	bool				output_en;
+	bool				pinctrl_en;
 	struct spi_transfer		xfer;
 	struct spi_message		msg;
 	struct spi_transfer		freq_xfer[4];
@@ -110,7 +124,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);
@@ -118,19 +132,33 @@ static unsigned long ad9832_calc_freqreg(unsigned long mclk, unsigned long fout)
 	return freqreg;
 }
 
-static int ad9832_write_frequency(struct ad9832_state *st,
-				  unsigned int addr, unsigned long fout)
+static ssize_t ad9832_write_frequency(struct iio_dev *indio_dev,
+				      uintptr_t private,
+				      struct iio_chan_spec const *chan,
+				      const char *buf, size_t len)
 {
+	struct ad9832_state *st = iio_priv(indio_dev);
 	unsigned long clk_freq;
 	unsigned long regval;
 	u8 regval_bytes[4];
 	u16 freq_cmd;
+	u32 fout, addr;
+	int ret;
 
-	clk_freq = clk_get_rate(st->mclk);
+	if (private > 1)
+		return -EINVAL;
+
+	addr = (private == 0) ? AD9832_FREQ0HM : AD9832_FREQ1HM;
+
+	ret = kstrtou32(buf, 10, &fout);
+	if (ret)
+		return ret;
 
+	clk_freq = clk_get_rate(st->mclk);
 	if (!clk_freq || fout > (clk_freq / 2))
 		return -EINVAL;
 
+	guard(mutex)(&st->lock);
 	regval = ad9832_calc_freqreg(clk_freq, fout);
 	put_unaligned_be32(regval, regval_bytes);
 
@@ -142,32 +170,102 @@ 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[private] = fout;
+
+	return len;
+}
+
+static ssize_t ad9832_read_frequency(struct iio_dev *indio_dev,
+				     uintptr_t private,
+				     struct iio_chan_spec const *chan,
+				     char *buf)
+{
+	struct ad9832_state *st = iio_priv(indio_dev);
+	u32 val;
+
+	if (private > 1)
+		return -EINVAL;
+
+	guard(mutex)(&st->lock);
+	val = st->freq[private];
+
+	return sysfs_emit(buf, "%u\n", val);
 }
 
-static int ad9832_write_phase(struct ad9832_state *st,
-			      unsigned long addr, unsigned long phase)
+static const u32 ad9832_phase_addr[] = {
+	AD9832_PHASE0H, AD9832_PHASE1H, AD9832_PHASE2H, AD9832_PHASE3H
+};
+
+static ssize_t ad9832_write_phase(struct iio_dev *indio_dev,
+				  uintptr_t private,
+				  struct iio_chan_spec const *chan,
+				  const char *buf, size_t len)
 {
+	struct ad9832_state *st = iio_priv(indio_dev);
 	u8 phase_bytes[2];
 	u16 phase_cmd;
+	u32 phase_urad, phase;
+	int val, val2, ret;
 
-	if (phase >= BIT(AD9832_PHASE_BITS))
+	if (private >= ARRAY_SIZE(ad9832_phase_addr))
 		return -EINVAL;
 
-	put_unaligned_be16(phase, phase_bytes);
+	ret = iio_str_to_fixpoint(buf, 100000, &val, &val2);
+	if (ret)
+		return ret;
+
+	if (val < 0 || val2 < 0)
+		return -EINVAL;
+
+	phase_urad = val * MICRO + val2;
+	if (phase_urad >= AD9832_2PI_URAD)
+		return -EINVAL;
 
+	/* Convert microradians to 12-bit phase register value (0 to 4095) */
+	phase = ((u64)phase_urad << AD9832_PHASE_BITS) / AD9832_2PI_URAD;
+
+	guard(mutex)(&st->lock);
+	put_unaligned_be16(phase, phase_bytes);
 	for (int i = 0; i < ARRAY_SIZE(phase_bytes); i++) {
 		phase_cmd = (i % 2 == 0) ? AD9832_CMD_PHA8BITSW : AD9832_CMD_PHA16BITSW;
 
 		st->phase_data[i] = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, phase_cmd) |
-			FIELD_PREP(AD9832_ADD_MSK, addr - i) |
+			FIELD_PREP(AD9832_ADD_MSK, ad9832_phase_addr[private] - i) |
 			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[private] = phase;
+
+	return len;
 }
 
-static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
+static ssize_t ad9832_read_phase(struct iio_dev *indio_dev,
+				 uintptr_t private,
+				 struct iio_chan_spec const *chan,
+				 char *buf)
+{
+	struct ad9832_state *st = iio_priv(indio_dev);
+	u32 phase_urad;
+
+	if (private >= ARRAY_SIZE(ad9832_phase_addr))
+		return -EINVAL;
+
+	guard(mutex)(&st->lock);
+	phase_urad = ((u64)st->phase[private] * AD9832_2PI_URAD) >> AD9832_PHASE_BITS;
+
+	return sysfs_emit(buf, "%u.%06u\n", phase_urad / (u32)MICRO, phase_urad % (u32)MICRO);
+}
+
+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);
@@ -181,25 +279,7 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 		return ret;
 
 	guard(mutex)(&st->lock);
-	switch ((u32)this_attr->address) {
-	case AD9832_FREQ0HM:
-	case AD9832_FREQ1HM:
-		ret = ad9832_write_frequency(st, this_attr->address, val);
-		return ret ?: len;
-	case AD9832_PHASE0H:
-	case AD9832_PHASE1H:
-	case AD9832_PHASE2H:
-	case AD9832_PHASE3H:
-		ret = ad9832_write_phase(st, this_attr->address, val);
-		return ret ?: len;
-	case AD9832_PINCTRL_EN:
-		st->ctrl_ss &= ~AD9832_SELSRC;
-		st->ctrl_ss |= FIELD_PREP(AD9832_SELSRC, val ? 0 : 1);
-
-		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_SYNCSELSRC) |
-						  st->ctrl_ss);
-		ret = spi_sync(st->spi, &st->msg);
-		return ret ?: len;
+	switch (this_attr->address) {
 	case AD9832_FREQ_SYM:
 		if (val != 1 && val != 0)
 			return -EINVAL;
@@ -209,7 +289,11 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_FPSELECT) |
 						  st->ctrl_fp);
 		ret = spi_sync(st->spi, &st->msg);
-		return ret ?: len;
+		if (ret)
+			return ret;
+
+		st->freq_sym = val;
+		break;
 	case AD9832_PHASE_SYM:
 		if (val > 3)
 			return -EINVAL;
@@ -220,8 +304,15 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_FPSELECT) |
 						  st->ctrl_fp);
 		ret = spi_sync(st->spi, &st->msg);
-		return ret ?: len;
+		if (ret)
+			return ret;
+
+		st->phase_sym = val;
+		break;
 	case AD9832_OUTPUT_EN:
+		if (val != 1 && val != 0)
+			return -EINVAL;
+
 		if (val)
 			st->ctrl_src &= ~(AD9832_RESET | AD9832_SLEEP | AD9832_CLR);
 		else
@@ -230,42 +321,111 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_SLEEPRESCLR) |
 						  st->ctrl_src);
 		ret = spi_sync(st->spi, &st->msg);
-		return ret ?: len;
+		if (ret)
+			return ret;
+
+		st->output_en = val;
+		break;
+	case AD9832_PINCTRL_EN:
+		if (val != 1 && val != 0)
+			return -EINVAL;
+
+		st->ctrl_ss &= ~AD9832_SELSRC;
+		st->ctrl_ss |= FIELD_PREP(AD9832_SELSRC, !val);
+
+		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_SYNCSELSRC) |
+						 st->ctrl_ss);
+		ret = spi_sync(st->spi, &st->msg);
+		if (ret)
+			return ret;
+
+		st->pinctrl_en = val;
+		break;
 	default:
 		return -ENODEV;
 	}
+
+	return len;
 }
 
-static IIO_DEVICE_ATTR(out_altvoltage0_frequency0, 0200, NULL, ad9832_write, AD9832_FREQ0HM);
-static IIO_DEVICE_ATTR(out_altvoltage0_frequency1, 0200, NULL, ad9832_write, AD9832_FREQ1HM);
+static ssize_t ad9832_show(struct device *dev,
+			   struct device_attribute *attr,
+			   char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct ad9832_state *st = iio_priv(indio_dev);
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 
-static IIO_DEVICE_ATTR(out_altvoltage0_frequencysymbol, 0200, NULL, ad9832_write, AD9832_FREQ_SYM);
-static IIO_CONST_ATTR(out_altvoltage0_frequency_scale, "1"); /* 1Hz */
+	guard(mutex)(&st->lock);
+	switch (this_attr->address) {
+	case AD9832_FREQ_SYM:
+		return sysfs_emit(buf, "%u\n", st->freq_sym);
+	case AD9832_PHASE_SYM:
+		return sysfs_emit(buf, "%u\n", st->phase_sym);
+	case AD9832_OUTPUT_EN:
+		return sysfs_emit(buf, "%u\n", st->output_en);
+	case AD9832_PINCTRL_EN:
+		return sysfs_emit(buf, "%u\n", st->pinctrl_en);
+	default:
+		return -ENODEV;
+	}
+}
 
-static IIO_DEVICE_ATTR(out_altvoltage0_phase0, 0200, NULL, ad9832_write, AD9832_PHASE0H);
-static IIO_DEVICE_ATTR(out_altvoltage0_phase1, 0200, NULL, ad9832_write, AD9832_PHASE1H);
-static IIO_DEVICE_ATTR(out_altvoltage0_phase2, 0200, NULL, ad9832_write, AD9832_PHASE2H);
-static IIO_DEVICE_ATTR(out_altvoltage0_phase3, 0200, NULL, ad9832_write, AD9832_PHASE3H);
+#define AD9832_CHAN_FREQ(_name, _select) {			\
+	.name = _name,						\
+	.write = ad9832_write_frequency,			\
+	.read = ad9832_read_frequency,				\
+	.private = _select,					\
+	.shared = IIO_SEPARATE,					\
+}
+
+#define AD9832_CHAN_PHASE(_name, _select) {			\
+	.name = _name,						\
+	.write = ad9832_write_phase,				\
+	.read = ad9832_read_phase,				\
+	.private = _select,					\
+	.shared = IIO_SEPARATE,					\
+}
 
-static IIO_DEVICE_ATTR(out_altvoltage0_phasesymbol, 0200, NULL, ad9832_write, AD9832_PHASE_SYM);
-static IIO_CONST_ATTR(out_altvoltage0_phase_scale, "0.0015339808"); /* 2PI/2^12 rad */
+static const struct iio_chan_spec_ext_info ad9832_ext_info[] = {
+	AD9832_CHAN_FREQ("frequency0", 0),
+	AD9832_CHAN_FREQ("frequency1", 1),
+	AD9832_CHAN_PHASE("phase0", 0),
+	AD9832_CHAN_PHASE("phase1", 1),
+	AD9832_CHAN_PHASE("phase2", 2),
+	AD9832_CHAN_PHASE("phase3", 3),
+	{ }
+};
 
-static IIO_DEVICE_ATTR(out_altvoltage0_pincontrol_en, 0200, NULL, ad9832_write, AD9832_PINCTRL_EN);
-static IIO_DEVICE_ATTR(out_altvoltage0_out_enable, 0200, NULL, ad9832_write, AD9832_OUTPUT_EN);
+static const struct iio_chan_spec ad9832_channels[] = {
+	{
+		.type = IIO_ALTCURRENT,
+		.output = 1,
+		.indexed = 1,
+		.channel = 0,
+		.ext_info = ad9832_ext_info,
+	},
+};
+
+static IIO_DEVICE_ATTR(out_altcurrent0_frequency_symbol, 0644,
+		       ad9832_show, ad9832_store, AD9832_FREQ_SYM);
+static IIO_DEVICE_ATTR(out_altcurrent0_phase_symbol, 0644,
+		       ad9832_show, ad9832_store, AD9832_PHASE_SYM);
+static IIO_DEVICE_ATTR(out_altcurrent0_enable, 0644,
+		       ad9832_show, ad9832_store, AD9832_OUTPUT_EN);
+
+/*
+ * TODO: Convert to DT property when graduating from staging.
+ * Pin control configuration depends on hardware wiring.
+ */
+static IIO_DEVICE_ATTR(out_altcurrent0_pincontrol_en, 0644,
+		       ad9832_show, 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_dev_attr_out_altcurrent0_frequency_symbol.dev_attr.attr,
+	&iio_dev_attr_out_altcurrent0_phase_symbol.dev_attr.attr,
+	&iio_dev_attr_out_altcurrent0_enable.dev_attr.attr,
+	&iio_dev_attr_out_altcurrent0_pincontrol_en.dev_attr.attr,
 	NULL,
 };
 
@@ -310,6 +470,8 @@ 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;
-- 
2.43.0

[PATCH v2 6/6] staging: iio: ad9832: add sysfs documentation

[Tomas Borquez]

Add sysfs ABI documentation for the AD9832/AD9835 Direct Digital
Synthesizer chips, documenting frequency, phase, output control,
and pin control attributes.

Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>
---
 .../iio/Documentation/sysfs-bus-iio-frequency | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 drivers/staging/iio/Documentation/sysfs-bus-iio-frequency

diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-frequency b/drivers/staging/iio/Documentation/sysfs-bus-iio-frequency
new file mode 100644
index 000000000000..10627c19bdb7
--- /dev/null
+++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-frequency
@@ -0,0 +1,40 @@
+What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrentY_frequencyZ
+KernelVersion:	6.19
+Contact:	linux-iio@vger.kernel.org
+Description:
+    Frequency in Hz for symbol Z of channel Y. The active
+    frequency symbol is selected via out_altcurrentY_frequency_symbol.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrentY_phaseZ
+KernelVersion:	6.19
+Contact:	linux-iio@vger.kernel.org
+Description:
+    Phase offset in radians for symbol Z of channel Y. Valid range
+    is 0 to 2*PI (exclusive). The active phase symbol is selected
+    via out_altcurrentY_phase_symbol.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrentY_frequency_symbol
+KernelVersion:	6.19
+Contact:	linux-iio@vger.kernel.org
+Description:
+    Selects which frequency symbol is active for channel Y.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrentY_phase_symbol
+KernelVersion:	6.19
+Contact:	linux-iio@vger.kernel.org
+Description:
+    Selects which phase symbol is active for channel Y.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrentY_enable
+KernelVersion:	6.19
+Contact:	linux-iio@vger.kernel.org
+Description:
+    Enables (1) or disables (0) the output for channel Y.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrentY_pincontrol_en
+KernelVersion:	6.19
+Contact:	linux-iio@vger.kernel.org
+Description:
+    Enables (1) or disables (0) hardware pin control for frequency
+    and phase symbol selection on channel Y. When enabled, external
+    pins control symbol selection instead of software.
-- 
2.43.0

Re: [PATCH v2 4/6] staging: iio: ad9832: remove dds.h dependency

[Andy Shevchenko]

On Tue, Dec 30, 2025 at 10:35 PM Tomas Borquez <tomasborquez13@gmail.com> wrote:
>
> Remove dependency on dds.h by converting custom macros to standard IIO
> attribute declarations.


> +static IIO_DEVICE_ATTR(out_altvoltage0_frequency0, 0200, NULL, ad9832_write, AD9832_FREQ0HM);
> +static IIO_DEVICE_ATTR(out_altvoltage0_frequency1, 0200, NULL, ad9832_write, AD9832_FREQ1HM);

Any particular point in not using _WO() / _RO() variants of the
IIO_DEVICE_ATTR_*() macros?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/6] staging: iio: ad9832: cleanup dev_err_probe()

[Andy Shevchenko]

On Tue, Dec 30, 2025 at 10:35 PM Tomas Borquez <tomasborquez13@gmail.com> wrote:
>
> Cleanup dev_err_probe() by keeping messages consistent and adding
> error message for clock acquisition failure.

AFAICS this does two things, the second one is reuse of the temporary
variable for struct device.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/6] staging: iio: ad9832: convert to guard(mutex)

[Andy Shevchenko]

On Tue, Dec 30, 2025 at 10:35 PM Tomas Borquez <tomasborquez13@gmail.com> wrote:
>
> Use guard(mutex) for cleaner lock handling and simpler error paths.

...

Don't remember if it was in the previous rounds of review or somewhere
else, but Jonathan (? IIRC) suggestd to use

  ret = foo(...);
  if (ret)
   return ret;
  break;
  ...
  return len;

However, this one just duplicates what is already in use. Have you
checked the bloat-o-meter before and after and see if there is any
difference in the compiled object file?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/6] staging: iio: ad9832: convert to iio channels and ext_info attrs

[Andy Shevchenko]

On Tue, Dec 30, 2025 at 10:35 PM Tomas Borquez <tomasborquez13@gmail.com> wrote:
>
> Convert ad9832 from sysfs attributes to standard channel interface
> using a single IIO_ALTCURRENT channel with ext_info attributes, as this
> device is a current source DAC with one output.

...

> -static int ad9832_write_frequency(struct ad9832_state *st,
> -                                 unsigned int addr, unsigned long fout)
> +static ssize_t ad9832_write_frequency(struct iio_dev *indio_dev,
> +                                     uintptr_t private,

Torvalds said that uintptr_t shouldn't be used in the Linux kernel,
the unsigned long suffice and enough. Why do we need it here?



> +static IIO_DEVICE_ATTR(out_altcurrent0_frequency_symbol, 0644,
> +                      ad9832_show, ad9832_store, AD9832_FREQ_SYM);
> +static IIO_DEVICE_ATTR(out_altcurrent0_phase_symbol, 0644,
> +                      ad9832_show, ad9832_store, AD9832_PHASE_SYM);
> +static IIO_DEVICE_ATTR(out_altcurrent0_enable, 0644,
> +                      ad9832_show, ad9832_store, AD9832_OUTPUT_EN);

Why not IIO_DEVICE_ATTR_RW()?

...

> +       &iio_dev_attr_out_altcurrent0_frequency_symbol.dev_attr.attr,
> +       &iio_dev_attr_out_altcurrent0_phase_symbol.dev_attr.attr,
> +       &iio_dev_attr_out_altcurrent0_enable.dev_attr.attr,
> +       &iio_dev_attr_out_altcurrent0_pincontrol_en.dev_attr.attr,
>         NULL,

At some point we may also drop the comma in the terminator entry.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/6] staging: iio: ad9832: add sysfs documentation

[Andy Shevchenko]

On Tue, Dec 30, 2025 at 10:35 PM Tomas Borquez <tomasborquez13@gmail.com> wrote:
>
> Add sysfs ABI documentation for the AD9832/AD9835 Direct Digital
> Synthesizer chips, documenting frequency, phase, output control,
> and pin control attributes.

...

> +KernelVersion: 6.19

6.20. The 6.19 boar already sailed.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/6] staging: iio: ad9832: convert to guard(mutex)

[Tomas Borquez]

On Wed, Dec 31, 2025 at 12:50:54AM +0200, Andy Shevchenko wrote:
> On Tue, Dec 30, 2025 at 10:35 PM Tomas Borquez <tomasborquez13@gmail.com> wrote:
> >
> > Use guard(mutex) for cleaner lock handling and simpler error paths.
> 
> ...
> 
> Don't remember if it was in the previous rounds of review or somewhere
> else, but Jonathan (? IIRC) suggestd to use
> 
>   ret = foo(...);
>   if (ret)
>    return ret;
>   break;
>   ...
>   return len;

You are right, I should have sticked to that one since anyways I ended
up using that pattern on patch 5.

> However, this one just duplicates what is already in use. Have you
> checked the bloat-o-meter before and after and see if there is any
> difference in the compiled object file?

Jonathan's is better by 7 delta, so I'll stick with his

> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2 5/6] staging: iio: ad9832: convert to iio channels and ext_info attrs

[Tomas Borquez]

On Wed, Dec 31, 2025 at 12:55:50AM +0200, Andy Shevchenko wrote:
> On Tue, Dec 30, 2025 at 10:35 PM Tomas Borquez <tomasborquez13@gmail.com> wrote:
> >
> > Convert ad9832 from sysfs attributes to standard channel interface
> > using a single IIO_ALTCURRENT channel with ext_info attributes, as this
> > device is a current source DAC with one output.

...

> > -static int ad9832_write_frequency(struct ad9832_state *st,
> > -                                 unsigned int addr, unsigned long fout)
> > +static ssize_t ad9832_write_frequency(struct iio_dev *indio_dev,
> > +                                     uintptr_t private,
> 
> Torvalds said that uintptr_t shouldn't be used in the Linux kernel,
> the unsigned long suffice and enough. Why do we need it here?

Copied it from the definition of iio_chan_spec_ext_info:

struct iio_chan_spec_ext_info {
	const char *name;
	enum iio_shared_by shared;
	ssize_t (*read)(struct iio_dev *, uintptr_t private,
			struct iio_chan_spec const *, char *buf);
	ssize_t (*write)(struct iio_dev *, uintptr_t private,
			 struct iio_chan_spec const *, const char *buf,
			 size_t len);
	uintptr_t private;
};

But can change it

> 
> > +static IIO_DEVICE_ATTR(out_altcurrent0_frequency_symbol, 0644,
> > +                      ad9832_show, ad9832_store, AD9832_FREQ_SYM);
> > +static IIO_DEVICE_ATTR(out_altcurrent0_phase_symbol, 0644,
> > +                      ad9832_show, ad9832_store, AD9832_PHASE_SYM);
> > +static IIO_DEVICE_ATTR(out_altcurrent0_enable, 0644,
> > +                      ad9832_show, ad9832_store, AD9832_OUTPUT_EN);
> 
> Why not IIO_DEVICE_ATTR_RW()?

Not any good reason just didn't know it existed.

> ...
> 
> > +       &iio_dev_attr_out_altcurrent0_frequency_symbol.dev_attr.attr,
> > +       &iio_dev_attr_out_altcurrent0_phase_symbol.dev_attr.attr,
> > +       &iio_dev_attr_out_altcurrent0_enable.dev_attr.attr,
> > +       &iio_dev_attr_out_altcurrent0_pincontrol_en.dev_attr.attr,
> >         NULL,
> 
> At some point we may also drop the comma in the terminator entry.

Will remove it as part of V3

> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2 1/6] staging: iio: ad9832: cleanup dev_err_probe()

[Jonathan Cameron]

On Wed, 31 Dec 2025 00:47:43 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Dec 30, 2025 at 10:35 PM Tomas Borquez <tomasborquez13@gmail.com> wrote:
> >
> > Cleanup dev_err_probe() by keeping messages consistent and adding
> > error message for clock acquisition failure.  
> 
> AFAICS this does two things, the second one is reuse of the temporary
> variable for struct device.
> 
Agreed. Ideal patch break up would be to not just mention
them but do the struct device *dev first, then the dev_err_probe()
change.

Re: [PATCH v2 4/6] staging: iio: ad9832: remove dds.h dependency

[Jonathan Cameron]

On Tue, 30 Dec 2025 17:34:57 -0300
Tomas Borquez <tomasborquez13@gmail.com> wrote:

> Remove dependency on dds.h by converting custom macros to standard IIO
> attribute declarations.
> 
> Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>
Hi Tomas,

Happy new year (almost)

> ---
>  drivers/staging/iio/frequency/ad9832.c | 37 +++++++++++---------------
>  1 file changed, 15 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> index 4bb203a67046..aa78973c3a3c 100644
> --- a/drivers/staging/iio/frequency/ad9832.c
> +++ b/drivers/staging/iio/frequency/ad9832.c
> @@ -24,8 +24,6 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  
> -#include "dds.h"
> -
>  /* Registers */
>  #define AD9832_FREQ0LL		0x0
>  #define AD9832_FREQ0HL		0x1
> @@ -238,27 +236,22 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
>  	}
>  }
>  
> -/*
> - * see dds.h for further information
> - */
> +static IIO_DEVICE_ATTR(out_altvoltage0_frequency0, 0200, NULL, ad9832_write, AD9832_FREQ0HM);
> +static IIO_DEVICE_ATTR(out_altvoltage0_frequency1, 0200, NULL, ad9832_write, AD9832_FREQ1HM);
> +
> +static IIO_DEVICE_ATTR(out_altvoltage0_frequencysymbol, 0200, NULL, ad9832_write, AD9832_FREQ_SYM);
> +static IIO_CONST_ATTR(out_altvoltage0_frequency_scale, "1"); /* 1Hz */

This seems like a pointless attribute.  Default scaling for everything in IIO when
attributes don't tell us otherwise is 1 so should be fine dropping this one.

> +
> +static IIO_DEVICE_ATTR(out_altvoltage0_phase0, 0200, NULL, ad9832_write, AD9832_PHASE0H);
> +static IIO_DEVICE_ATTR(out_altvoltage0_phase1, 0200, NULL, ad9832_write, AD9832_PHASE1H);
> +static IIO_DEVICE_ATTR(out_altvoltage0_phase2, 0200, NULL, ad9832_write, AD9832_PHASE2H);
> +static IIO_DEVICE_ATTR(out_altvoltage0_phase3, 0200, NULL, ad9832_write, AD9832_PHASE3H);
> +
> +static IIO_DEVICE_ATTR(out_altvoltage0_phasesymbol, 0200, NULL, ad9832_write, AD9832_PHASE_SYM);
> +static IIO_CONST_ATTR(out_altvoltage0_phase_scale, "0.0015339808"); /* 2PI/2^12 rad */

I can't immediately think of precedence for scaling of an attribute other than
_raw.  Whilst it's painful, this isn't a high perf path, so we should probably
just do fixed point inputs for phase0,phase1 etc and deal with the scaling
in the driver.  That avoids adding new ABI for this very rare case.

>  
> -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 */
> -
> -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_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_altvoltage0_pincontrol_en, 0200, NULL, ad9832_write, AD9832_PINCTRL_EN);

I'm not that keen on having the documentation only several patches later. Drag that
before this patch or combine adding the new ABI and documentation in the same patch

Jonathan


> +static IIO_DEVICE_ATTR(out_altvoltage0_out_enable, 0200, NULL, ad9832_write, AD9832_OUTPUT_EN);
>  
>  static struct attribute *ad9832_attributes[] = {
>  	&iio_dev_attr_out_altvoltage0_frequency0.dev_attr.attr,

Re: [PATCH v2 4/6] staging: iio: ad9832: remove dds.h dependency

[Jonathan Cameron]

On Wed, 31 Dec 2025 18:09:39 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 30 Dec 2025 17:34:57 -0300
> Tomas Borquez <tomasborquez13@gmail.com> wrote:
> 
> > Remove dependency on dds.h by converting custom macros to standard IIO
> > attribute declarations.
> > 
> > Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>  
> Hi Tomas,
> 
> Happy new year (almost)
> 
> > ---
> >  drivers/staging/iio/frequency/ad9832.c | 37 +++++++++++---------------
> >  1 file changed, 15 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> > index 4bb203a67046..aa78973c3a3c 100644
> > --- a/drivers/staging/iio/frequency/ad9832.c
> > +++ b/drivers/staging/iio/frequency/ad9832.c
> > @@ -24,8 +24,6 @@
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> >  
> > -#include "dds.h"
> > -
> >  /* Registers */
> >  #define AD9832_FREQ0LL		0x0
> >  #define AD9832_FREQ0HL		0x1
> > @@ -238,27 +236,22 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
> >  	}
> >  }
> >  
> > -/*
> > - * see dds.h for further information
> > - */
> > +static IIO_DEVICE_ATTR(out_altvoltage0_frequency0, 0200, NULL, ad9832_write, AD9832_FREQ0HM);
> > +static IIO_DEVICE_ATTR(out_altvoltage0_frequency1, 0200, NULL, ad9832_write, AD9832_FREQ1HM);
> > +
> > +static IIO_DEVICE_ATTR(out_altvoltage0_frequencysymbol, 0200, NULL, ad9832_write, AD9832_FREQ_SYM);
> > +static IIO_CONST_ATTR(out_altvoltage0_frequency_scale, "1"); /* 1Hz */  
> 
> This seems like a pointless attribute.  Default scaling for everything in IIO when
> attributes don't tell us otherwise is 1 so should be fine dropping this one.
> 
> > +
> > +static IIO_DEVICE_ATTR(out_altvoltage0_phase0, 0200, NULL, ad9832_write, AD9832_PHASE0H);
> > +static IIO_DEVICE_ATTR(out_altvoltage0_phase1, 0200, NULL, ad9832_write, AD9832_PHASE1H);
> > +static IIO_DEVICE_ATTR(out_altvoltage0_phase2, 0200, NULL, ad9832_write, AD9832_PHASE2H);
> > +static IIO_DEVICE_ATTR(out_altvoltage0_phase3, 0200, NULL, ad9832_write, AD9832_PHASE3H);
> > +
> > +static IIO_DEVICE_ATTR(out_altvoltage0_phasesymbol, 0200, NULL, ad9832_write, AD9832_PHASE_SYM);
> > +static IIO_CONST_ATTR(out_altvoltage0_phase_scale, "0.0015339808"); /* 2PI/2^12 rad */  
> 
> I can't immediately think of precedence for scaling of an attribute other than
> _raw.  Whilst it's painful, this isn't a high perf path, so we should probably
> just do fixed point inputs for phase0,phase1 etc and deal with the scaling
> in the driver.  That avoids adding new ABI for this very rare case.
> 
> >  
> > -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 */
> > -
> > -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_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_altvoltage0_pincontrol_en, 0200, NULL, ad9832_write, AD9832_PINCTRL_EN);  
> 
> I'm not that keen on having the documentation only several patches later. Drag that
> before this patch or combine adding the new ABI and documentation in the same patch
Ah. I'd missed that this is deliberately a no change patch with old abi.

So ignore the stuff that doesn't make sense with that in mind!

> 
> Jonathan
> 
> 
> > +static IIO_DEVICE_ATTR(out_altvoltage0_out_enable, 0200, NULL, ad9832_write, AD9832_OUTPUT_EN);
> >  
> >  static struct attribute *ad9832_attributes[] = {
> >  	&iio_dev_attr_out_altvoltage0_frequency0.dev_attr.attr,  
> 

Re: [PATCH v2 5/6] staging: iio: ad9832: convert to iio channels and ext_info attrs

[Jonathan Cameron]

On Tue, 30 Dec 2025 17:34:58 -0300
Tomas Borquez <tomasborquez13@gmail.com> wrote:

> Convert ad9832 from sysfs attributes to standard channel interface
> using a single IIO_ALTCURRENT channel with ext_info attributes, as this
> device is a current source DAC with one output.
> 
> Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>

A couple of things inline.


> +
> @@ -230,42 +321,111 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,

> +	case AD9832_PINCTRL_EN:
> +		if (val != 1 && val != 0)
> +			return -EINVAL;
> +
> +		st->ctrl_ss &= ~AD9832_SELSRC;
> +		st->ctrl_ss |= FIELD_PREP(AD9832_SELSRC, !val);
It's not particularly important as this pattern is common, but there is
FIELD_MODIFY() available to make the above a single thing without
needing the mask twice

> +
> +		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_SYNCSELSRC) |
> +						 st->ctrl_ss);
> +		ret = spi_sync(st->spi, &st->msg);
> +		if (ret)
> +			return ret;
> +
> +		st->pinctrl_en = val;
> +		break;
>  	default:
>  		return -ENODEV;
>  	}
> +
> +	return len;
>  }
>
> -static IIO_DEVICE_ATTR(out_altvoltage0_phasesymbol, 0200, NULL, ad9832_write, AD9832_PHASE_SYM);
> -static IIO_CONST_ATTR(out_altvoltage0_phase_scale, "0.0015339808"); /* 2PI/2^12 rad */
> +static const struct iio_chan_spec_ext_info ad9832_ext_info[] = {
> +	AD9832_CHAN_FREQ("frequency0", 0),
> +	AD9832_CHAN_FREQ("frequency1", 1),
> +	AD9832_CHAN_PHASE("phase0", 0),
> +	AD9832_CHAN_PHASE("phase1", 1),
> +	AD9832_CHAN_PHASE("phase2", 2),
> +	AD9832_CHAN_PHASE("phase3", 3),
> +	{ }
> +};
>  
> -static IIO_DEVICE_ATTR(out_altvoltage0_pincontrol_en, 0200, NULL, ad9832_write, AD9832_PINCTRL_EN);
> -static IIO_DEVICE_ATTR(out_altvoltage0_out_enable, 0200, NULL, ad9832_write, AD9832_OUTPUT_EN);
> +static const struct iio_chan_spec ad9832_channels[] = {
> +	{
> +		.type = IIO_ALTCURRENT,
> +		.output = 1,
> +		.indexed = 1,
> +		.channel = 0,
> +		.ext_info = ad9832_ext_info,
> +	},
> +};
> +
> +static IIO_DEVICE_ATTR(out_altcurrent0_frequency_symbol, 0644,
> +		       ad9832_show, ad9832_store, AD9832_FREQ_SYM);
> +static IIO_DEVICE_ATTR(out_altcurrent0_phase_symbol, 0644,
> +		       ad9832_show, ad9832_store, AD9832_PHASE_SYM);
Why not do these two via ext_info? 

> +static IIO_DEVICE_ATTR(out_altcurrent0_enable, 0644,

This one can be done with standard ABI + infomask element at read_raw
so do it that way rather than via IIO_DEVICE_ATTR() which should be
used only when none of the standard stuff is possible because these
direct attr declarations hide things from internal kernel usage and
mean we have to much more carefully check them against ABI documentation.

> +		       ad9832_show, ad9832_store, AD9832_OUTPUT_EN);
> +
> +/*
> + * TODO: Convert to DT property when graduating from staging.
> + * Pin control configuration depends on hardware wiring.
> + */
> +static IIO_DEVICE_ATTR(out_altcurrent0_pincontrol_en, 0644,
> +		       ad9832_show, 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_dev_attr_out_altcurrent0_frequency_symbol.dev_attr.attr,
> +	&iio_dev_attr_out_altcurrent0_phase_symbol.dev_attr.attr,
> +	&iio_dev_attr_out_altcurrent0_enable.dev_attr.attr,
> +	&iio_dev_attr_out_altcurrent0_pincontrol_en.dev_attr.attr,
>  	NULL,
>  };

Re: [PATCH v2 6/6] staging: iio: ad9832: add sysfs documentation

[Jonathan Cameron]

On Tue, 30 Dec 2025 17:34:59 -0300
Tomas Borquez <tomasborquez13@gmail.com> wrote:

> Add sysfs ABI documentation for the AD9832/AD9835 Direct Digital
> Synthesizer chips, documenting frequency, phase, output control,
> and pin control attributes.
> 
> Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>
Hi Tomas,

Good docs, one entry of which reminded me we tend to use powerdown for
output devices rather than _en (for enable)

Jonathan

> ---
>  .../iio/Documentation/sysfs-bus-iio-frequency | 40 +++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 drivers/staging/iio/Documentation/sysfs-bus-iio-frequency
> 
> diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-frequency b/drivers/staging/iio/Documentation/sysfs-bus-iio-frequency
> new file mode 100644
> index 000000000000..10627c19bdb7
> --- /dev/null
> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-frequency
> @@ -0,0 +1,40 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrentY_frequencyZ
> +KernelVersion:	6.19
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +    Frequency in Hz for symbol Z of channel Y. The active
> +    frequency symbol is selected via out_altcurrentY_frequency_symbol.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrentY_phaseZ
> +KernelVersion:	6.19
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +    Phase offset in radians for symbol Z of channel Y. Valid range
> +    is 0 to 2*PI (exclusive). The active phase symbol is selected
> +    via out_altcurrentY_phase_symbol.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrentY_frequency_symbol
> +KernelVersion:	6.19
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +    Selects which frequency symbol is active for channel Y.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrentY_phase_symbol
> +KernelVersion:	6.19
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +    Selects which phase symbol is active for channel Y.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrentY_enable

Ah. I probably led you astray on this in an earlier review. Would be _en
and added to the standard docs in sysfs-bus-iio in the block that starts with in_energyY_en

However, I think that might not be the right attribute to use based on this
being more similar to a DAC channel than those which are about enabling internal
accumulators. For a DAC the common terminology is about powerdown  (so effectively !enable).
That lines up with the use of SLEEP here on the datasheet.

So I think this should be out_altcurrentY_powerdown and documented in the block
with out_altvoltageY_powerdown in sysfs-bus-iio with a tweak to make the descriptive
text cover currents as well as voltages.

Obviously remember to flip the sense of the control though.

> +KernelVersion:	6.19
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +    Enables (1) or disables (0) the output for channel Y.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrentY_pincontrol_en
> +KernelVersion:	6.19
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +    Enables (1) or disables (0) hardware pin control for frequency
> +    and phase symbol selection on channel Y. When enabled, external
> +    pins control symbol selection instead of software.

Re: [PATCH v2 4/6] staging: iio: ad9832: remove dds.h dependency

[Tomas Borquez]

On Wed, Dec 31, 2025 at 12:46:28AM +0200, Andy Shevchenko wrote:
> On Tue, Dec 30, 2025 at 10:35 PM Tomas Borquez <tomasborquez13@gmail.com> wrote:
> >
> > Remove dependency on dds.h by converting custom macros to standard IIO
> > attribute declarations.
> 
> 
> > +static IIO_DEVICE_ATTR(out_altvoltage0_frequency0, 0200, NULL, ad9832_write, AD9832_FREQ0HM);
> > +static IIO_DEVICE_ATTR(out_altvoltage0_frequency1, 0200, NULL, ad9832_write, AD9832_FREQ1HM);
> 
> Any particular point in not using _WO() / _RO() variants of the
> IIO_DEVICE_ATTR_*() macros?
I was looking into this and saw that the definition for both _WO() and _RO() only takes _name and _addr:

#define IIO_DEVICE_ATTR_WO(_name, _addr) \
	struct iio_dev_attr iio_dev_attr_##_name = IIO_ATTR_WO(_name, _addr)

So if we use it for frequency0 for example, it assumes the store function
since we don't pass it:

static IIO_DEVICE_ATTR_WO(out_altvoltage0_frequency0, AD9832_FREQ0HM);

// Expands to
struct iio_dev_attr iio_dev_attr_out_altvoltage0_frequency0 = {
    .dev_attr = {
        . attr = {
          ...
          .store = out_altvoltage0_frequency0_store,
        }
    }
}

Meaning we would have to create a store for each one instead of using
just one write function

> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2 4/6] staging: iio: ad9832: remove dds.h dependency

[Tomas Borquez]

On Wed, Dec 31, 2025 at 06:11:53PM +0000, Jonathan Cameron wrote:
> On Wed, 31 Dec 2025 18:09:39 +0000
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Tue, 30 Dec 2025 17:34:57 -0300
> > Tomas Borquez <tomasborquez13@gmail.com> wrote:
> > 
> > > Remove dependency on dds.h by converting custom macros to standard IIO
> > > attribute declarations.
> > > 
> > > Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>  
> > Hi Tomas,
> > 
> > Happy new year (almost)
Hey Jonathan,

Happy new year!

...

> > I'm not that keen on having the documentation only several patches later. Drag that
> > before this patch or combine adding the new ABI and documentation in the same patch
> Ah. I'd missed that this is deliberately a no change patch with old abi.
> 
> So ignore the stuff that doesn't make sense with that in mind!

Just to make sure I understood:
- I should just remove out_altvoltage0_frequency_scale
- And add documentation in the same patch with all the ABI changes
  "staging: iio: ad9832: convert to iio channels and ext_info attrs"
  or as a separate patch like it is now?

Re: [PATCH v2 4/6] staging: iio: ad9832: remove dds.h dependency

[Andy Shevchenko]

On Sun, Jan 04, 2026 at 02:25:23AM -0300, Tomas Borquez wrote:
> On Wed, Dec 31, 2025 at 12:46:28AM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 30, 2025 at 10:35 PM Tomas Borquez <tomasborquez13@gmail.com> wrote:

...

> > > +static IIO_DEVICE_ATTR(out_altvoltage0_frequency0, 0200, NULL, ad9832_write, AD9832_FREQ0HM);
> > > +static IIO_DEVICE_ATTR(out_altvoltage0_frequency1, 0200, NULL, ad9832_write, AD9832_FREQ1HM);
> > 
> > Any particular point in not using _WO() / _RO() variants of the
> > IIO_DEVICE_ATTR_*() macros?
> I was looking into this and saw that the definition for both _WO() and _RO() only takes _name and _addr:
> 
> #define IIO_DEVICE_ATTR_WO(_name, _addr) \
> 	struct iio_dev_attr iio_dev_attr_##_name = IIO_ATTR_WO(_name, _addr)
> 
> So if we use it for frequency0 for example, it assumes the store function
> since we don't pass it:
> 
> static IIO_DEVICE_ATTR_WO(out_altvoltage0_frequency0, AD9832_FREQ0HM);
> 
> // Expands to
> struct iio_dev_attr iio_dev_attr_out_altvoltage0_frequency0 = {
>     .dev_attr = {
>         . attr = {
>           ...
>           .store = out_altvoltage0_frequency0_store,
>         }
>     }
> }
> 
> Meaning we would have to create a store for each one instead of using
> just one write function

Yes, and it will be fine, no? Explicit is better than implicit
(at least in this case).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/6] staging: iio: ad9832: remove dds.h dependency

[Jonathan Cameron]

On Sun, 4 Jan 2026 02:38:50 -0300
Tomas Borquez <tomasborquez13@gmail.com> wrote:

> On Wed, Dec 31, 2025 at 06:11:53PM +0000, Jonathan Cameron wrote:
> > On Wed, 31 Dec 2025 18:09:39 +0000
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >   
> > > On Tue, 30 Dec 2025 17:34:57 -0300
> > > Tomas Borquez <tomasborquez13@gmail.com> wrote:
> > >   
> > > > Remove dependency on dds.h by converting custom macros to standard IIO
> > > > attribute declarations.
> > > > 
> > > > Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>    
> > > Hi Tomas,
> > > 
> > > Happy new year (almost)  
> Hey Jonathan,
> 
> Happy new year!
> 
> ...
> 
> > > I'm not that keen on having the documentation only several patches later. Drag that
> > > before this patch or combine adding the new ABI and documentation in the same patch  
> > Ah. I'd missed that this is deliberately a no change patch with old abi.
> > 
> > So ignore the stuff that doesn't make sense with that in mind!  
> 
> Just to make sure I understood:
> - I should just remove out_altvoltage0_frequency_scale
> - And add documentation in the same patch with all the ABI changes
>   "staging: iio: ad9832: convert to iio channels and ext_info attrs"
>   or as a separate patch like it is now?

It's fine as you have it already.  Ultimately out_altvotage0_frequency_scale
should go away but that can come in the ABI update patch.

I'd just misunderstood that this was simply a 'get rid of dds.h' usage
patch and the real ABI changes are later.

Jonathan

> 

Re: [PATCH v2 5/6] staging: iio: ad9832: convert to iio channels and ext_info attrs

[Jonathan Cameron]

On Wed, 31 Dec 2025 14:08:52 -0300
Tomas Borquez <tomasborquez13@gmail.com> wrote:

> On Wed, Dec 31, 2025 at 12:55:50AM +0200, Andy Shevchenko wrote:
> > On Tue, Dec 30, 2025 at 10:35 PM Tomas Borquez <tomasborquez13@gmail.com> wrote:  
> > >
> > > Convert ad9832 from sysfs attributes to standard channel interface
> > > using a single IIO_ALTCURRENT channel with ext_info attributes, as this
> > > device is a current source DAC with one output.  
> 
> ...
> 
> > > -static int ad9832_write_frequency(struct ad9832_state *st,
> > > -                                 unsigned int addr, unsigned long fout)
> > > +static ssize_t ad9832_write_frequency(struct iio_dev *indio_dev,
> > > +                                     uintptr_t private,  
> > 
> > Torvalds said that uintptr_t shouldn't be used in the Linux kernel,
> > the unsigned long suffice and enough. Why do we need it here?  
> 
> Copied it from the definition of iio_chan_spec_ext_info:
> 
> struct iio_chan_spec_ext_info {
> 	const char *name;
> 	enum iio_shared_by shared;
> 	ssize_t (*read)(struct iio_dev *, uintptr_t private,
> 			struct iio_chan_spec const *, char *buf);
> 	ssize_t (*write)(struct iio_dev *, uintptr_t private,
> 			 struct iio_chan_spec const *, const char *buf,
> 			 size_t len);
> 	uintptr_t private;
> };
> 
> But can change it
As Linus has been clear on his preferences on this, we should probably
clean that iio_chan_spec_ext_info up as well at some
point.  Not urgent though and don't gate this patch on it.  Better
to clean up all existing users in one go.

This will do as a reference for why:

https://lore.kernel.org/all/CAHk-=wgSvPVGZp56uFCjOZoKcgQp7xpsj3P-Hhg+NXvhPnzszg@mail.gmail.com/

Jonathan

> 
> >   
> > > +static IIO_DEVICE_ATTR(out_altcurrent0_frequency_symbol, 0644,
> > > +                      ad9832_show, ad9832_store, AD9832_FREQ_SYM);
> > > +static IIO_DEVICE_ATTR(out_altcurrent0_phase_symbol, 0644,
> > > +                      ad9832_show, ad9832_store, AD9832_PHASE_SYM);
> > > +static IIO_DEVICE_ATTR(out_altcurrent0_enable, 0644,
> > > +                      ad9832_show, ad9832_store, AD9832_OUTPUT_EN);  
> > 
> > Why not IIO_DEVICE_ATTR_RW()?  
> 
> Not any good reason just didn't know it existed.
> 
> > ...
> >   
> > > +       &iio_dev_attr_out_altcurrent0_frequency_symbol.dev_attr.attr,
> > > +       &iio_dev_attr_out_altcurrent0_phase_symbol.dev_attr.attr,
> > > +       &iio_dev_attr_out_altcurrent0_enable.dev_attr.attr,
> > > +       &iio_dev_attr_out_altcurrent0_pincontrol_en.dev_attr.attr,
> > >         NULL,  
> > 
> > At some point we may also drop the comma in the terminator entry.  
> 
> Will remove it as part of V3
> 
> > -- 
> > With Best Regards,
> > Andy Shevchenko  

Re: [PATCH v2 3/6] staging: iio: ad9832: convert to devm_mutex_init()

[Marcelo Schmitt]

On 12/30, Tomas Borquez wrote:
> Use devm_mutex_init() for automatic resource cleanup when the device
> is removed or probe fails.
> 
> Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>
> ---
>  drivers/staging/iio/frequency/ad9832.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> index 35d8d51d5c2b..4bb203a67046 100644
> --- a/drivers/staging/iio/frequency/ad9832.c
> +++ b/drivers/staging/iio/frequency/ad9832.c
> @@ -310,7 +310,9 @@ static int ad9832_probe(struct spi_device *spi)
>  		return dev_err_probe(dev, PTR_ERR(st->mclk), "failed to enable MCLK\n");
>  
>  	st->spi = spi;
> -	mutex_init(&st->lock);
> +	ret = devm_mutex_init(&spi->dev, &st->lock);
Can use just 'dev' instead of '&spi->dev' after the update from patch 1.

With that
Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>

> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to initialize mutex\n");
>  
>  	indio_dev->name = spi_get_device_id(spi)->name;
>  	indio_dev->info = &ad9832_info;
> -- 
> 2.43.0
> 
>