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

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

On Fri, Feb 20, 2026 at 04:46:07PM +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.

...

> +	case AD9910_PP_FREQ_SCALE:
> +		if (val32 > BIT(15) || !is_power_of_2(val32))
> +			return -EINVAL;


> +		val32 = FIELD_PREP(AD9910_CFR2_FM_GAIN_MSK, ilog2(val32));

My gut feelings here that this can be simplified to avoid some checks,
or make it more like an expression.

In any case this is an edge between code readability and the size of
the generated binary object.

> +		ret = ad9910_reg32_update(st, AD9910_REG_CFR2,
> +					  AD9910_CFR2_FM_GAIN_MSK,
> +					  val32, true);
> +		break;

...

> +static ssize_t ad9910_pp_attrs_read(struct iio_dev *indio_dev,
> +				    uintptr_t private,

Hmm... Is it a requirement to have uintptr_t? Linus was clear that this is
the type that shouldn't be used in the kernel. unsigned long does the same.

Jonathan, is there any plans to get rid of uintptr_t in IIO?

> +				    const struct iio_chan_spec *chan,
> +				    char *buf)
> +{
> +	struct ad9910_state *st = iio_priv(indio_dev);
> +	int vals[2];
> +	u32 tmp32;
> +	u64 tmp64;

> +	guard(mutex)(&st->lock);

I don't see the need to have vals assignment followed by iio_format_value()
call be under the lock. OTOH, I understand that this makes code simple...

> +	switch (private) {
> +	case AD9910_PP_FREQ_OFFSET:
> +		tmp64 = (u64)st->reg[AD9910_REG_FTW].val32 * st->data.sysclk_freq_hz;
> +		vals[0] = upper_32_bits(tmp64);
> +		vals[1] = upper_32_bits((u64)lower_32_bits(tmp64) * MICRO);
> +		break;
> +	case AD9910_PP_PHASE_OFFSET:
> +		tmp32 = FIELD_GET(AD9910_POW_PP_LSB_MSK,
> +				  st->reg[AD9910_REG_POW].val16);
> +		tmp32 = (tmp32 * AD9910_MAX_PHASE_MICRORAD) >> 16;
> +		vals[0] = tmp32 / MICRO;
> +		vals[1] = tmp32 % MICRO;
> +		break;
> +	case AD9910_PP_AMP_OFFSET:
> +		tmp32 = FIELD_GET(AD9910_ASF_SCALE_FACTOR_PP_LSB_MSK,
> +				  st->reg[AD9910_REG_ASF].val32);
> +		vals[0] = 0;
> +		vals[1] = (u64)tmp32 * MICRO >> 14;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return iio_format_value(buf, IIO_VAL_INT_PLUS_MICRO, ARRAY_SIZE(vals), vals);
> +}

...

> +static ssize_t ad9910_pp_attrs_write(struct iio_dev *indio_dev,
> +				     uintptr_t private,
> +				     const struct iio_chan_spec *chan,
> +				     const char *buf, size_t len)
> +{
> +	struct ad9910_state *st = iio_priv(indio_dev);
> +	int val, val2;
> +	u32 tmp32;
> +	int ret;
> +
> +	ret = iio_str_to_fixpoint(buf, MICRO / 10, &val, &val2);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	switch (private) {
> +	case AD9910_PP_FREQ_OFFSET:
> +		if (val < 0 || val >= st->data.sysclk_freq_hz / 2)

in_range() ?

> +			return -EINVAL;
> +
> +		tmp32 = ad9910_rational_scale((u64)val * MICRO + val2, BIT_ULL(32),
> +					      (u64)MICRO * st->data.sysclk_freq_hz);
> +		ret = ad9910_reg32_write(st, AD9910_REG_FTW, tmp32, true);
> +		break;
> +	case AD9910_PP_PHASE_OFFSET:
> +		if (val != 0 || val2 < 0 || val2 >= (AD9910_MAX_PHASE_MICRORAD >> 8))

		if (val)
			return -EINVAL;

		if (!in_range(val2, ...))

> +			return -EINVAL;

?

> +		tmp32 = DIV_ROUND_CLOSEST((u32)val2 << 16, AD9910_MAX_PHASE_MICRORAD);
> +		tmp32 = min(tmp32, AD9910_POW_PP_LSB_MAX);
> +		tmp32 = FIELD_PREP(AD9910_POW_PP_LSB_MSK, tmp32);
> +		ret = ad9910_reg16_update(st, AD9910_REG_POW,
> +					  AD9910_POW_PP_LSB_MSK,
> +					  tmp32, true);
> +		break;
> +	case AD9910_PP_AMP_OFFSET:
> +		if (val != 0 || val2 < 0 || val2 >= (MICRO >> 8))
> +			return -EINVAL;
> +
> +		tmp32 = DIV_ROUND_CLOSEST((u32)val2 << 14, MICRO);
> +		tmp32 = min(tmp32, AD9910_ASF_PP_LSB_MAX);
> +		tmp32 = FIELD_PREP(AD9910_ASF_SCALE_FACTOR_PP_LSB_MSK, tmp32);
> +		ret = ad9910_reg32_update(st, AD9910_REG_ASF,
> +					  AD9910_ASF_SCALE_FACTOR_PP_LSB_MSK,
> +					  tmp32, true);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return ret ?: len;
> +}

...

> +static const struct iio_chan_spec_ext_info ad9910_pp_ext_info[] = {
> +	AD9910_EXT_INFO("frequency_scale", AD9910_PP_FREQ_SCALE, IIO_SEPARATE),
> +	AD9910_PP_EXT_INFO("frequency_offset", AD9910_PP_FREQ_OFFSET),
> +	AD9910_PP_EXT_INFO("phase_offset", AD9910_PP_PHASE_OFFSET),
> +	AD9910_PP_EXT_INFO("scale_offset", AD9910_PP_AMP_OFFSET),
> +	{ },

Please, use IIO accepted style for terminator entry.
(Yes, for the consistency's sake it might require another cleanup patch.)

> +};

...

> +		val = val ? 1 : 0;

Replaces this...

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

...by  !!val here.

> +			return ad9910_reg32_update(st, AD9910_REG_CFR2,
> +						   AD9910_CFR2_PARALLEL_DATA_PORT_EN_MSK,
> +						   tmp32, true);

-- 
With Best Regards,
Andy Shevchenko