staging: ad9832: driver cleanup

PATCH Superseded Tomas Borquez 3 participants 18 messages Dec 15, 2025 – Dec 21, 2025

Subsystems (4)

Staging IIO Analog Devices IIO drivers Staging - industrial io

lore

Share & tools

lore

Tomas Borquez

author

2025-12-15 19:08 UTC #0 lore raw

cover staging: ad9832: driver cleanup

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 RFC:
- Split unrelated changes into separate patches: whitespace cleanup, guard() conversion and dev_err_probe() (Jonathan)
- Changed channel type from IIO_ALTVOLTAGE to IIO_ALTCURRENT since this is a current source DAC (Jonathan)
- Kept single channel with ext_info for frequencyN/phaseN attributes rather than multiple indexed channels, as the device has only one output (Jonathan)
- Phase attributes now accept radians directly instead of raw register values, driver performs the conversion internally (Jonathan)
- Added read callbacks for frequency and phase attributes
- Added TODO comment for pincontrol_en noting it should become a DT property during graduation (Jonathan)
- Added ABI documentation for new sysfs attributes

Tomas Borquez (5):
staging: iio: ad9832: clean up whitespace
staging: iio: ad9832: convert to guard(mutex)
staging: iio: ad9832: cleanup dev_err_probe()
staging: iio: ad9832: convert to iio channels and ext_info attrs
staging: iio: ad9832: add sysfs documentation

 .../Documentation/sysfs-bus-iio-dds-ad9832    |  41 +++
 drivers/staging/iio/frequency/ad9832.c        | 315 +++++++++++++-----
 2 files changed, 270 insertions(+), 86 deletions(-)
 create mode 100644 drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832

Signature · 1 line

2.43.0

Tomas Borquez

author

2025-12-15 19:08 UTC #1 lore raw

PATCH 1/5 staging: iio: ad9832: clean up whitespace

Remove unnecessary blank lines between comment sections to improve readability.

Signed-off-by Tomas Borquez <[email protected]>

---
 drivers/staging/iio/frequency/ad9832.c | 4 ----
 1 file changed, 4 deletions(-)

drivers/staging/iio/frequency/ad9832.c +0 -4

index e2ad3e5a7a..00813dab7c 100644
@@ -26,7 +26,6 @@
 #include "dds.h"
 
 /* Registers */
-
 #define AD9832_FREQ0LL		0x0
 #define AD9832_FREQ0HL		0x1
 #define AD9832_FREQ0LM		0x2

@@ -50,7 +49,6 @@
 #define AD9832_OUTPUT_EN	0x13
 
 /* Command Control Bits */
-
 #define AD9832_CMD_PHA8BITSW	0x1
 #define AD9832_CMD_PHA16BITSW	0x0
 #define AD9832_CMD_FRE8BITSW	0x3

@@ -90,7 +88,6 @@
  * @phase_data:		tuning word spi transmit buffer
  * @freq_data:		tuning word spi transmit buffer
  */
