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

[Jonathan Cameron <jic23@kernel.org>]

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

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

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

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