Re: [PATCH RFC v4 05/10] iio: frequency: ad9910: add digital ramp generator support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing bounds check for negative fractional inputs causes integer underflow, silently configuring hardware to maximum limits.
- [Medium] The `IIO_CHAN_INFO_ENABLE` attribute is exposed for DRG ramp channels but unconditionally returns `-EINVAL`.
- [Low] Incompatible pointer type mismatch when passing `int *` to `div_u64_rem()`.
--

commit 81e59a90f90a26812172e27f2860e2027f281ffb
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>
Subject: iio: frequency: ad9910: add digital ramp generator support

This commit adds Digital Ramp Generator channels with destination selection
based on attribute writes, dwell mode control, configurable upper/lower
limits, step size controlled with rate of change config, and step rate
controlled as sampling frequency.

> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index b069b849e8d72..d6c88ec51145e 100644
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c

[ ... ]

> @@ -716,6 +986,40 @@ static const struct iio_chan_spec ad9910_channels[] = {
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_ENABLE),
>  		.ext_info = ad9910_pp_ext_info,
>  	},
> +	[AD9910_CHAN_IDX_DRG] = {
> +		.type = IIO_ALTVOLTAGE,
> +		.indexed = 1,
> +		.output = 1,
> +		.channel = AD9910_CHANNEL_DRG,
> +		.address = AD9910_CHAN_IDX_DRG,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_ENABLE),
> +	},
> +	[AD9910_CHAN_IDX_DRG_RAMP_UP] = {
> +		.type = IIO_ALTVOLTAGE,
> +		.indexed = 1,
> +		.output = 1,
> +		.channel = AD9910_CHANNEL_DRG_RAMP_UP,
> +		.address = AD9910_CHAN_IDX_DRG_RAMP_UP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_ENABLE) |

The channel definition for AD9910_CHAN_IDX_DRG_RAMP_UP and
AD9910_CHAN_IDX_DRG_RAMP_DOWN includes BIT(IIO_CHAN_INFO_ENABLE) in the
info_mask_separate.

However, in ad9910_read_raw() and ad9910_write_raw(), only
AD9910_CHANNEL_DRG is handled for IIO_CHAN_INFO_ENABLE.

Will reads and writes to the ramp channels' enable attributes fall
through to the default case and unconditionally return -EINVAL? Should
the enable bit be removed from the ramp channels' info mask?

> +				      BIT(IIO_CHAN_INFO_FREQUENCY) |
> +				      BIT(IIO_CHAN_INFO_PHASE) |
> +				      BIT(IIO_CHAN_INFO_SCALE) |
> +				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.ext_info = ad9910_drg_ramp_ext_info,
> +	},

[ ... ]

> @@ -767,6 +1092,26 @@ static int ad9910_read_raw(struct iio_dev *indio_dev,
>  			*val = tmp32 / MICRO;
>  			*val2 = tmp32 % MICRO;
>  			return IIO_VAL_INT_PLUS_MICRO;
> +		case AD9910_CHANNEL_DRG_RAMP_UP:
> +			ret = ad9910_drg_destination_assert(st, AD9910_DEST_PHASE);
> +			if (ret)
> +				return ret;
> +
> +			tmp64 = FIELD_GET(AD9910_DRG_LIMIT_UPPER_MSK,
> +					  st->reg[AD9910_REG_DRG_LIMIT].val64);
> +			tmp64 = (tmp64 * AD9910_PI_NANORAD) >> 31;
> +			*val = div_u64_rem(tmp64, NANO, val2);

Here val2 is provided by the IIO core as an int *, but div_u64_rem()
expects its third argument to be a u32 *remainder.

Does passing a signed integer pointer to a function expecting an unsigned
integer pointer cause a compiler warning or strict aliasing violation?
Should a local u32 variable be used to capture the remainder before
assigning it to *val2?

> +			return IIO_VAL_INT_PLUS_NANO;

[ ... ]

>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		return ad9910_set_sysclk_freq(st, val, true);
> +		if (chan->channel == AD9910_CHANNEL_PHY)
> +			return ad9910_set_sysclk_freq(st, val, true);
> +
> +		tmp64 = ((u64)val * MICRO + val2) * 4;

If a negative fractional string like "-0.5" is provided by userspace, the
IIO core sets val to 0 and val2 to a negative value (like -500000).

Since val2 is signed and (u64)val * MICRO is unsigned, won't adding them
together cause a sign-extended underflow, resulting in a very large tmp64
value that is then clamped to the hardware maximums?

Should there be a bounds check to ensure val2 >= 0, or an in_range()
check for these attributes before calculating tmp64?

> +		if (!tmp64)
> +			return -EINVAL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-ad9910-iio-driver-v4-0-d26bfd20ee3d@analog.com?part=5