-
 struct ad9832_state {
 	struct spi_device		*spi;
 	struct clk			*mclk;

@@ -327,7 +324,6 @@ static int ad9832_probe(struct spi_device *spi)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	/* Setup default messages */
-
 	st->xfer.tx_buf = &st->data;
 	st->xfer.len = 2;

Signature · 1 line

2.43.0

Marcelo Schmitt

2025-12-18 14:25 UTC #2 lore raw

Re: [PATCH 1/5] staging: iio: ad9832: clean up whitespace

Hi Tomas,

LGTM.
One minor suggestion.

Quoted 21 lines · Tomas Borquez

Remove unnecessary blank lines between comment sections to improve readability.
Signed-off-by Tomas Borquez <[email protected]>
---
 drivers/staging/iio/frequency/ad9832.c | 4 ----
 1 file changed, 4 deletions(-)
drivers/staging/iio/frequency/ad9832.c +0 -1
index e2ad3e5a7a..00813dab7c 100644
@@ -26,7 +26,6 @@
 #include "dds.h"
 
 /* Registers */
-
 #define AD9832_FREQ0LL		0x0
 #define AD9832_FREQ0HL		0x1
 #define AD9832_FREQ0LM		0x2

drivers/staging/iio/frequency/ad9832.c line 49

Quoted 1 line

drivers/staging/iio/frequency/ad9832.c +0 -0
@@ -50,7 +49,6 @@
 #define AD9832_OUTPUT_EN	0x13

There is a blank line between AD9832_PHASE3H and AD9832_PHASE_SYM that could also be removed, IMO.
#define AD9832_PHASE3L 0xE
#define AD9832_PHASE3H 0xF
-
#define AD9832_PHASE_SYM 0x10
#define AD9832_FREQ_SYM 0x11

Quoted 4 lines

drivers/staging/iio/frequency/ad9832.c +0 -1
 /* Command Control Bits */
-
 #define AD9832_CMD_PHA8BITSW	0x1

Jonathan Cameron

maintainer

2025-12-21 19:17 UTC #3 lore raw

Re: [PATCH 1/5] staging: iio: ad9832: clean up whitespace

Quoted 21 lines · On Thu, 18 Dec 2025 11:27:18 -0300 Marcelo Schmitt…

Hi Tomas,
LGTM.
One minor suggestion.
Quoted 15 lines · Tomas Borquez
Remove unnecessary blank lines between comment sections to improve readability.
Signed-off-by Tomas Borquez <[email protected]>
---
 drivers/staging/iio/frequency/ad9832.c | 4 ----
 1 file changed, 4 deletions(-)
drivers/staging/iio/frequency/ad9832.c +0 -0
index e2ad3e5a7a..00813dab7c 100644
@@ -26,7 +26,6 @@
 #include "dds.h"
 

Quoted 16 lines

drivers/staging/iio/frequency/ad9832.c line 49
Quoted 7 lines
drivers/staging/iio/frequency/ad9832.c +0 -1
 /* Registers */
-
 #define AD9832_FREQ0LL		0x0
 #define AD9832_FREQ0HL		0x1
 #define AD9832_FREQ0LM		0x2
@@ -50,7 +49,6 @@
 #define AD9832_OUTPUT_EN	0x13  
There is a blank line between AD9832_PHASE3H and AD9832_PHASE_SYM that could also be removed, IMO.
#define AD9832_PHASE3L 0xE
#define AD9832_PHASE3H 0xF
-
#define AD9832_PHASE_SYM 0x10
#define AD9832_FREQ_SYM 0x11

Agreed. In the interests of efficiency all round I applied this tweak whilst picking up the patch.

For this sort of cleanup series I often to it piecemeal. So I've applied this one (no idea if I'll apply the later ones yet!)

Jonathan

Quoted 4 lines

Quoted 4 lines
drivers/staging/iio/frequency/ad9832.c +0 -1
 /* Command Control Bits */
-
 #define AD9832_CMD_PHA8BITSW	0x1  

Tomas Borquez

author

2025-12-15 19:08 UTC #4 lore raw

PATCH 2/5 staging: iio: ad9832: convert to guard(mutex)

In reply to Tomas Borquez

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

Signed-off-by Tomas Borquez <[email protected]>

---
 drivers/staging/iio/frequency/ad9832.c | 28 +++++++++++---------------
 1 file changed, 12 insertions(+), 16 deletions(-)

drivers/staging/iio/frequency/ad9832.c +11 ~~-16~~

index 00813dab7c..f9ef3aede4 100644
@@ -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>

@@ -180,9 +182,9 @@ 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:

@@ -203,22 +205,18 @@ 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 {
-			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;
 	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);

@@ -238,11 +236,9 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 		ret = spi_sync(st->spi, &st->msg);
 		break;
 	default:
-		ret = -ENODEV;
+		return -ENODEV;
 	}
-	mutex_unlock(&st->lock);
 
-error_ret:
 	return ret ? ret : len;
 }

Signature · 1 line

2.43.0

Marcelo Schmitt

2025-12-18 14:32 UTC #5 lore raw

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

Quoted 13 lines · Tomas Borquez

Use guard(mutex) for cleaner lock handling and simpler error paths.
Signed-off-by Tomas Borquez <[email protected]>
---
 drivers/staging/iio/frequency/ad9832.c | 28 +++++++++++---------------
 1 file changed, 12 insertions(+), 16 deletions(-)
drivers/staging/iio/frequency/ad9832.c +0 -0
index 00813dab7c..f9ef3aede4 100644
@@ -9,6 +9,7 @@
 

...

Quoted 15 lines

drivers/staging/iio/frequency/ad9832.c +1 -7

