[PATCH 0/5] staging: ad9832: driver cleanup

[PATCH 0/5] 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 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

-- 
2.43.0

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

[Tomas Borquez]

Remove unnecessary blank lines between comment sections to improve
readability.

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

diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
index e2ad3e5a7a..00813dab7c 100644
--- a/drivers/staging/iio/frequency/ad9832.c
+++ b/drivers/staging/iio/frequency/ad9832.c
@@ -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;
 
-- 
2.43.0

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

[Marcelo Schmitt]

Hi Tomas,

LGTM.
One minor suggestion.

On 12/15, Tomas Borquez wrote:
> Remove unnecessary blank lines between comment sections to improve
> readability.
> 
> Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>
> ---
>  drivers/staging/iio/frequency/ad9832.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> index e2ad3e5a7a..00813dab7c 100644
> --- a/drivers/staging/iio/frequency/ad9832.c
> +++ b/drivers/staging/iio/frequency/ad9832.c
> @@ -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

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

>  
>  /* Command Control Bits */
> -
>  #define AD9832_CMD_PHA8BITSW	0x1

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

[Jonathan Cameron]

On Thu, 18 Dec 2025 11:27:18 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> Hi Tomas,
> 
> LGTM.
> One minor suggestion.
> 
> On 12/15, Tomas Borquez wrote:
> > Remove unnecessary blank lines between comment sections to improve
> > readability.
> > 
> > Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>
> > ---
> >  drivers/staging/iio/frequency/ad9832.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> > index e2ad3e5a7a..00813dab7c 100644
> > --- a/drivers/staging/iio/frequency/ad9832.c
> > +++ b/drivers/staging/iio/frequency/ad9832.c
> > @@ -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  
> 
> 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

> >  
> >  /* Command Control Bits */
> > -
> >  #define AD9832_CMD_PHA8BITSW	0x1  

[PATCH 2/5] 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 | 28 +++++++++++---------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
index 00813dab7c..f9ef3aede4 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>
@@ -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;
 }
 
-- 
2.43.0

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

[Marcelo Schmitt]

On 12/15, Tomas Borquez wrote:
> 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 | 28 +++++++++++---------------
>  1 file changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> index 00813dab7c..f9ef3aede4 100644
> --- a/drivers/staging/iio/frequency/ad9832.c
> +++ b/drivers/staging/iio/frequency/ad9832.c
> @@ -9,6 +9,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;
> -		}
> +		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).

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

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

[Tomas Borquez]

On Thu, Dec 18, 2025 at 11:34:05AM -0300, Marcelo Schmitt wrote:
> On 12/15, Tomas Borquez wrote:
...
> > -	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;
> 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.
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.

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

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

[Jonathan Cameron]

On Thu, 18 Dec 2025 12:16:32 -0300
Tomas Borquez <tomasborquez13@gmail.com> wrote:

> On Thu, Dec 18, 2025 at 11:34:05AM -0300, Marcelo Schmitt wrote:
> > On 12/15, Tomas Borquez wrote:  
> ...
> > > -	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;  
> > 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.  
> 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;

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



> 
> Which is a bit more repetitive than just returning at the end ret ?: len,
> but lemme know what you think.
> 
> >
> > 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

[PATCH 3/5] 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 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
index f9ef3aede4..8d04f1b44f 100644
--- a/drivers/staging/iio/frequency/ad9832.c
+++ b/drivers/staging/iio/frequency/ad9832.c
@@ -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);
-- 
2.43.0

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

[Marcelo Schmitt]

On 12/15, Tomas Borquez wrote:
> 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().

> 
> Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>
> ---
>  drivers/staging/iio/frequency/ad9832.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> index f9ef3aede4..8d04f1b44f 100644
> --- a/drivers/staging/iio/frequency/ad9832.c
> +++ b/drivers/staging/iio/frequency/ad9832.c
> @@ -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.

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

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

[Jonathan Cameron]

On Thu, 18 Dec 2025 11:38:54 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> On 12/15, Tomas Borquez wrote:
> > 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

