Re: [PATCH v5 14/15] iio: adc: ad4134: Support high-speed data capture

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] iio: adc: ad4134: positive return from pwm_round_waveform_might_sleep()
- [High] iio: adc: ad4134: ODR PWM enabled in probe and never disabled
- [Medium] iio: adc: ad4134: period rounding drift in ad4134_update_conversion_rate()
- [Medium] iio: adc: ad4134: trigger offset uses unrounded ODR high time
--

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

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

This commit adds SPI transfer offload support to the ad4134 ADC driver,
using a periodic PWM trigger and DMA engine buffer to capture samples at
rates up to 1.496 MSPS.

> diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> index 6b05e40f7aeb7..21e8039b34116 100644
> --- a/drivers/iio/adc/ad4134.c
> +++ b/drivers/iio/adc/ad4134.c
[ ... ]
> @@ -461,6 +515,92 @@ static const struct regmap_config ad4134_regmap_config = {
>  	.max_register = AD4134_CH_VREG(AD4134_NUM_CHANNELS - 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 + 1))
> +		return -ERANGE;
> +
> +	odr_wf.period_length_ns = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq_Hz);
> +	/*
> +	 * Set the PWM duty cycle to keep ODR high for at least minimum required
> +	 * time. If the rounded PWM's value is less than the minimum required,
> +	 * increase the target value by 10 and attempt to round the waveform
> +	 * again, until the minimum (or try count limit) is reached.
> +	 */
> +	odr_high_time_ns = div64_ul(6ULL * NSEC_PER_SEC, st->sys_clk_hz);
> +	target_ns = 0;
> +	count = 100;
> +	do {
> +		target_ns += 10; /* Increment by PWM duty cycle period */
> +		odr_wf.duty_length_ns = target_ns;
> +		ret = pwm_round_waveform_might_sleep(st->odr_pwm, &odr_wf);
> +		if (ret)
> +			return ret;
> +	} while (--count && odr_wf.duty_length_ns < odr_high_time_ns);

[Severity: High]
In ad4134_update_conversion_rate(), pwm_round_waveform_might_sleep() returns
0 on exact or downward rounding, 1 when a waveform value had to be rounded up,
or a negative errno on failure:

drivers/pwm/core.c:
 * Returns: 0 on success, 1 if at least one value had to be rounded up or a
 * negative errno.
 * Context: May sleep.
 */
int pwm_round_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf)

Because target_ns starts at 0 (requesting 10 ns on the first iteration) and
the return value is checked with if (ret) instead of if (ret < 0), won't any
upward rounding cause ad4134_update_conversion_rate() to abort and return +1
to ad4134_offload_setup() -> ad4134_probe() or ad4134_write_raw()?

Also, since target_ns starts at 0 and only increments up to 1000 ns across
100 iterations rather than starting at odr_high_time_ns, this performs
unnecessary rounding calls at the nominal 48 MHz clock (where
odr_high_time_ns is 125 ns) and will always exhaust count and fail with
-EDOM if st->sys_clk_hz < 6 MHz. Should target_ns start at odr_high_time_ns
and check if (ret < 0)?

> +
> +	/* Check the minimum ODR high time is met */
> +	if (odr_wf.duty_length_ns < odr_high_time_ns)
> +		return -EDOM;
> +
> +	if (odr_wf.period_length_ns < 2 * odr_high_time_ns)
> +		return -EDOM;
> +
> +	/*
> +	 * Configure SPI offload PWM trigger.
> +	 * For gated DCLK, the minimum required time between ODR rising edge
> +	 * and DCLK rising edge is the sum of ODR high time and ODR falling
> +	 * edge to DCLK rising edge time. Delay the offload trigger for at least
> +	 * that amount of time so the ADC sample data will be available when the
> +	 * SPI transfer begin.
> +	 *
> +	 * Use the same period as ODR PWM to avoid timing issues.
> +	 * Convert back from period to frequency for the SPI offload API.
> +	 */
> +	offload_period_ns = odr_wf.period_length_ns;
> +	config.periodic.frequency_hz = DIV_ROUND_UP_ULL(HZ_PER_GHZ, offload_period_ns);
> +	offload_offset_ns = odr_high_time_ns + AD4134_DCLK_RISING_OFFSET_NS;
> +	count = 100;
> +	do {
> +		config.periodic.offset_ns = offload_offset_ns;
> +		ret = spi_offload_trigger_validate(st->offload_trigger, &config);
> +		if (ret)
> +			return ret;
> +
> +		offload_offset_ns += 10;
> +	} while (--count && config.periodic.offset_ns < odr_high_time_ns +
> +							AD4134_DCLK_RISING_OFFSET_NS);
> +
> +	/* Check the minimum ODR to DCLK delay is met */
> +	if (config.periodic.offset_ns < odr_high_time_ns + AD4134_DCLK_RISING_OFFSET_NS)
> +		return -EDOM;