-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 	switch ((u32)this_attr->address) {
 	case AD9832_FREQ0HM:
 	case AD9832_FREQ1HM:

@@ -203,22 +205,18 @@ 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 {
-			ret = -EINVAL;
-			break;

drivers/staging/iio/frequency/ad9832.c static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr, · line 216

Quoted 10 lines

drivers/staging/iio/frequency/ad9832.c +5 -1

-		}
+		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;

Since we now have the mutex unlock handled by guard, why not returning directly from each case?
E.g.

	case AD9832_FREQ1HM:

- ret = ad9832_write_frequency(st, this_attr->address, val);
- break;
+ return ad9832_write_frequency(st, this_attr->address, val);
I think the last return (outside the default clause) won't be needed anymore.

And, since you are touching the lock, you may also update to use devm_mutex_init() (that would probably be best appreciated as a separate patch).

Quoted 7 lines

drivers/staging/iio/frequency/ad9832.c +2 -4

 	case AD9832_PHASE_SYM:
-		if (val > 3) {
-			ret = -EINVAL;
-			break;
-		}
+		if (val > 3)
+			return -EINVAL;

Tomas Borquez

author

2025-12-18 15:16 UTC #6 lore raw

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

Quoted 1 line · Marcelo Schmitt

On 12/15, Tomas Borquez wrote:

...

Quoted 32 lines

Quoted 15 lines

drivers/staging/iio/frequency/ad9832.c +1 -7

-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 	switch ((u32)this_attr->address) {
 	case AD9832_FREQ0HM:
 	case AD9832_FREQ1HM:

@@ -203,22 +205,18 @@ 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 {
-			ret = -EINVAL;
-			break;

drivers/staging/iio/frequency/ad9832.c static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr, · line 216

Quoted 10 lines

drivers/staging/iio/frequency/ad9832.c +5 -1

-		}
+		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;

Since we now have the mutex unlock handled by guard, why not returning directly from each case?
E.g.

	case AD9832_FREQ1HM:

(quoted diff) +1 -2

-		ret = ad9832_write_frequency(st, this_attr->address, val);
-		break;
+		return ad9832_write_frequency(st, this_attr->address, val);

Quoted 1 line

I think the last return (outside the default clause) won't be needed anymore.

Wouldn't work because we need to return len too, so it would be more like:

case AD9832_FREQ1HM:

		 ret = ad9832_write_frequency(st, this_attr->address, val);

+ return ret ?: len;

Which is a bit more repetitive than just returning at the end ret ?: len, but lemme know what you think.

Quoted 3 lines

And, since you are touching the lock, you may also update to use devm_mutex_init() (that would probably be best appreciated as a separate patch).

Yup, sounds good :)

Tomas

Jonathan Cameron

maintainer

2025-12-21 19:23 UTC #7 lore raw

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

Quoted 38 lines · On Thu, 18 Dec 2025 12:16:32 -0300 Tomas Borquez <[email protected]>…

Quoted 1 line · Marcelo Schmitt
On 12/15, Tomas Borquez wrote:
...
Quoted 32 lines
Quoted 15 lines
drivers/staging/iio/frequency/ad9832.c +1 -7
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 	switch ((u32)this_attr->address) {
 	case AD9832_FREQ0HM:
 	case AD9832_FREQ1HM:
@@ -203,22 +205,18 @@ 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 {
-			ret = -EINVAL;
-			break;
drivers/staging/iio/frequency/ad9832.c static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr, · line 216
Quoted 10 lines
drivers/staging/iio/frequency/ad9832.c +5 -1
-		}
+		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;  
Since we now have the mutex unlock handled by guard, why not returning directly from each case?
E.g.
	case AD9832_FREQ1HM:
(quoted diff) +1 -2
-		ret = ad9832_write_frequency(st, this_attr->address, val);
-		break;
+		return ad9832_write_frequency(st, this_attr->address, val);
Quoted 1 line
I think the last return (outside the default clause) won't be needed anymore.
Wouldn't work because we need to return len too, so it would be more like:

Quoted 3 lines

case AD9832_FREQ1HM:
		 ret = ad9832_write_frequency(st, this_attr->address, val);
+ return ret ?: len;

I'd prefer the error dealt with locally as it's more consistent with the other paths in the code where we return directly. The fact this is the last one in each block doesn't to me mean we should bother to handle it in a special way.

You could stick to

		if (ret)
			return ret;

		break;
	...
	}

	return len;

If you prefer to keep that shared return of len in the good path. Or just return it as you suggested in each case: block using the ternary.

Jonathan

Quoted 11 lines

Which is a bit more repetitive than just returning at the end ret ?: len, but lemme know what you think.
Quoted 3 lines
And, since you are touching the lock, you may also update to use devm_mutex_init() (that would probably be best appreciated as a separate patch).
Yup, sounds good :)
Tomas

