Re: [PATCH v3 12/13] iio: adc: ad4134: Support high-speed data capture

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] iio: adc: ad4134: off-by-one error rejects maximum sample rate
- [Medium] iio: adc: ad4134: PWM left running on driver unbind
- [Low] iio: adc: ad4134: exposing unsupported IIO_CHAN_INFO_RAW in offload mode
--

commit e47f312fc16187aac53abe53836e642d8e031a66
Author: Marcelo Schmitt <marcelo.schmitt@analog.com>

iio: adc: ad4134: Support high-speed data capture

This commit introduces SPI transfer offloading to speed up data capture for
the AD4134 ADC. This enhancement enables data acquisition at faster sample
rates up to 1.496 MSPS.

> diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> index 50021001b3741..277f6de7c8714 100644
> --- a/drivers/iio/adc/ad4134.c
> +++ b/drivers/iio/adc/ad4134.c
[ ... ]
> @@ -160,6 +174,39 @@ static const struct iio_chan_spec ad4134_chan_set[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(4),
>  };
>  
> +/*
> + * Hardcoded 32-bit storagebits and CPU endianness because the currently
> + * available HDL only supports that configuration.
> + */
> +#define AD4134_OFFLOAD_CHANNEL(_index) {					\
> +	.type = IIO_VOLTAGE,							\
> +	.indexed = 1,								\
> +	.channel = (_index),							\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),				\

[Severity: Low]
Is it intentional to expose the raw voltage attribute for offload channels?

The IIO_CHAN_INFO_RAW bit is set in the info_mask_separate here, exposing
the sysfs attribute. However, ad4134_read_raw() is modified later in this
patch to unconditionally return -EPROTONOSUPPORT for raw reads when offload
is enabled. Does this clutter the interface with an attribute that is
guaranteed to always fail?

> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |			\
> +				    BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
[ ... ]
> @@ -489,6 +544,92 @@ static struct regmap_config ad4134_regmap_config = {
>  	.max_register = AD4134_CH_VREG(ARRAY_SIZE(ad4134_chan_set) - 1),
>  };
>  
> +static int ad4134_update_conversion_rate(struct ad4134_state *st,
> +					 unsigned int freq_Hz)
> +{
> +	struct spi_offload_trigger_config config = st->offload_trigger_config;
> +	struct pwm_waveform odr_wf = { };
> +	u64 offload_period_ns;
> +	u64 offload_offset_ns;
> +	u64 odr_high_time_ns;
> +	unsigned int count;
> +	u64 target_ns;
> +	int ret;
> +
> +	if (!in_range(freq_Hz, AD4134_MIN_ODR_FREQ_HZ,
> +		      AD4134_MAX_ODR_FREQ_HZ - AD4134_MIN_ODR_FREQ_HZ))
> +		return -ERANGE;

[Severity: Medium]
Does this validation check inadvertently reject the maximum supported
frequency?

The in_range(val, start, len) macro strictly checks if val < start + len.
If a user requests exactly AD4134_MAX_ODR_FREQ_HZ, the condition evaluates
to MAX < MAX (which is false) and incorrectly returns -ERANGE.

> +
> +	odr_wf.period_length_ns = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq_Hz);
[ ... ]
> +	/* Check the PWM periods remain the same */
> +	offload_period_ns = DIV_ROUND_UP_ULL(HZ_PER_GHZ, config.periodic.frequency_hz);
> +	if (odr_wf.period_length_ns != offload_period_ns)
> +		return -EDOM;
> +
> +	ret = pwm_set_waveform_might_sleep(st->odr_pwm, &odr_wf, false);

[Severity: Medium]
Could this leave the PWM hardware running indefinitely after the driver
unbinds?

The ODR PWM is retrieved via devm_pwm_get() and started here, but no explicit
cleanup callback or devm action is registered to stop the waveform. When
the driver unloads, pwm_put() will drop the reference, but the hardware
could continue running, resulting in a resource leak.

> +	if (ret)
> +		return ret;
> +
> +	st->offload_trigger_config = config;
> +	st->odr_wf = odr_wf;
> +	st->odr_hz = DIV_ROUND_UP_ULL(NSEC_PER_SEC, odr_wf.period_length_ns);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790719424.git.marcelo.schmitt@analog.com?part=12