[Severity: Medium]
Here in ad4134_update_conversion_rate(), offload_offset_ns and the loop
termination checks use the theoretical minimum odr_high_time_ns rather than
the actual rounded ODR pulse width odr_wf.duty_length_ns.

If pwm_round_waveform_might_sleep() rounds odr_wf.duty_length_ns up above
odr_high_time_ns (for example, to 140 ns or 150 ns when odr_high_time_ns is
125 ns), the ODR falling edge occurs at odr_wf.duty_length_ns. Validating
config.periodic.offset_ns against odr_high_time_ns +
AD4134_DCLK_RISING_OFFSET_NS (133 ns) allows the SPI offload trigger to fire
less than 8 ns after (or simultaneously with) the actual ODR falling edge.

Should offload_offset_ns and the post-validation checks use
odr_wf.duty_length_ns + AD4134_DCLK_RISING_OFFSET_NS instead?

> +
> +	/* 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;

[Severity: Medium]
Can this period equality check in ad4134_update_conversion_rate() reject valid
sampling frequencies in the [10, 1496000] Hz range with -EDOM?

In spi_offload_trigger_pwm_validate(), config.periodic.frequency_hz is
converted back to a period via DIV_ROUND_UP_ULL(NSEC_PER_SEC,
periodic->frequency_hz), rounded down by the PWM hardware, and written back to
periodic->frequency_hz:

drivers/spi/spi-offload-trigger-pwm.c:spi_offload_trigger_pwm_validate() {
	wf.period_length_ns = DIV_ROUND_UP_ULL(NSEC_PER_SEC, periodic->frequency_hz);
	...
	ret = pwm_round_waveform_might_sleep(st->pwm, &wf);
	if (ret < 0)
		return ret;

	periodic->frequency_hz = DIV_ROUND_UP_ULL(NSEC_PER_SEC, wf.period_length_ns);
	periodic->offset_ns = wf.duty_offset_ns;
}

When odr_wf.period_length_ns is converted to an integer frequency_hz via
DIV_ROUND_UP_ULL(HZ_PER_GHZ, offload_period_ns), any period where
ceil(10^9 / ceil(10^9 / P)) != P (such as almost all frequencies below
31.6 kHz, e.g., 60 Hz or 19200 Hz) produces a smaller wf.period_length_ns in
spi_offload_trigger_pwm_validate().

In addition, because config.periodic.frequency_hz is set outside the do-while
loop and modified in-place by spi_offload_trigger_validate(), each loop
iteration feeds the rounded-up frequency_hz back into
spi_offload_trigger_validate(), causing the trigger period to shrink further
across iterations and fail the odr_wf.period_length_ns != offload_period_ns
check.

> +
> +	ret = pwm_set_waveform_might_sleep(st->odr_pwm, &odr_wf, false);
> +	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;
> +}
[ ... ]
> @@ -602,6 +783,166 @@ static const struct iio_info ad4134_info = {
>  	.debugfs_reg_access = ad4134_debugfs_reg_access,
>  };
[ ... ]
> +static int ad4134_offload_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4134_state *st = iio_priv(indio_dev);
> +	int ret, access_ret;
> +
> +	ret = ad4134_claim_sample_access(st);
> +	if (ret)
> +		return ret;
> +
> +	ad4134_prepare_offload_msg(indio_dev);
> +	st->msg.offload = st->offload;
> +	ret = spi_optimize_message(st->spi, &st->msg);
> +	if (ret)
> +		goto out_offload_release;
> +
> +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> +					 &st->offload_trigger_config);
> +	if (ret)
> +		goto out_unoptimize;
> +
> +	return 0;
> +
> +out_unoptimize:
> +	spi_unoptimize_message(&st->msg);
> +
> +out_offload_release:
> +	access_ret = ad4134_release_sample_access(st);
> +	if (access_ret)
> +		dev_err(&st->spi->dev, "reg input select error: %d\n", access_ret);
> +
> +	return ret;
> +}
> +
> +static int ad4134_offload_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct ad4134_state *st = iio_priv(indio_dev);
> +
> +	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> +	spi_unoptimize_message(&st->msg);
> +
> +	return ad4134_release_sample_access(st);
> +}
> +
> +static const struct iio_buffer_setup_ops ad4134_offload_buffer_setup_ops = {
> +	.postenable = &ad4134_offload_buffer_postenable,
> +	.predisable = &ad4134_offload_buffer_predisable,
> +};
> +
> +static int ad4134_pwm_get(struct ad4134_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +
> +	st->odr_pwm = devm_pwm_get(dev, NULL);
> +	if (IS_ERR(st->odr_pwm))
> +		return dev_err_probe(dev, PTR_ERR(st->odr_pwm),
> +				     "failed to get ODR PWM\n");
> +
> +	return 0;
> +}

[Severity: High]
How is the lifecycle of st->odr_pwm managed?

During probe, ad4134_offload_setup() calls ad4134_update_conversion_rate(),
which calls pwm_set_waveform_might_sleep(st->odr_pwm, &odr_wf, false) with a
non-zero duty cycle and period. This immediately enables the ODR PWM output
before the AD4134 registers are configured.

Furthermore, st->odr_wf is saved in ad4134_update_conversion_rate() but never
used, and neither ad4134_offload_buffer_predisable() nor any devm cleanup
action ever disables st->odr_pwm. Because devm_pwm_get() only releases the
PWM reference without stopping hardware output, won't st->odr_pwm remain
running continuously when the buffer is disabled, or after late probe failure
and driver unbind (driving the ODR pin after the ADC's clocks and regulators
have been disabled)?

[ ... ]

> +static int ad4134_offload_setup(struct iio_dev *indio_dev, struct ad4134_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	int ret;
> +
> +	ret = ad4134_pwm_get(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4134_offload_buffer_setup(indio_dev, st->spi);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Start with a sampling rate slower than 374 kSPS because that is the
> +	 * maximum rate supported with wideband filter (default on power up).
> +	 */
> +	st->odr_hz = 250 * HZ_PER_KHZ;
> +	ret = ad4134_update_conversion_rate(st, st->odr_hz);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to set sampling freq\n");

[ ... ]

> +}

[ ... ]

> @@ -776,17 +1117,40 @@ static int ad4134_probe(struct spi_device *spi)

[ ... ]

> +	st->offload = devm_spi_offload_get(dev, spi, &ad4134_offload_config);
> +	ret = PTR_ERR_OR_ZERO(st->offload);
> +	/* Fall back to low speed usage when no SPI offload is available. */
> +	if (ret == -ENODEV) {

[ ... ]

> +	} else if (ret) {
> +		return dev_err_probe(dev, ret, "failed to get offload\n");
> +	} else {
> +		if (st->spi_mode == AD4134_SPI_MODE_NO_CS)
> +			return dev_err_probe(dev, -EPROTONOSUPPORT,
> +					     "Unsupported min I/O + offload config\n");
> +
> +		indio_dev->info = &ad4134_offload_info;
> +		indio_dev->channels = ad4134_offload_chan_set;
> +		indio_dev->num_channels = ARRAY_SIZE(ad4134_offload_chan_set);
> +		ret = ad4134_offload_setup(indio_dev, st);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to setup SPI offload\n");
> +	}

[ ... ]

>  	return devm_iio_device_register(dev, indio_dev);
>  }

[ ... ]

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