Tomas Borquez

author

2025-12-15 19:08 UTC #8 lore raw

PATCH 3/5 staging: iio: ad9832: cleanup dev_err_probe()

In reply to Tomas Borquez

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

Signed-off-by Tomas Borquez <[email protected]>

---
 drivers/staging/iio/frequency/ad9832.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/staging/iio/frequency/ad9832.c +3 -3

index f9ef3aede4..8d04f1b44f 100644
@@ -302,15 +302,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);

Signature · 1 line

2.43.0

Marcelo Schmitt

2025-12-18 14:37 UTC #9 lore raw

Re: [PATCH 3/5] staging: iio: ad9832: cleanup dev_err_probe()

Quoted 2 lines · Tomas Borquez

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

This is also a clean-up patch while patch 2 is a driver update so I would provide this patch (currently patch 3) before the patch updating to use guard().

Quoted 11 lines

Signed-off-by Tomas Borquez <[email protected]>
---
 drivers/staging/iio/frequency/ad9832.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/staging/iio/frequency/ad9832.c +0 -0
index f9ef3aede4..8d04f1b44f 100644

drivers/staging/iio/frequency/ad9832.c static int ad9832_probe(struct spi_device *spi) · line 305

Quoted 5 lines

drivers/staging/iio/frequency/ad9832.c +1 -1

@@ -302,15 +302,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");

I'd break the lines and write the message in the line below for this and other dev_err_probe() that exceed 80 columns. E.g.

		return dev_err_probe(&spi->dev, ret,
				     "failed to enable AVDD supply\n");

Note this is just a personal preference of mine, not enforced code style, so fine if you prefer keep it as it is.

Quoted 17 lines

drivers/staging/iio/frequency/ad9832.c +2 -2

 	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);

Signature · 1 line

2.43.0

Jonathan Cameron

maintainer

2025-12-21 19:25 UTC #10 lore raw

Re: [PATCH 3/5] staging: iio: ad9832: cleanup dev_err_probe()

Quoted 7 lines · On Thu, 18 Dec 2025 11:38:54 -0300 Marcelo Schmitt…

Quoted 2 lines · Tomas Borquez
Cleanup dev_err_probe() by keeping messages consistent and adding error message for clock acquisition failure.
This is also a clean-up patch while patch 2 is a driver update so I would provide this patch (currently patch 3) before the patch updating to use guard().

There are a lot of &spi->dev in the probe function. BEeore doing any of this I'd suggest introducing a
struct device *dev = &spi->dev;
and replacing them all with dev.

That also shortens all the lines so maybe some return dev_err_probe() end up below 80 chars and make the suggestion below irrelvant? (I haven't checked!)

Jonathan

Quoted 42 lines

Quoted 11 lines
Signed-off-by Tomas Borquez <[email protected]>
---
 drivers/staging/iio/frequency/ad9832.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/staging/iio/frequency/ad9832.c +0 -0
index f9ef3aede4..8d04f1b44f 100644
drivers/staging/iio/frequency/ad9832.c static int ad9832_probe(struct spi_device *spi) · line 305
Quoted 5 lines
drivers/staging/iio/frequency/ad9832.c +1 -1
@@ -302,15 +302,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");  
I'd break the lines and write the message in the line below for this and other dev_err_probe() that exceed 80 columns. E.g.
		return dev_err_probe(&spi->dev, ret,
				     "failed to enable AVDD supply\n");
Note this is just a personal preference of mine, not enforced code style, so fine if you prefer keep it as it is.
Quoted 17 lines
drivers/staging/iio/frequency/ad9832.c +2 -2
 	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);
Signature · 1 line
2.43.0

Tomas Borquez

author

2025-12-15 19:08 UTC #11 lore raw

PATCH 4/5 staging: iio: ad9832: convert to iio channels and ext_info attrs

In reply to Tomas Borquez

Convert ad9832 from custom 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, as well as removing the dds.h header.

Changes:
- Add single iio_chan_spec with ext_info for frequency0/1 and phase0-3
- Phase attributes accept radians directly, driver converts internally
- Frequency attributes accept Hz (unchanged)
- Cache frequency and phase values in driver state for readback
- Remove dependency on dds.h macros
- Rename symbol attributes to frequency_symbol and phase_symbol

The pincontrol_en attribute is kept temporarily with a TODO noting it should become a DT property during staging graduation.

NOTE: This changes the ABI from out_altvoltage0_* to out_altcurrent0_* with different attribute organization.

Signed-off-by Tomas Borquez <[email protected]>

