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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 30 Dec 2025 17:34:58 -0300
Tomas Borquez <tomasborquez13@gmail.com> wrote:

> Convert ad9832 from 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.
> 
> Signed-off-by: Tomas Borquez <tomasborquez13@gmail.com>

A couple of things inline.


> +
> @@ -230,42 +321,111 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,

> +	case AD9832_PINCTRL_EN:
> +		if (val != 1 && val != 0)
> +			return -EINVAL;
> +
> +		st->ctrl_ss &= ~AD9832_SELSRC;
> +		st->ctrl_ss |= FIELD_PREP(AD9832_SELSRC, !val);
It's not particularly important as this pattern is common, but there is
FIELD_MODIFY() available to make the above a single thing without
needing the mask twice

> +
> +		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 len;
>  }
>
> -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 */
> +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_DEVICE_ATTR(out_altvoltage0_pincontrol_en, 0200, NULL, ad9832_write, AD9832_PINCTRL_EN);
> -static IIO_DEVICE_ATTR(out_altvoltage0_out_enable, 0200, NULL, ad9832_write, AD9832_OUTPUT_EN);
> +static const struct iio_chan_spec ad9832_channels[] = {
> +	{
> +		.type = IIO_ALTCURRENT,
> +		.output = 1,
> +		.indexed = 1,
> +		.channel = 0,
> +		.ext_info = ad9832_ext_info,
> +	},
> +};
> +
> +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);
Why not do these two via ext_info? 

> +static IIO_DEVICE_ATTR(out_altcurrent0_enable, 0644,

This one can be done with standard ABI + infomask element at read_raw
so do it that way rather than via IIO_DEVICE_ATTR() which should be
used only when none of the standard stuff is possible because these
direct attr declarations hide things from internal kernel usage and
mean we have to much more carefully check them against ABI documentation.

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