Re: [PATCH RFC v2 3/9] iio: frequency: ad9910: add simple parallel port mode support

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Mar 18, 2026 at 05:56:03PM +0000, Rodrigo Alencar via B4 Relay wrote:

> Add parallel port channel with frequency scale, frequency offset, phase
> offset, and amplitude offset extended attributes for configuring the
> parallel data path.

...

> +	ret = iio_str_to_fixpoint(buf, MICRO / 10, &val, &val2);

I think here we just use 100000 as it's in so many drivers de facto use.
ideally this should be fixed on API level.

> +	if (ret)
> +		return ret;

...

> -#define AD9910_EXT_INFO(_name, _ident, _shared) { \

> +#define AD9910_EXT_INFO_TMPL(_name, _ident, _shared, _fn_desc) { \
>  	.name = _name, \
> -	.read = ad9910_ext_info_read, \
> -	.write = ad9910_ext_info_write, \
> +	.read = ad9910_ ## _fn_desc ## _read, \
> +	.write = ad9910_ ## _fn_desc ## _write, \
>  	.private = _ident, \
>  	.shared = _shared, \
>  }

> +#define AD9910_EXT_INFO(_name, _ident, _shared) \
> +	AD9910_EXT_INFO_TMPL(_name, _ident, _shared, ext_info)
> +
> +#define AD9910_PP_EXT_INFO(_name, _ident) \
> +	AD9910_EXT_INFO_TMPL(_name, _ident, IIO_SEPARATE, pp_attrs)

I don't see why you should have so many - lines. This TMPL should be introduced
in the first patch.

...

> +	case IIO_CHAN_INFO_ENABLE:
> +		val = !!val;

Only used once, why do we need this...

> +		switch (chan->channel) {
> +		case AD9910_CHANNEL_PARALLEL_PORT:
> +			tmp32 = FIELD_PREP(AD9910_CFR2_PARALLEL_DATA_PORT_EN_MSK, val);

...and not just here?

> +			return ad9910_reg32_update(st, AD9910_REG_CFR2,
> +						   AD9910_CFR2_PARALLEL_DATA_PORT_EN_MSK,
> +						   tmp32, true);
> +		default:
> +			return -EINVAL;
> +		}

-- 
With Best Regards,
Andy Shevchenko