---
 drivers/staging/iio/frequency/ad9832.c | 279 +++++++++++++++++++------
 1 file changed, 215 insertions(+), 64 deletions(-)

drivers/staging/iio/frequency/ad9832.c +216 ~~-64~~

index 8d04f1b44f..454a317732 100644
@@ -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

@@ -67,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)

@@ -78,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

@@ -95,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];

@@ -113,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);

@@ -121,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);

@@ -145,18 +170,64 @@ 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 int ad9832_write_phase(struct ad9832_state *st,
-			      unsigned long addr, unsigned long phase)
+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 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;
 
+	ret = iio_str_to_fixpoint(buf, 100000, &val, &val2);
+	if (ret)
+		return ret;
+
+	if (val < 0 || val2 < 0)
+		return -EINVAL;
+
+	phase_urad = val * 1000000 + 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);

@@ -164,14 +235,38 @@ static int ad9832_write_phase(struct ad9832_state *st,
 		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_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 / 1000000, phase_urad % 1000000);
 }
 
-static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
+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);

@@ -185,34 +280,20 @@ 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);
-		break;
-	case AD9832_PHASE0H:
-	case AD9832_PHASE1H:
-	case AD9832_PHASE2H:
-	case AD9832_PHASE3H:
-		ret = ad9832_write_phase(st, this_attr->address, val);
-		break;
-	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);
-		break;
+	switch (this_attr->address) {
 	case AD9832_FREQ_SYM:
 		if (val != 1 && val != 0)
 			return -EINVAL;
 
 		st->ctrl_fp &= ~AD9832_FREQ;
-		st->ctrl_fp |= FIELD_PREP(AD9832_FREQ, val ? 1 : 0);
+		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);
+		if (ret)
+			return ret;
+
+		st->freq_sym = val;
 		break;
 	case AD9832_PHASE_SYM:
 		if (val > 3)

@@ -224,8 +305,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);
+		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

@@ -234,49 +322,110 @@ 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);
+		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 ? 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);
+		if (ret)
+			return ret;
+
+		st->pinctrl_en = val;
 		break;
 	default:
 		return -ENODEV;
 	}
 
-	return ret ? ret : len;
+	return len;
 }
 
-/*
- * see dds.h for further information
- */
+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_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 */
+	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;
+	}
+}
+
+#define AD9832_CHAN_FREQ(_name, _channel) {			\
+	.name = _name,						\
+	.write = ad9832_write_frequency,			\
+	.read = ad9832_read_frequency,				\
+	.private = _channel,					\
+	.shared = IIO_SEPARATE,					\
+}
+
+#define AD9832_CHAN_PHASE(_name, _channel) {			\
+	.name = _name,						\
+	.write = ad9832_write_phase,				\
+	.read = ad9832_read_phase,				\
+	.private = _channel,					\
+	.shared = IIO_SEPARATE,					\
+}
+
+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_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 const struct iio_chan_spec ad9832_channels[] = {
+	{
+		.type = IIO_ALTCURRENT,
+		.output = 1,
+		.indexed = 1,
+		.channel = 0,
+		.ext_info = ad9832_ext_info,
+	},
+};
 
-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_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,
 };

@@ -318,6 +467,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;

Signature · 1 line

2.43.0

Marcelo Schmitt

2025-12-18 15:02 UTC #12 lore raw

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

Hi Tomas,

The updates look mostly good to me. A few comments inline.

Quoted 11 lines · Tomas Borquez

Convert ad9832 from custom 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, as well as removing the dds.h header.
Changes:
- Add single iio_chan_spec with ext_info for frequency0/1 and phase0-3
- Phase attributes accept radians directly, driver converts internally
- Frequency attributes accept Hz (unchanged)
- Cache frequency and phase values in driver state for readback
- Remove dependency on dds.h macros

I'm not sure, was the dds stuff being used before this patch? Maybe dds.h removal would be better as another separate clean up patch.

Quoted 1 line

- Rename symbol attributes to frequency_symbol and phase_symbol

It's nice to have a change log between patch versions. Though, it's usually provided as part of extra patch info, not commit message.

Quoted 9 lines

The pincontrol_en attribute is kept temporarily with a TODO noting it should become a DT property during staging graduation.
NOTE: This changes the ABI from out_altvoltage0_* to out_altcurrent0_* with different attribute organization.
Signed-off-by Tomas Borquez <[email protected]>
---

The change log is usually provided here, below the '---'.

