drivers/staging/iio/frequency/ad9832.c | 135 +++++++++++++++---------- 1 file changed, 80 insertions(+), 55 deletions(-)
The AD9832 driver currently relies on legacy custom IIO macros defined
in dds.h. This triggers checkpatch.pl warnings (NON_OCTAL_PERMISSIONS)
and, more importantly, exposes a non-standard sysfs ABI (e.g.,
frequency0, frequency1, phase0-3) directly to user space.
This patch removes the custom macros and migrates the driver to standard
IIO API mechanisms:
- Standard attributes (frequency, phase) now use info_mask_separate.
- Non standard specific toggles (frequencysymbol, phasesymbol,
pincontrol) have been migrated to an ext_info array.
- Remove dds.h header dependency.
- Pointless frequency_scale and phase_scale attributes are dropped as
suggested by Jonathan in
https://lore.kernel.org/linux-iio/20251231180939.422e9e62@jic23-huawei/
NOTE: This patch introduces an intentional ABI changes. The non-standard
attributes (out_altvoltage0_frequency0, etc.) have been removed. They
are replaced by standard attributes (out_altvoltage0_frequency and
out_altvoltage0_phase). Routing to correct register while writing is
handled by checking currently active frequencysymbol or phasesymbol.
Testing: This patch has been strictly compile-tested. I do not have
access to physical AD9832 hardware. I am submitting this as an RFC to
see if these changes are acceptable, and to ask if someone with physical
hardware could test thisg and provide a Tested-by tag.
Signed-off-by: Bhargav Joshi <rougueprince47@gmail.com>
---
This patch is heavily inspired from discussions in following
thread.
https://lore.kernel.org/linux-iio/20251215190806.11003-1-tomasborquez13@gmail.com/
Since this is an RFC please let me know if these changes are acceptable.
drivers/staging/iio/frequency/ad9832.c | 135 +++++++++++++++----------
1 file changed, 80 insertions(+), 55 deletions(-)
diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
index b87ea1781b27..066a1b9ee8d5 100644
--- a/drivers/staging/iio/frequency/ad9832.c
+++ b/drivers/staging/iio/frequency/ad9832.c
@@ -23,8 +23,6 @@
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
-#include "dds.h"
-
/* Registers */
#define AD9832_FREQ0LL 0x0
#define AD9832_FREQ0HL 0x1
@@ -168,12 +166,63 @@ static int ad9832_write_phase(struct ad9832_state *st,
return spi_sync(st->spi, &st->phase_msg);
}
-static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
- const char *buf, size_t len)
+static int ad9832_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct ad9832_state *st = iio_priv(indio_dev);
+ int ret;
+ unsigned int addr;
+
+ if (val < 0)
+ return -EINVAL;
+
+ mutex_lock(&st->lock);
+ switch (mask) {
+ case IIO_CHAN_INFO_FREQUENCY:
+ if (st->ctrl_fp & AD9832_FREQ)
+ addr = AD9832_FREQ1HM;
+ else
+ addr = AD9832_FREQ0HM;
+
+ ret = ad9832_write_frequency(st, addr, val);
+ break;
+
+ case IIO_CHAN_INFO_PHASE:
+ switch (FIELD_GET(AD9832_PHASE_MASK, st->ctrl_fp)) {
+ case 0:
+ addr = AD9832_PHASE0H;
+ break;
+ case 1:
+ addr = AD9832_PHASE1H;
+ break;
+ case 2:
+ addr = AD9832_PHASE2H;
+ break;
+ case 3:
+ addr = AD9832_PHASE3H;
+ break;
+ default:
+ addr = AD9832_PHASE0H;
+ break;
+ }
+ ret = ad9832_write_phase(st, addr, val);
+ break;
+
+ default:
+ ret = -EINVAL;
+ }
+ mutex_unlock(&st->lock);
+
+ return ret;
+}
+
+static ssize_t ad9832_write_ext_info(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ const char *buf, size_t len)
{
- 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);
int ret;
unsigned long val;
@@ -182,17 +231,7 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
goto error_ret;
mutex_lock(&st->lock);
- switch ((u32)this_attr->address) {
- case AD9832_FREQ0HM:
- case AD9832_FREQ1HM:
- ret = ad9832_write_frequency(st, this_attr->address, val);
- break;
- case AD9832_PHASE0H:
- case AD9832_PHASE1H:
- case AD9832_PHASE2H:
- case AD9832_PHASE3H:
- ret = ad9832_write_phase(st, this_attr->address, val);
- break;
+ switch ((u32)private) {
case AD9832_PINCTRL_EN:
st->ctrl_ss &= ~AD9832_SELSRC;
st->ctrl_ss |= FIELD_PREP(AD9832_SELSRC, val ? 0 : 1);
@@ -245,50 +284,34 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
return ret ? ret : len;
}
-/*
- * see dds.h for further information
- */
+#define AD9832_EXT_INFO(_name, _ident) { \
+ .name = _name, \
+ .write = ad9832_write_ext_info, \
+ .private = _ident, \
+ .shared = IIO_SEPARATE, \
+}
-static IIO_DEV_ATTR_FREQ(0, 0, 0200, NULL, ad9832_write, AD9832_FREQ0HM);
-static IIO_DEV_ATTR_FREQ(0, 1, 0200, NULL, ad9832_write, AD9832_FREQ1HM);
-static IIO_DEV_ATTR_FREQSYMBOL(0, 0200, NULL, ad9832_write, AD9832_FREQ_SYM);
-static IIO_CONST_ATTR_FREQ_SCALE(0, "1"); /* 1Hz */
-
-static IIO_DEV_ATTR_PHASE(0, 0, 0200, NULL, ad9832_write, AD9832_PHASE0H);
-static IIO_DEV_ATTR_PHASE(0, 1, 0200, NULL, ad9832_write, AD9832_PHASE1H);
-static IIO_DEV_ATTR_PHASE(0, 2, 0200, NULL, ad9832_write, AD9832_PHASE2H);
-static IIO_DEV_ATTR_PHASE(0, 3, 0200, NULL, ad9832_write, AD9832_PHASE3H);
-static IIO_DEV_ATTR_PHASESYMBOL(0, 0200, NULL,
- ad9832_write, AD9832_PHASE_SYM);
-static IIO_CONST_ATTR_PHASE_SCALE(0, "0.0015339808"); /* 2PI/2^12 rad*/
-
-static IIO_DEV_ATTR_PINCONTROL_EN(0, 0200, NULL,
- ad9832_write, AD9832_PINCTRL_EN);
-static IIO_DEV_ATTR_OUT_ENABLE(0, 0200, NULL,
- ad9832_write, AD9832_OUTPUT_EN);
-
-static 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,
- NULL,
+static const struct iio_chan_spec_ext_info ad9832_ext_info[] = {
+ AD9832_EXT_INFO("pincontrol_en", AD9832_PINCTRL_EN),
+ AD9832_EXT_INFO("frequencysymbol", AD9832_FREQ_SYM),
+ AD9832_EXT_INFO("phasesymbol", AD9832_PHASE_SYM),
+ AD9832_EXT_INFO("out_enable", AD9832_OUTPUT_EN),
+ { }
};
-static const struct attribute_group ad9832_attribute_group = {
- .attrs = ad9832_attributes,
+static const struct iio_chan_spec ad9832_channels[] = {
+ {
+ .type = IIO_ALTVOLTAGE,
+ .indexed = 1,
+ .channel = 0,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY) |
+ BIT(IIO_CHAN_INFO_PHASE),
+ .ext_info = ad9832_ext_info,
+ },
};
static const struct iio_info ad9832_info = {
- .attrs = &ad9832_attribute_group,
+ .write_raw = ad9832_write_raw,
};
static int ad9832_probe(struct spi_device *spi)
@@ -321,6 +344,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.53.0On Fri, 6 Mar 2026 02:33:47 +0530 Bhargav Joshi <rougueprince47@gmail.com> wrote: > The AD9832 driver currently relies on legacy custom IIO macros defined > in dds.h. This triggers checkpatch.pl warnings (NON_OCTAL_PERMISSIONS) > and, more importantly, exposes a non-standard sysfs ABI (e.g., > frequency0, frequency1, phase0-3) directly to user space. > > This patch removes the custom macros and migrates the driver to standard > IIO API mechanisms: > - Standard attributes (frequency, phase) now use info_mask_separate. > - Non standard specific toggles (frequencysymbol, phasesymbol, > pincontrol) have been migrated to an ext_info array. > - Remove dds.h header dependency. > - Pointless frequency_scale and phase_scale attributes are dropped as > suggested by Jonathan in > https://lore.kernel.org/linux-iio/20251231180939.422e9e62@jic23-huawei/ > > NOTE: This patch introduces an intentional ABI changes. The non-standard > attributes (out_altvoltage0_frequency0, etc.) have been removed. They > are replaced by standard attributes (out_altvoltage0_frequency and > out_altvoltage0_phase). Routing to correct register while writing is > handled by checking currently active frequencysymbol or phasesymbol. Ah I think maybe the discussions around this bit got confused as this was not the direction I was expecting it to go in. This doesn't align with the existing cases of how we do symbol selection devices. Often the last thing we want to do is change the active symbol before we've set the value. We could have done a multiplexing write function but if we had it would need to be separate from the controls on which symbol is in use. There are a few examples of existing ABI for this in tree. e.g. dac/ad8460.c which has 16 different rawX outputs and a symbol to pick between those. dac/ltc2644.c which has _raw0 and _raw1 for similar purposes dac/ltc2688.c is similar. We've never added more 'standard' control for channels with symbol controls simply because there aren't that many users yet. > > Testing: This patch has been strictly compile-tested. I do not have > access to physical AD9832 hardware. I am submitting this as an RFC to > see if these changes are acceptable, and to ask if someone with physical > hardware could test thisg and provide a Tested-by tag. > > Signed-off-by: Bhargav Joshi <rougueprince47@gmail.com>
Hello Jonathan, On Mon, Mar 9, 2026 at 12:01 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Fri, 6 Mar 2026 02:33:47 +0530 > Bhargav Joshi <rougueprince47@gmail.com> wrote: > > > The AD9832 driver currently relies on legacy custom IIO macros defined > > in dds.h. This triggers checkpatch.pl warnings (NON_OCTAL_PERMISSIONS) > > and, more importantly, exposes a non-standard sysfs ABI (e.g., > > frequency0, frequency1, phase0-3) directly to user space. > > > > This patch removes the custom macros and migrates the driver to standard > > IIO API mechanisms: > > - Standard attributes (frequency, phase) now use info_mask_separate. > > - Non standard specific toggles (frequencysymbol, phasesymbol, > > pincontrol) have been migrated to an ext_info array. > > - Remove dds.h header dependency. > > - Pointless frequency_scale and phase_scale attributes are dropped as > > suggested by Jonathan in > > https://lore.kernel.org/linux-iio/20251231180939.422e9e62@jic23-huawei/ > > > > NOTE: This patch introduces an intentional ABI changes. The non-standard > > attributes (out_altvoltage0_frequency0, etc.) have been removed. They > > are replaced by standard attributes (out_altvoltage0_frequency and > > out_altvoltage0_phase). Routing to correct register while writing is > > handled by checking currently active frequencysymbol or phasesymbol. > > Ah I think maybe the discussions around this bit got confused as this was > not the direction I was expecting it to go in. > > This doesn't align with the existing cases of how we do symbol selection > devices. > > Often the last thing we want to do is change the active symbol before we've set > the value. We could have done a multiplexing write function but if > we had it would need to be separate from the controls on which symbol is > in use. > I initially thought multiplexing would help perfectly standardize the frequency and phase attributes, but you are right. Multiplexing would break hardware's modulation leading to unwanted glitches. While we can still use a single standard attribute by introducing separate freq_write_target (and phase_write_target) selector, but that will overcomplicate it for the user. So, for v2, I should drop the multiplexing approach and keep frequency0/1 and phase0-3 separated using the ext_info array. > There are a few examples of existing ABI for this in tree. > e.g. > dac/ad8460.c which has 16 different rawX outputs and a symbol to pick between those. > dac/ltc2644.c which has _raw0 and _raw1 for similar purposes > dac/ltc2688.c is similar. > > We've never added more 'standard' control for channels with symbol controls simply > because there aren't that many users yet. > > > > > > Testing: This patch has been strictly compile-tested. I do not have > > access to physical AD9832 hardware. I am submitting this as an RFC to > > see if these changes are acceptable, and to ask if someone with physical > > hardware could test thisg and provide a Tested-by tag. > > Considering David's valid reply: I would not be able to get hands on AD9832 hardware to validate these changes. So I am not quite sure if I should continue with v2 or not? Thank you for the feedback! Best regards, Bhargav > > Signed-off-by: Bhargav Joshi <rougueprince47@gmail.com> >
On Mon, 9 Mar 2026 04:49:01 +0530 Bhargav Joshi <rougueprince47@gmail.com> wrote: > Hello Jonathan, > > On Mon, Mar 9, 2026 at 12:01 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Fri, 6 Mar 2026 02:33:47 +0530 > > Bhargav Joshi <rougueprince47@gmail.com> wrote: > > > > > The AD9832 driver currently relies on legacy custom IIO macros defined > > > in dds.h. This triggers checkpatch.pl warnings (NON_OCTAL_PERMISSIONS) > > > and, more importantly, exposes a non-standard sysfs ABI (e.g., > > > frequency0, frequency1, phase0-3) directly to user space. > > > > > > This patch removes the custom macros and migrates the driver to standard > > > IIO API mechanisms: > > > - Standard attributes (frequency, phase) now use info_mask_separate. > > > - Non standard specific toggles (frequencysymbol, phasesymbol, > > > pincontrol) have been migrated to an ext_info array. > > > - Remove dds.h header dependency. > > > - Pointless frequency_scale and phase_scale attributes are dropped as > > > suggested by Jonathan in > > > https://lore.kernel.org/linux-iio/20251231180939.422e9e62@jic23-huawei/ > > > > > > NOTE: This patch introduces an intentional ABI changes. The non-standard > > > attributes (out_altvoltage0_frequency0, etc.) have been removed. They > > > are replaced by standard attributes (out_altvoltage0_frequency and > > > out_altvoltage0_phase). Routing to correct register while writing is > > > handled by checking currently active frequencysymbol or phasesymbol. > > > > Ah I think maybe the discussions around this bit got confused as this was > > not the direction I was expecting it to go in. > > > > This doesn't align with the existing cases of how we do symbol selection > > devices. > > > > Often the last thing we want to do is change the active symbol before we've set > > the value. We could have done a multiplexing write function but if > > we had it would need to be separate from the controls on which symbol is > > in use. > > > I initially thought multiplexing would help perfectly standardize the > frequency and > phase attributes, but you are right. Multiplexing would break > hardware's modulation > leading to unwanted glitches. > > While we can still use a single standard attribute by introducing > separate freq_write_target > (and phase_write_target) selector, but that will overcomplicate it for the user. > > So, for v2, I should drop the multiplexing approach and keep > frequency0/1 and phase0-3 > separated using the ext_info array. > > > There are a few examples of existing ABI for this in tree. > > e.g. > > dac/ad8460.c which has 16 different rawX outputs and a symbol to pick between those. > > dac/ltc2644.c which has _raw0 and _raw1 for similar purposes > > dac/ltc2688.c is similar. > > > > We've never added more 'standard' control for channels with symbol controls simply > > because there aren't that many users yet. > > > > > > > > > > Testing: This patch has been strictly compile-tested. I do not have > > > access to physical AD9832 hardware. I am submitting this as an RFC to > > > see if these changes are acceptable, and to ask if someone with physical > > > hardware could test thisg and provide a Tested-by tag. > > > > Considering David's valid reply: I would not be able to get hands on > AD9832 hardware > to validate these changes. So I am not quite sure if I should continue > with v2 or not? This sort of ABI update can in my view be safely done against emulation, but for that you'd need to first emulate the device carefully then verify that emulation (to some degree anyway) using the existing driver. So up to you on how large a project you want to take on! Jonathan > > Thank you for the feedback! > > Best regards, > Bhargav > > > > Signed-off-by: Bhargav Joshi <rougueprince47@gmail.com> > >
Hello Bhargav,
On 03/06, Bhargav Joshi wrote:
> The AD9832 driver currently relies on legacy custom IIO macros defined
> in dds.h. This triggers checkpatch.pl warnings (NON_OCTAL_PERMISSIONS)
> and, more importantly, exposes a non-standard sysfs ABI (e.g.,
> frequency0, frequency1, phase0-3) directly to user space.
>
> This patch removes the custom macros and migrates the driver to standard
> IIO API mechanisms:
> - Standard attributes (frequency, phase) now use info_mask_separate.
> - Non standard specific toggles (frequencysymbol, phasesymbol,
> pincontrol) have been migrated to an ext_info array.
> - Remove dds.h header dependency.
> - Pointless frequency_scale and phase_scale attributes are dropped as
> suggested by Jonathan in
> https://lore.kernel.org/linux-iio/20251231180939.422e9e62@jic23-huawei/
>
> NOTE: This patch introduces an intentional ABI changes. The non-standard
> attributes (out_altvoltage0_frequency0, etc.) have been removed. They
> are replaced by standard attributes (out_altvoltage0_frequency and
> out_altvoltage0_phase). Routing to correct register while writing is
> handled by checking currently active frequencysymbol or phasesymbol.
>
> Testing: This patch has been strictly compile-tested. I do not have
> access to physical AD9832 hardware. I am submitting this as an RFC to
> see if these changes are acceptable, and to ask if someone with physical
> hardware could test thisg and provide a Tested-by tag.
>
> Signed-off-by: Bhargav Joshi <rougueprince47@gmail.com>
> ---
> This patch is heavily inspired from discussions in following
> thread.
> https://lore.kernel.org/linux-iio/20251215190806.11003-1-tomasborquez13@gmail.com/
Thanks for making that clear. Still, a change log highlighting the main
differences between yours and Tomas' patches would have been appreciated.
By the way, I'm assuming Tomas is fine with you caring on the driver update?
More comments inline.
>
> Since this is an RFC please let me know if these changes are acceptable.
>
> drivers/staging/iio/frequency/ad9832.c | 135 +++++++++++++++----------
> 1 file changed, 80 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> index b87ea1781b27..066a1b9ee8d5 100644
> --- a/drivers/staging/iio/frequency/ad9832.c
> +++ b/drivers/staging/iio/frequency/ad9832.c
...
> -/*
> - * see dds.h for further information
> - */
> +#define AD9832_EXT_INFO(_name, _ident) { \
> + .name = _name, \
> + .write = ad9832_write_ext_info, \
Why no .read procedure? I think at least for the
out_enable/out_altcurrent0_enable attribute we would want a read function.
> + .private = _ident, \
> + .shared = IIO_SEPARATE, \
> +}
>
> -static IIO_DEV_ATTR_FREQ(0, 0, 0200, NULL, ad9832_write, AD9832_FREQ0HM);
> -static IIO_DEV_ATTR_FREQ(0, 1, 0200, NULL, ad9832_write, AD9832_FREQ1HM);
> -static IIO_DEV_ATTR_FREQSYMBOL(0, 0200, NULL, ad9832_write, AD9832_FREQ_SYM);
> -static IIO_CONST_ATTR_FREQ_SCALE(0, "1"); /* 1Hz */
> -
> -static IIO_DEV_ATTR_PHASE(0, 0, 0200, NULL, ad9832_write, AD9832_PHASE0H);
> -static IIO_DEV_ATTR_PHASE(0, 1, 0200, NULL, ad9832_write, AD9832_PHASE1H);
> -static IIO_DEV_ATTR_PHASE(0, 2, 0200, NULL, ad9832_write, AD9832_PHASE2H);
> -static IIO_DEV_ATTR_PHASE(0, 3, 0200, NULL, ad9832_write, AD9832_PHASE3H);
> -static IIO_DEV_ATTR_PHASESYMBOL(0, 0200, NULL,
> - ad9832_write, AD9832_PHASE_SYM);
> -static IIO_CONST_ATTR_PHASE_SCALE(0, "0.0015339808"); /* 2PI/2^12 rad*/
> -
> -static IIO_DEV_ATTR_PINCONTROL_EN(0, 0200, NULL,
> - ad9832_write, AD9832_PINCTRL_EN);
> -static IIO_DEV_ATTR_OUT_ENABLE(0, 0200, NULL,
> - ad9832_write, AD9832_OUTPUT_EN);
> -
> -static 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,
I'd try to split this patch into smaller ones.
One for updating the frequency and phase channels.
> - &iio_const_attr_out_altvoltage0_phase_scale.dev_attr.attr,
One patch for dropping the _scale attributes.
> - &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,
> - NULL,
> +static const struct iio_chan_spec_ext_info ad9832_ext_info[] = {
> + AD9832_EXT_INFO("pincontrol_en", AD9832_PINCTRL_EN),
Why did you drop the comment about making pincontrol_en a DT property?
Nevertheless, pincontrol_en is another piece I'd put in a separate patch.
> + AD9832_EXT_INFO("frequencysymbol", AD9832_FREQ_SYM),
> + AD9832_EXT_INFO("phasesymbol", AD9832_PHASE_SYM),
Maybe I'm missing something but this doesn't seem to follow the suggested ABI.
https://lore.kernel.org/linux-iio/20251221194358.3284acb4@jic23-huawei/
Why did you chose this particular naming for the ABI?
> + AD9832_EXT_INFO("out_enable", AD9832_OUTPUT_EN),
I think it is okay to change the enable from device attribute to channel
attribute in this case since it's staging and this device has only one channel.
Though, I believe we should call it out_altcurrent0_enable, as proposed on
Tomas' set. I'd also set this on a separate patch.
> + { }
> };
>
> -static const struct attribute_group ad9832_attribute_group = {
> - .attrs = ad9832_attributes,
> +static const struct iio_chan_spec ad9832_channels[] = {
> + {
> + .type = IIO_ALTVOLTAGE,
Maybe I've missed something. Didn't the previous review reached the conclusion
that the channels should be IIO_ALTCURRENT? The datasheet it documents IOUT as
current output.
> + .indexed = 1,
> + .channel = 0,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY) |
> + BIT(IIO_CHAN_INFO_PHASE),
> + .ext_info = ad9832_ext_info,
> + },
> };
>
> static const struct iio_info ad9832_info = {
> - .attrs = &ad9832_attribute_group,
> + .write_raw = ad9832_write_raw,
No .read_raw ?
> };
Hello Marcelo,
Thank you for the review and feedback.
On Sun, Mar 8, 2026 at 2:34 AM Marcelo Schmitt
<marcelo.schmitt1@gmail.com> wrote:
>
> Hello Bhargav,
>
> On 03/06, Bhargav Joshi wrote:
> > The AD9832 driver currently relies on legacy custom IIO macros defined
> > in dds.h. This triggers checkpatch.pl warnings (NON_OCTAL_PERMISSIONS)
> > and, more importantly, exposes a non-standard sysfs ABI (e.g.,
> > frequency0, frequency1, phase0-3) directly to user space.
> >
> > This patch removes the custom macros and migrates the driver to standard
> > IIO API mechanisms:
> > - Standard attributes (frequency, phase) now use info_mask_separate.
> > - Non standard specific toggles (frequencysymbol, phasesymbol,
> > pincontrol) have been migrated to an ext_info array.
> > - Remove dds.h header dependency.
> > - Pointless frequency_scale and phase_scale attributes are dropped as
> > suggested by Jonathan in
> > https://lore.kernel.org/linux-iio/20251231180939.422e9e62@jic23-huawei/
> >
> > NOTE: This patch introduces an intentional ABI changes. The non-standard
> > attributes (out_altvoltage0_frequency0, etc.) have been removed. They
> > are replaced by standard attributes (out_altvoltage0_frequency and
> > out_altvoltage0_phase). Routing to correct register while writing is
> > handled by checking currently active frequencysymbol or phasesymbol.
> >
> > Testing: This patch has been strictly compile-tested. I do not have
> > access to physical AD9832 hardware. I am submitting this as an RFC to
> > see if these changes are acceptable, and to ask if someone with physical
> > hardware could test thisg and provide a Tested-by tag.
> >
> > Signed-off-by: Bhargav Joshi <rougueprince47@gmail.com>
> > ---
> > This patch is heavily inspired from discussions in following
> > thread.
> > https://lore.kernel.org/linux-iio/20251215190806.11003-1-tomasborquez13@gmail.com/
>
> Thanks for making that clear. Still, a change log highlighting the main
> differences between yours and Tomas' patches would have been appreciated.
Understood. I will include a detailed changelog in the cover letter for v2,
>
> By the way, I'm assuming Tomas is fine with you caring on the driver update?
>
Tomas's thread went stale, so I picked it up. Tomas please let me know if
you are still working on this or fine with me picking this up.
> More comments inline.
> >
> > Since this is an RFC please let me know if these changes are acceptable.
> >
> > drivers/staging/iio/frequency/ad9832.c | 135 +++++++++++++++----------
> > 1 file changed, 80 insertions(+), 55 deletions(-)
> >
> > diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> > index b87ea1781b27..066a1b9ee8d5 100644
> > --- a/drivers/staging/iio/frequency/ad9832.c
> > +++ b/drivers/staging/iio/frequency/ad9832.c
> ...
> > -/*
> > - * see dds.h for further information
> > - */
> > +#define AD9832_EXT_INFO(_name, _ident) { \
> > + .name = _name, \
> > + .write = ad9832_write_ext_info, \
> Why no .read procedure? I think at least for the
> out_enable/out_altcurrent0_enable attribute we would want a read function.
>
Yeah, I will add it in a separate patch in v2.
> > + .private = _ident, \
> > + .shared = IIO_SEPARATE, \
> > +}
> >
> > -static IIO_DEV_ATTR_FREQ(0, 0, 0200, NULL, ad9832_write, AD9832_FREQ0HM);
> > -static IIO_DEV_ATTR_FREQ(0, 1, 0200, NULL, ad9832_write, AD9832_FREQ1HM);
> > -static IIO_DEV_ATTR_FREQSYMBOL(0, 0200, NULL, ad9832_write, AD9832_FREQ_SYM);
> > -static IIO_CONST_ATTR_FREQ_SCALE(0, "1"); /* 1Hz */
> > -
> > -static IIO_DEV_ATTR_PHASE(0, 0, 0200, NULL, ad9832_write, AD9832_PHASE0H);
> > -static IIO_DEV_ATTR_PHASE(0, 1, 0200, NULL, ad9832_write, AD9832_PHASE1H);
> > -static IIO_DEV_ATTR_PHASE(0, 2, 0200, NULL, ad9832_write, AD9832_PHASE2H);
> > -static IIO_DEV_ATTR_PHASE(0, 3, 0200, NULL, ad9832_write, AD9832_PHASE3H);
> > -static IIO_DEV_ATTR_PHASESYMBOL(0, 0200, NULL,
> > - ad9832_write, AD9832_PHASE_SYM);
> > -static IIO_CONST_ATTR_PHASE_SCALE(0, "0.0015339808"); /* 2PI/2^12 rad*/
> > -
> > -static IIO_DEV_ATTR_PINCONTROL_EN(0, 0200, NULL,
> > - ad9832_write, AD9832_PINCTRL_EN);
> > -static IIO_DEV_ATTR_OUT_ENABLE(0, 0200, NULL,
> > - ad9832_write, AD9832_OUTPUT_EN);
> > -
> > -static 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,
> I'd try to split this patch into smaller ones.
> One for updating the frequency and phase channels.
>
got it we will split these up in v2.
> > - &iio_const_attr_out_altvoltage0_phase_scale.dev_attr.attr,
> One patch for dropping the _scale attributes.
>
> > - &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,
> > - NULL,
> > +static const struct iio_chan_spec_ext_info ad9832_ext_info[] = {
> > + AD9832_EXT_INFO("pincontrol_en", AD9832_PINCTRL_EN),
> Why did you drop the comment about making pincontrol_en a DT property?
> Nevertheless, pincontrol_en is another piece I'd put in a separate patch.
>
Actually I didn't copy any of the code from Tomas's patches so I missed this.
I will add this as well in v2.
> > + AD9832_EXT_INFO("frequencysymbol", AD9832_FREQ_SYM),
> > + AD9832_EXT_INFO("phasesymbol", AD9832_PHASE_SYM),
> Maybe I'm missing something but this doesn't seem to follow the suggested ABI.
> https://lore.kernel.org/linux-iio/20251221194358.3284acb4@jic23-huawei/
> Why did you chose this particular naming for the ABI?
>
I will review that and update the attribute names in v2 to strictly
follow the suggested ABI
> > + AD9832_EXT_INFO("out_enable", AD9832_OUTPUT_EN),
> I think it is okay to change the enable from device attribute to channel
> attribute in this case since it's staging and this device has only one channel.
> Though, I believe we should call it out_altcurrent0_enable, as proposed on
> Tomas' set. I'd also set this on a separate patch.
>
I will drop this from ext_info entirely and instead use
BIT(IIO_CHAN_INFO_ENABLE).
> > + { }
> > };
> >
> > -static const struct attribute_group ad9832_attribute_group = {
> > - .attrs = ad9832_attributes,
> > +static const struct iio_chan_spec ad9832_channels[] = {
> > + {
> > + .type = IIO_ALTVOLTAGE,
> Maybe I've missed something. Didn't the previous review reached the conclusion
> that the channels should be IIO_ALTCURRENT? The datasheet it documents IOUT as
> current output.
>
Apologies I missed this from that thread. I will correct this to
IIO_ALTCURRENT in v2.
> > + .indexed = 1,
> > + .channel = 0,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY) |
> > + BIT(IIO_CHAN_INFO_PHASE),
> > + .ext_info = ad9832_ext_info,
> > + },
> > };
> >
> > static const struct iio_info ad9832_info = {
> > - .attrs = &ad9832_attribute_group,
> > + .write_raw = ad9832_write_raw,
> No .read_raw ?
I will implement .read_raw in a separate patch in v2.
>
> > };
Thanks again!
Best regards,
Bhargav
+CC: tomasborquez13@gmail.com
for the opportunity to comment about the proposed updates.
On 03/07, Marcelo Schmitt wrote:
> Hello Bhargav,
>
> On 03/06, Bhargav Joshi wrote:
> > The AD9832 driver currently relies on legacy custom IIO macros defined
> > in dds.h. This triggers checkpatch.pl warnings (NON_OCTAL_PERMISSIONS)
> > and, more importantly, exposes a non-standard sysfs ABI (e.g.,
> > frequency0, frequency1, phase0-3) directly to user space.
> >
> > This patch removes the custom macros and migrates the driver to standard
> > IIO API mechanisms:
> > - Standard attributes (frequency, phase) now use info_mask_separate.
> > - Non standard specific toggles (frequencysymbol, phasesymbol,
> > pincontrol) have been migrated to an ext_info array.
> > - Remove dds.h header dependency.
> > - Pointless frequency_scale and phase_scale attributes are dropped as
> > suggested by Jonathan in
> > https://lore.kernel.org/linux-iio/20251231180939.422e9e62@jic23-huawei/
> >
> > NOTE: This patch introduces an intentional ABI changes. The non-standard
> > attributes (out_altvoltage0_frequency0, etc.) have been removed. They
> > are replaced by standard attributes (out_altvoltage0_frequency and
> > out_altvoltage0_phase). Routing to correct register while writing is
> > handled by checking currently active frequencysymbol or phasesymbol.
> >
> > Testing: This patch has been strictly compile-tested. I do not have
> > access to physical AD9832 hardware. I am submitting this as an RFC to
> > see if these changes are acceptable, and to ask if someone with physical
> > hardware could test thisg and provide a Tested-by tag.
> >
> > Signed-off-by: Bhargav Joshi <rougueprince47@gmail.com>
> > ---
> > This patch is heavily inspired from discussions in following
> > thread.
> > https://lore.kernel.org/linux-iio/20251215190806.11003-1-tomasborquez13@gmail.com/
>
> Thanks for making that clear. Still, a change log highlighting the main
> differences between yours and Tomas' patches would have been appreciated.
>
> By the way, I'm assuming Tomas is fine with you caring on the driver update?
>
> More comments inline.
> >
> > Since this is an RFC please let me know if these changes are acceptable.
> >
> > drivers/staging/iio/frequency/ad9832.c | 135 +++++++++++++++----------
> > 1 file changed, 80 insertions(+), 55 deletions(-)
> >
> > diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> > index b87ea1781b27..066a1b9ee8d5 100644
> > --- a/drivers/staging/iio/frequency/ad9832.c
> > +++ b/drivers/staging/iio/frequency/ad9832.c
> ...
> > -/*
> > - * see dds.h for further information
> > - */
> > +#define AD9832_EXT_INFO(_name, _ident) { \
> > + .name = _name, \
> > + .write = ad9832_write_ext_info, \
> Why no .read procedure? I think at least for the
> out_enable/out_altcurrent0_enable attribute we would want a read function.
>
> > + .private = _ident, \
> > + .shared = IIO_SEPARATE, \
> > +}
> >
> > -static IIO_DEV_ATTR_FREQ(0, 0, 0200, NULL, ad9832_write, AD9832_FREQ0HM);
> > -static IIO_DEV_ATTR_FREQ(0, 1, 0200, NULL, ad9832_write, AD9832_FREQ1HM);
> > -static IIO_DEV_ATTR_FREQSYMBOL(0, 0200, NULL, ad9832_write, AD9832_FREQ_SYM);
> > -static IIO_CONST_ATTR_FREQ_SCALE(0, "1"); /* 1Hz */
> > -
> > -static IIO_DEV_ATTR_PHASE(0, 0, 0200, NULL, ad9832_write, AD9832_PHASE0H);
> > -static IIO_DEV_ATTR_PHASE(0, 1, 0200, NULL, ad9832_write, AD9832_PHASE1H);
> > -static IIO_DEV_ATTR_PHASE(0, 2, 0200, NULL, ad9832_write, AD9832_PHASE2H);
> > -static IIO_DEV_ATTR_PHASE(0, 3, 0200, NULL, ad9832_write, AD9832_PHASE3H);
> > -static IIO_DEV_ATTR_PHASESYMBOL(0, 0200, NULL,
> > - ad9832_write, AD9832_PHASE_SYM);
> > -static IIO_CONST_ATTR_PHASE_SCALE(0, "0.0015339808"); /* 2PI/2^12 rad*/
> > -
> > -static IIO_DEV_ATTR_PINCONTROL_EN(0, 0200, NULL,
> > - ad9832_write, AD9832_PINCTRL_EN);
> > -static IIO_DEV_ATTR_OUT_ENABLE(0, 0200, NULL,
> > - ad9832_write, AD9832_OUTPUT_EN);
> > -
> > -static 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,
> I'd try to split this patch into smaller ones.
> One for updating the frequency and phase channels.
>
> > - &iio_const_attr_out_altvoltage0_phase_scale.dev_attr.attr,
> One patch for dropping the _scale attributes.
>
> > - &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,
> > - NULL,
> > +static const struct iio_chan_spec_ext_info ad9832_ext_info[] = {
> > + AD9832_EXT_INFO("pincontrol_en", AD9832_PINCTRL_EN),
> Why did you drop the comment about making pincontrol_en a DT property?
> Nevertheless, pincontrol_en is another piece I'd put in a separate patch.
>
> > + AD9832_EXT_INFO("frequencysymbol", AD9832_FREQ_SYM),
> > + AD9832_EXT_INFO("phasesymbol", AD9832_PHASE_SYM),
> Maybe I'm missing something but this doesn't seem to follow the suggested ABI.
> https://lore.kernel.org/linux-iio/20251221194358.3284acb4@jic23-huawei/
> Why did you chose this particular naming for the ABI?
>
> > + AD9832_EXT_INFO("out_enable", AD9832_OUTPUT_EN),
> I think it is okay to change the enable from device attribute to channel
> attribute in this case since it's staging and this device has only one channel.
> Though, I believe we should call it out_altcurrent0_enable, as proposed on
> Tomas' set. I'd also set this on a separate patch.
>
> > + { }
> > };
> >
> > -static const struct attribute_group ad9832_attribute_group = {
> > - .attrs = ad9832_attributes,
> > +static const struct iio_chan_spec ad9832_channels[] = {
> > + {
> > + .type = IIO_ALTVOLTAGE,
> Maybe I've missed something. Didn't the previous review reached the conclusion
> that the channels should be IIO_ALTCURRENT? The datasheet it documents IOUT as
> current output.
>
> > + .indexed = 1,
> > + .channel = 0,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY) |
> > + BIT(IIO_CHAN_INFO_PHASE),
> > + .ext_info = ad9832_ext_info,
> > + },
> > };
> >
> > static const struct iio_info ad9832_info = {
> > - .attrs = &ad9832_attribute_group,
> > + .write_raw = ad9832_write_raw,
> No .read_raw ?
>
> > };
On 3/5/26 3:03 PM, Bhargav Joshi wrote: > The AD9832 driver currently relies on legacy custom IIO macros defined > in dds.h. This triggers checkpatch.pl warnings (NON_OCTAL_PERMISSIONS) > and, more importantly, exposes a non-standard sysfs ABI (e.g., > frequency0, frequency1, phase0-3) directly to user space. > > This patch removes the custom macros and migrates the driver to standard > IIO API mechanisms: > - Standard attributes (frequency, phase) now use info_mask_separate. > - Non standard specific toggles (frequencysymbol, phasesymbol, > pincontrol) have been migrated to an ext_info array. > - Remove dds.h header dependency. > - Pointless frequency_scale and phase_scale attributes are dropped as > suggested by Jonathan in > https://lore.kernel.org/linux-iio/20251231180939.422e9e62@jic23-huawei/ > > NOTE: This patch introduces an intentional ABI changes. The non-standard > attributes (out_altvoltage0_frequency0, etc.) have been removed. They > are replaced by standard attributes (out_altvoltage0_frequency and > out_altvoltage0_phase). Routing to correct register while writing is > handled by checking currently active frequencysymbol or phasesymbol. > > Testing: This patch has been strictly compile-tested. I do not have > access to physical AD9832 hardware. I am submitting this as an RFC to > see if these changes are acceptable, and to ask if someone with physical > hardware could test thisg and provide a Tested-by tag. > Since this is changing ABI, I think it would best if someone who actually has the hardware and plans to use it should be making the changes to make sure they actually work as intended.
© 2016 - 2026 Red Hat, Inc.