> > 
> > Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>
> > ---
> >  drivers/staging/iio/frequency/ad9832.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> > index f9ef3aede4..8d04f1b44f 100644
> > --- a/drivers/staging/iio/frequency/ad9832.c
> > +++ b/drivers/staging/iio/frequency/ad9832.c
> > @@ -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.
> 
> >  
> >  	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);
> > -- 
> > 2.43.0
> > 
> >   

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

[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 <tomasborquez13@gmail.com>
---
 drivers/staging/iio/frequency/ad9832.c | 279 +++++++++++++++++++------
 1 file changed, 215 insertions(+), 64 deletions(-)

diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
index 8d04f1b44f..454a317732 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
@@ -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;
-- 
2.43.0

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

[Marcelo Schmitt]

Hi Tomas,

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

On 12/15, Tomas Borquez wrote:
> 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.

> - 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.

> 
> 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 <tomasborquez13@gmail.com>
> ---

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

>  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.

> 
> diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> index 8d04f1b44f..454a317732 100644
> --- a/drivers/staging/iio/frequency/ad9832.c
> +++ b/drivers/staging/iio/frequency/ad9832.c
...
> +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);
Maybe I'm missing something but, why 100000 here? Should it be MICRO instead?

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

...
> +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);
	return sysfs_emit(buf, "%u.%06u\n", phase_urad / MICRO, phase_urad % MICRO);
?

>  }
>  
...
> @@ -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);
this would then be only
		st->ctrl_ss |= FIELD_PREP(AD9832_SELSRC, val);
so we keep consistency with previous FIELD_PREP() in ad9832_store().

> +
> +		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;
>  }
>  
...
> +#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?

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

> +	.shared = IIO_SEPARATE,					\
> +}
> +

Best regards,
Marcelo

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

[Tomas Borquez]

On Thu, Dec 18, 2025 at 12:04:37PM -0300, Marcelo Schmitt wrote:
> Hi Tomas,

...

> > 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.

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.

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

...

> 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.

...

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

> 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.

...

> Best regards,
> Marcelo

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

Tomas

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

[Jonathan Cameron]

On Mon, 15 Dec 2025 16:08:05 -0300
Tomas Borquez <tomasborquez13@gmail.com> wrote:

> 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

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

> 
> 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 <tomasborquez13@gmail.com>

[PATCH 5/5] 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>
---
 .../Documentation/sysfs-bus-iio-dds-ad9832    | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832

diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832 b/drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832
new file mode 100644
index 0000000000..5ceea57917
--- /dev/null
+++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832
@@ -0,0 +1,41 @@
+What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrent0_frequencyY
+KernelVersion:	6.19
+Contact:	linux-iio@vger.kernel.org
+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:	linux-iio@vger.kernel.org
+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:	linux-iio@vger.kernel.org
+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:	linux-iio@vger.kernel.org
+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:	linux-iio@vger.kernel.org
+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:	linux-iio@vger.kernel.org
+Description:
+    Enables (1) or disables (0) hardware pin control for frequency
+    and phase selection. When enabled, external pins control
+    register selection instead of software.
-- 
2.43.0

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

[Marcelo Schmitt]

On 12/15, Tomas Borquez 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>
> ---
>  .../Documentation/sysfs-bus-iio-dds-ad9832    | 41 +++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832
> 
> diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832 b/drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832
> new file mode 100644
> index 0000000000..5ceea57917
> --- /dev/null
> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832
> @@ -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

> +KernelVersion:	6.19
> +Contact:	linux-iio@vger.kernel.org
> +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:	linux-iio@vger.kernel.org
+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.

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

[Jonathan Cameron]

On Thu, 18 Dec 2025 12:10:19 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> On 12/15, Tomas Borquez 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>
> > ---
> >  .../Documentation/sysfs-bus-iio-dds-ad9832    | 41 +++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >  create mode 100644 drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832
> > 
> > diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832 b/drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832
> > new file mode 100644
> > index 0000000000..5ceea57917
> > --- /dev/null
> > +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-dds-ad9832
> > @@ -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
> 
> > +KernelVersion:	6.19
> > +Contact:	linux-iio@vger.kernel.org
> > +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

> 
> What if we do something like
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altcurrentY_frequency_symbolZ
> +KernelVersion:	6.19
> +Contact:	linux-iio@vger.kernel.org
> +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.