Quoted 2 lines

 drivers/staging/iio/frequency/ad9832.c | 279 +++++++++++++++++++------
 1 file changed, 215 insertions(+), 64 deletions(-)

This patch fails to apply? I've tried getting it applied on top of current IIO testing branch with b4 shazam, git am <individual patches>, and git apply <patch4 on top of applied patch3>, but patch 4 fails to apply either way. Couldn't figure out how to fix that.

Quoted 5 lines

drivers/staging/iio/frequency/ad9832.c +0 -0
index 8d04f1b44f..454a317732 100644

...

Quoted 10 lines

drivers/staging/iio/frequency/ad9832.c +9 -0

+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);

drivers/staging/iio/frequency/ad9832.c static int ad9832_write_frequency(struct ad9832_state *st, · line 217

Quoted 10 lines

drivers/staging/iio/frequency/ad9832.c +4 -1

 	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;
 
+	ret = iio_str_to_fixpoint(buf, 100000, &val, &val2);

Maybe I'm missing something but, why 100000 here? Should it be MICRO instead?

drivers/staging/iio/frequency/ad9832.c static int ad9832_write_frequency(struct ad9832_state *st, · line 224

Quoted 7 lines

drivers/staging/iio/frequency/ad9832.c +7 -0

+	if (ret)
+		return ret;
+
+	if (val < 0 || val2 < 0)
+		return -EINVAL;
+
+	phase_urad = val * 1000000 + val2;

We can use macros to make it easier to read values that are related to metric units.

	phase_urad = val * MICRO + val2;

then
#include <linux/units.h>

...

Quoted 5 lines

drivers/staging/iio/frequency/ad9832.c +5 -0

+static ssize_t ad9832_read_phase(struct iio_dev *indio_dev,
+				 uintptr_t private,
+				 struct iio_chan_spec const *chan,
+				 char *buf)
+{

drivers/staging/iio/frequency/ad9832.c static int ad9832_write_phase(struct ad9832_state *st, · line 265

Quoted 10 lines

drivers/staging/iio/frequency/ad9832.c +10 -0

+	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 / 1000000, phase_urad % 1000000);

	return sysfs_emit(buf, "%u.%06u\n", phase_urad / MICRO, phase_urad % MICRO);

Quoted 2 lines

}

...

Quoted 5 lines

drivers/staging/iio/frequency/ad9832.c +1 -0

@@ -234,49 +322,110 @@ 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);
+		if (ret)

drivers/staging/iio/frequency/ad9832.c static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr, · line 335

Quoted 10 lines

drivers/staging/iio/frequency/ad9832.c +10 -0

+			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 ? 0 : 1);

this would then be only

		st->ctrl_ss |= FIELD_PREP(AD9832_SELSRC, val);

so we keep consistency with previous FIELD_PREP() in ad9832_store().

Quoted 17 lines

drivers/staging/iio/frequency/ad9832.c +9 -1

+
+		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 ret ? ret : len;
+	return len;
 }

...

drivers/staging/iio/frequency/ad9832.c static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr, · line 379

Quoted 5 lines

drivers/staging/iio/frequency/ad9832.c +5 -0

+#define AD9832_CHAN_FREQ(_name, _channel) {			\
+	.name = _name,						\
+	.write = ad9832_write_frequency,			\
+	.read = ad9832_read_frequency,				\
+	.private = _channel,					\

Since the DAC has only one output channel, doesn't it look somewhat misleading to call private data _channel?
Based on data sheet naming, I'd call it _select. Maybe _fselect _psel?

drivers/staging/iio/frequency/ad9832.c static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr, · line 387

Quoted 8 lines

drivers/staging/iio/frequency/ad9832.c +8 -0

+	.shared = IIO_SEPARATE,					\
+}
+
+#define AD9832_CHAN_PHASE(_name, _channel) {			\
+	.name = _name,						\
+	.write = ad9832_write_phase,				\
+	.read = ad9832_read_phase,				\
+	.private = _channel,					\

same here

drivers/staging/iio/frequency/ad9832.c static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr, · line 382

Quoted 3 lines

drivers/staging/iio/frequency/ad9832.c +3 -0
+	.shared = IIO_SEPARATE,					\
+}
+

Best regards,
Marcelo

Tomas Borquez

author

2025-12-18 15:46 UTC #13 lore raw

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

Quoted 1 line · Marcelo Schmitt

Hi Tomas,

...

Quoted 5 lines

Quoted 5 lines
Convert ad9832 from custom 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, as well as removing the dds.h header.

Quoted 8 lines

