Re: [RFC PATCH] staging: iio: ad9832: modernize ABI and remove dds.h dependency

[Marcelo Schmitt]

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@xxxxxxxxx>
> ---
> This patch is heavily inspired from discussions in following
> thread.
> https://lore.kernel.org/linux-iio/20251215190806.11003-1-tomasborquez13@xxxxxxxxx/

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 ?

>  };