Re: [PATCH v6 10/16] iio: frequency: ad9910: add basic parallel port support

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 18 Jun 2026 14:27:26 +0100
Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:

> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> Add parallel port support with amplitude, phase and frequency channels.
> Those will be buffered capable channels, but only basic control of offset
> and scale are implemented at this point. There are separate amplitude
> and phase control for polar destination, which will provide different scan
> types. Enabling and disabling of parallel mode will be implemented with
> buffer setup ops or with update_scan_mode() once IIO backend integration
> is in place.
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
Hi Rodrigo

Just a couple of comments on perhaps pushing the introduction of switch
statements on chan->address to the earlier patch to improve readability
of this one. + What looks to be a duplicated range check to me.

Jonathan
> ---
>  drivers/iio/frequency/ad9910.c | 187 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 178 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index 95b01295e4a0..262702b62738 100644
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c
...
>  
>  static int ad9910_read_raw(struct iio_dev *indio_dev,
> @@ -640,11 +702,60 @@ static int ad9910_read_raw(struct iio_dev *indio_dev,
>  			return -EINVAL;
>  		}
>  	case IIO_CHAN_INFO_SCALE:
> -		tmp64 = (u64)st->data.output_current_uA *
> -			AD9910_NANO_MILLIAMP_PER_MICROAMP;
> -		*val = 0;
> -		*val2 = tmp64 >> 14;
> -		return IIO_VAL_INT_PLUS_NANO;
> +		switch (chan->address) {
> +		case AD9910_CHAN_IDX_PHY:
> +			tmp64 = (u64)st->data.output_current_uA *
> +				AD9910_NANO_MILLIAMP_PER_MICROAMP;

Similar to below - I think I'd introduce a very minimal switch
in the earlier patch to reduce churn and make this patch
a tiny bit simpler.

> +			*val = 0;
> +			*val2 = tmp64 >> 14;
> +			return IIO_VAL_INT_PLUS_NANO;
> +		case AD9910_CHAN_IDX_PARALLEL_PHASE:
> +			*val = 0;
> +			*val2 = AD9910_PI_NANORAD >> 15;
> +			return IIO_VAL_INT_PLUS_NANO;
> +		case AD9910_CHAN_IDX_PARALLEL_FREQ:
> +			tmp32 = FIELD_GET(AD9910_CFR2_FM_GAIN_MSK,
> +					  st->reg[AD9910_REG_CFR2].val32);
> +			tmp64 = st->data.sysclk_freq_hz << tmp32;
> +			tmp64 = ad9910_rational_scale(tmp64, NANO, BIT_ULL(32));
> +			*val = div_s64_rem(tmp64, NANO, val2);
> +			return IIO_VAL_INT_PLUS_NANO;
> +		case AD9910_CHAN_IDX_PARALLEL_POLAR_AMP:
> +			tmp64 = (u64)st->data.output_current_uA *
> +				AD9910_NANO_MILLIAMP_PER_MICROAMP;
> +			*val = 0;
> +			*val2 = tmp64 >> 8;
> +			return IIO_VAL_INT_PLUS_NANO;
> +		case AD9910_CHAN_IDX_PARALLEL_POLAR_PHASE:
> +			*val = 0;
> +			*val2 = AD9910_PI_NANORAD >> 7;
> +			return IIO_VAL_INT_PLUS_NANO;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_OFFSET:
> +		switch (chan->address) {
> +		case AD9910_CHAN_IDX_PARALLEL_FREQ:
> +			tmp64 = (u64)st->reg[AD9910_REG_FTW].val32 * MICRO;
> +			tmp64 >>= FIELD_GET(AD9910_CFR2_FM_GAIN_MSK,
> +					    st->reg[AD9910_REG_CFR2].val32);
> +			*val = div_s64_rem(tmp64, MICRO, val2);
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		case AD9910_CHAN_IDX_PARALLEL_POLAR_AMP:
> +			tmp32 = FIELD_GET(AD9910_ASF_SCALE_FACTOR_PP_LSB_MSK,
> +					  st->reg[AD9910_REG_ASF].val32);
> +			*val = 0;
> +			*val2 = MICRO * tmp32 >> 6;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		case AD9910_CHAN_IDX_PARALLEL_POLAR_PHASE:
> +			tmp32 = FIELD_GET(AD9910_POW_PP_LSB_MSK,
> +					  st->reg[AD9910_REG_POW].val16);
> +			*val = 0;
> +			*val2 = MICRO * tmp32 >> 8;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		default:
> +			return -EINVAL;
> +		}
>  	default:
>  		return -EINVAL;
>  	}
> @@ -737,12 +848,63 @@ static int ad9910_write_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		return ad9910_set_sysclk_freq(st, val, true);
>  	case IIO_CHAN_INFO_SCALE:
> -		if (val != 0 || val2 < 0)
> +		switch (chan->address) {
> +		case AD9910_CHAN_IDX_PHY:

Perhaps it's worth adding the switch in the earlier patch
(with just this and default) so that we get a nicer
diff here?

> +			if (val != 0 || val2 < 0)
> +				return -EINVAL;
> +
> +			tmp32 = DIV_U64_ROUND_CLOSEST((u64)val2 << 14,
> +						      AD9910_NANO_MILLIAMP_PER_MICROAMP);
> +			return ad9910_set_dac_current(st, tmp32, true);
> +		case AD9910_CHAN_IDX_PARALLEL_FREQ:
> +			if (val < 0 || val2 < 0)
> +				return -EINVAL;
> +
> +			tmp64 = ad9910_rational_scale((u64)val * NANO + val2, BIT_ULL(32),
> +						      (u64)st->data.sysclk_freq_hz * NANO);
> +			tmp64 = roundup_pow_of_two(max(tmp64, 1ULL));
> +			tmp32 = min_t(u32, ilog2(tmp64), FIELD_MAX(AD9910_CFR2_FM_GAIN_MSK));
> +			tmp32 = FIELD_PREP(AD9910_CFR2_FM_GAIN_MSK, tmp32);
> +			return ad9910_reg32_update(st, AD9910_REG_CFR2,
> +						   AD9910_CFR2_FM_GAIN_MSK,
> +						   tmp32, true);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_OFFSET:
> +		if (val < 0 || val2 < 0)

This is the check I mention below as being duplicated (val2 < 0 part)

>  			return -EINVAL;
>  
> -		tmp32 = DIV_U64_ROUND_CLOSEST((u64)val2 << 14,
> -					      AD9910_NANO_MILLIAMP_PER_MICROAMP);
> -		return ad9910_set_dac_current(st, tmp32, true);
> +		switch (chan->address) {
> +		case AD9910_CHAN_IDX_PARALLEL_FREQ:
> +			tmp64 = (u64)val * MICRO + val2;
> +			tmp64 <<= FIELD_GET(AD9910_CFR2_FM_GAIN_MSK,
> +					    st->reg[AD9910_REG_CFR2].val32);
> +			tmp64 = min_t(u64, DIV_U64_ROUND_CLOSEST(tmp64, MICRO),
> +				      U32_MAX);
> +			return ad9910_reg32_write(st, AD9910_REG_FTW, tmp64, true);
> +		case AD9910_CHAN_IDX_PARALLEL_POLAR_AMP:
> +			if (val != 0 || val2 < 0)

Isn't val2 < 0 rejected already above?

> +				return -EINVAL;
> +			tmp32 = DIV_U64_ROUND_CLOSEST((u64)val2 << 6, MICRO);
> +			tmp32 = min(tmp32, AD9910_ASF_PP_LSB_MAX);
> +			tmp32 = FIELD_PREP(AD9910_ASF_SCALE_FACTOR_PP_LSB_MSK, tmp32);
> +			return ad9910_reg32_update(st, AD9910_REG_ASF,
> +						   AD9910_ASF_SCALE_FACTOR_PP_LSB_MSK,
> +						   tmp32, true);
> +		case AD9910_CHAN_IDX_PARALLEL_POLAR_PHASE:
> +			if (val != 0 || val2 < 0)
> +				return -EINVAL;
> +
> +			tmp32 = DIV_U64_ROUND_CLOSEST((u64)val2 << 8, MICRO);
> +			tmp32 = min(tmp32, AD9910_POW_PP_LSB_MAX);
> +			tmp32 = FIELD_PREP(AD9910_POW_PP_LSB_MSK, tmp32);
> +			return ad9910_reg16_update(st, AD9910_REG_POW,
> +						   AD9910_POW_PP_LSB_MSK,
> +						   tmp32, true);
> +		default:
> +			return -EINVAL;
> +		}
>  	default:
>  		return -EINVAL;
>  	}