Quoted 6 lines
Changes:
- Add single iio_chan_spec with ext_info for frequency0/1 and phase0-3
- Phase attributes accept radians directly, driver converts internally
- Frequency attributes accept Hz (unchanged)
- Cache frequency and phase values in driver state for readback
- Remove dependency on dds.h macros
I'm not sure, was the dds stuff being used before this patch? Maybe dds.h removal would be better as another separate clean up patch.

Yep it was used, dds.h contains some macros for creating the sysfs attributes, I could make a separate patch which transforms the custom macros to "native" ones.

Quoted 3 lines

Quoted 1 line
- Rename symbol attributes to frequency_symbol and phase_symbol
It's nice to have a change log between patch versions. Though, it's usually provided as part of extra patch info, not commit message.

You are right mb, I realized too late I'll add it next time under ---

...

Quoted 4 lines

This patch fails to apply? I've tried getting it applied on top of current IIO testing branch with b4 shazam, git am <individual patches>, and git apply <patch4 on top of applied patch3>, but patch 4 fails to apply either way. Couldn't figure out how to fix that.

I think I made some weird change which makes it unappliable but I'll make sure that does not happen next version.

...

Quoted 11 lines

drivers/staging/iio/frequency/ad9832.c +7 -0

+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;

Quoted 6 lines

drivers/staging/iio/frequency/ad9832.c static int ad9832_write_frequency(struct ad9832_state *st, · line 217
Quoted 5 lines
drivers/staging/iio/frequency/ad9832.c +2 -1
-	if (phase >= BIT(AD9832_PHASE_BITS))
+	if (private >= ARRAY_SIZE(ad9832_phase_addr))
 		return -EINVAL;
 
+	ret = iio_str_to_fixpoint(buf, 100000, &val, &val2);
Maybe I'm missing something but, why 100000 here? Should it be MICRO instead?

iio_str_to_fixpoint parses one more decimal digit than the fract_mult suggests, passing 100000 gives 6 decimal digits (microradians precision). Using MICRO here would parse 7 digits instead, as far as I understood.

Quoted 1 line

We can use macros to make it easier to read values that are related to metric units.

Agreed, I'll use MICRO for the multiplication and division/modulo operations.

...

Quoted 2 lines

Best regards,
Marcelo

Agree on the rest of the feedback, thanks for the comments from these and previous patches

Tomas

Jonathan Cameron

maintainer

2025-12-21 19:36 UTC #14 lore raw

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

In reply to Tomas Borquez

Quoted 5 lines · On Mon, 15 Dec 2025 16:08:05 -0300 Tomas Borquez <[email protected]>…

Convert ad9832 from custom 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, as well as removing the dds.h header.

Quoted 7 lines

Changes:
- Add single iio_chan_spec with ext_info for frequency0/1 and phase0-3
- Phase attributes accept radians directly, driver converts internally
- Frequency attributes accept Hz (unchanged)
- Cache frequency and phase values in driver state for readback
- Remove dependency on dds.h macros
- Rename symbol attributes to frequency_symbol and phase_symbol

If you can break this up into a few smaller patches that would make it easier to review. These symbol elements would be one thing I think could be done in a separate patch.

Otherwise I didn't see anything extra to comment on in the actual code.

Jonathan

Quoted 8 lines

The pincontrol_en attribute is kept temporarily with a TODO noting it should become a DT property during staging graduation.
NOTE: This changes the ABI from out_altvoltage0_* to out_altcurrent0_* with different attribute organization.
Signed-off-by Tomas Borquez <[email protected]>

Tomas Borquez

author

2025-12-15 19:08 UTC #15 lore raw

PATCH 5/5 staging: iio: ad9832: add sysfs documentation

In reply to 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 <[email protected]>

---
 .../Documentation/sysfs-bus-iio-dds-ad9832    | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832

drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832 new file +41 -0

new file mode 100644
index 0000000000..5ceea57917
@@ -0,0 +1,41 @@
+What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrent0_frequencyY
+KernelVersion:	6.19
+Contact:	[email protected]
+Description:
+    Frequency in Hz for frequency register Y (0-1). The active
+    frequency register is selected via out_altcurrent0_frequency_symbol.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrent0_phaseY
+KernelVersion:	6.19
+Contact:	[email protected]
+Description:
+    Phase offset in radians for phase register Y (0-3). Valid
+    range is 0 to 2*PI (exclusive) with 12-bit hardware resolution. The
+    active phase register is selected via out_altcurrent0_phase_symbol.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrent0_frequency_symbol
+KernelVersion:	6.19
+Contact:	[email protected]
+Description:
+    Selects which frequency register (0 or 1) is active for output.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrent0_phase_symbol
+KernelVersion:	6.19
+Contact:	[email protected]
+Description:
+    Selects which phase register (0-3) is active for output.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrent0_enable
+KernelVersion:	6.19
+Contact:	[email protected]
+Description:
+    Enables (1) or disables (0) the output. When disabled, the
+    device is held in reset state.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrent0_pincontrol_en
+KernelVersion:	6.19
+Contact:	[email protected]
+Description:
+    Enables (1) or disables (0) hardware pin control for frequency
+    and phase selection. When enabled, external pins control
+    register selection instead of software.

Signature · 1 line

2.43.0

Marcelo Schmitt

2025-12-18 15:08 UTC #16 lore raw

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

Quoted 16 lines · 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 <[email protected]>
---
 .../Documentation/sysfs-bus-iio-dds-ad9832    | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832
drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832 new file +0 -0
new file mode 100644
index 0000000000..5ceea57917

drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832 line 1

Quoted 1 line

drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832 new file +1 -0
@@ -0,0 +1,41 @@
+What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrent0_frequencyY

To be more generic, I think we could use a capital letter for the channel number too and avoid the symbol postfix in the this attribute's name.
+What: /sys/bus/iio/devices/iio:deviceX/out_altcurrentY_frequency

drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832 line 6

Quoted 5 lines

drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832 new file +5 -0

+KernelVersion:	6.19
+Contact:	[email protected]
+Description:
+    Frequency in Hz for frequency register Y (0-1). The active
+    frequency register is selected via out_altcurrent0_frequency_symbol.

Maybe, if we can keep only the 'Frequency in Hz for frequency channel Y.' part of the description, this is something that could actually go into sysfs-bus-iio when the driver graduates. Though, the 'register is selected via out_altcurrent0_frequency_symbol' part is trickier since it seems to be design specific.

What if we do something like
+What: /sys/bus/iio/devices/iio:deviceX/out_altcurrentY_frequency_symbolZ
+KernelVersion: 6.19
+Contact: [email protected]
+Description:
+ Frequency in Hz for symbol Z of frequency Y channel. The active
+ frequency symbol is selected via out_altcurrentY_frequency_symbol.
?

Same thoughts about out_altcurrent0_phaseY.

Jonathan Cameron

maintainer

2025-12-21 19:44 UTC #17 lore raw

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

Quoted 22 lines · On Thu, 18 Dec 2025 12:10:19 -0300 Marcelo Schmitt…

Quoted 16 lines · 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 <[email protected]>
---
 .../Documentation/sysfs-bus-iio-dds-ad9832    | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832
drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832 new file +0 -0
new file mode 100644
index 0000000000..5ceea57917
drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832 line 1
Quoted 1 line
drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832 new file +1 -0
@@ -0,0 +1,41 @@
+What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrent0_frequencyY  
To be more generic, I think we could use a capital letter for the channel number too and avoid the symbol postfix in the this attribute's name.
+What: /sys/bus/iio/devices/iio:deviceX/out_altcurrentY_frequency

Quoted 10 lines

drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832 line 6
Quoted 5 lines
drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832 new file +5 -0
+KernelVersion:	6.19
+Contact:	[email protected]
+Description:
+    Frequency in Hz for frequency register Y (0-1). The active
+    frequency register is selected via out_altcurrent0_frequency_symbol.  
Maybe, if we can keep only the 'Frequency in Hz for frequency channel Y.' part of the description, this is something that could actually go into sysfs-bus-iio when the driver graduates. Though, the 'register is selected via out_altcurrent0_frequency_symbol' part is trickier since it seems to be design specific.

For now, these interfaces are specific enough I'd be tempted to put them in sysfs-bus-iio-frequency or a driver specific file for now.

That lets us keep a little more track of how they are used. It's a lot easier to make sure documentation is generic enough once there are a few users (though as Marcelo suggests, make it as a general as possible from the start!)

Jonathan

Quoted 11 lines

What if we do something like
drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832 new file +6 -0
+What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrentY_frequency_symbolZ
+KernelVersion:	6.19
+Contact:	[email protected]
+Description:
+    Frequency in Hz for symbol Z of frequency Y channel. The active
+    frequency symbol is selected via out_altcurrentY_frequency_symbol.
?
Same thoughts about out_altcurrent0_phaseY.