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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] iio: adc: ad4134: premature -EDOM in ad4134_update_conversion_rate() offset loop
- [Medium] iio: adc: ad4134: st->odr_pwm left enabled on error, buffer disable, and unbind
--

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

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

This commit adds SPI offload support to the ad4134 ADC driver to allow
high-speed data capture up to 1.496 MSPS using a PWM-driven ODR signal and
DMA-backed IIO buffers.

> diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> index 4efb63c45aa29..bc6407ae25e06 100644
> --- a/drivers/iio/adc/ad4134.c
> +++ b/drivers/iio/adc/ad4134.c

[ ... ]

> @@ -462,6 +516,94 @@ 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 max_odr_high_time_ns;
> +	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 = odr_high_time_ns;
> +	count = 100;
> +	do {
> +		odr_wf.duty_length_ns = target_ns;
> +		ret = pwm_round_waveform_might_sleep(st->odr_pwm, &odr_wf);
> +		if (ret < 0)
> +			return ret;
> +		target_ns += 10; /* Increment by PWM duty cycle period */
> +	} while (--count && odr_wf.duty_length_ns < odr_high_time_ns);
> +
> +	/* 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 */
> +	max_odr_high_time_ns = max_t(u64, odr_high_time_ns, odr_wf.duty_length_ns);
> +	if (config.periodic.offset_ns < max_odr_high_time_ns + AD4134_DCLK_RISING_OFFSET_NS)
> +		return -EDOM;

[Severity: High]
Can this loop in ad4134_update_conversion_rate() exit early and cause the
subsequent check to fail with -EDOM?

After the ODR PWM rounding loop finishes, odr_wf.duty_length_ns is guaranteed
to be at least odr_high_time_ns (for example, 125 ns at a 48 MHz sys_clk_hz
rounded up to 130 ns or 140 ns depending on the PWM clock period), so
max_odr_high_time_ns is equal to odr_wf.duty_length_ns.

However, the SPI offload trigger validation loop initializes offload_offset_ns
to odr_high_time_ns + AD4134_DCLK_RISING_OFFSET_NS and stops as soon as
config.periodic.offset_ns reaches odr_high_time_ns +
AD4134_DCLK_RISING_OFFSET_NS.

When odr_wf.duty_length_ns was rounded above odr_high_time_ns and
spi_offload_trigger_validate() rounds config.periodic.offset_ns to a value in
[odr_high_time_ns + AD4134_DCLK_RISING_OFFSET_NS, max_odr_high_time_ns +
AD4134_DCLK_RISING_OFFSET_NS), the loop terminates on the first iteration and
the check against max_odr_high_time_ns + AD4134_DCLK_RISING_OFFSET_NS
immediately returns -EDOM, failing both ad4134_probe() ->
ad4134_offload_setup() and ad4134_write_raw().

Should max_odr_high_time_ns be computed before the loop and used for both the
initial offload_offset_ns and the loop termination condition (and should
config.periodic.frequency_hz be re-initialized on each iteration in case
spi_offload_trigger_validate() modifies it in place)?

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

[ ... ]

> @@ -603,6 +783,166 @@ static const struct iio_info ad4134_info = {

[ ... ]

> +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: Medium]
Does this leave st->odr_pwm enabled when the buffer is disabled, if probe
fails after ad4134_update_conversion_rate(), or when the driver is unbound?

In ad4134_probe() -> ad4134_offload_setup() -> ad4134_update_conversion_rate(),
pwm_set_waveform_might_sleep(st->odr_pwm, &odr_wf, false) is called with a
non-zero duty cycle and period, which immediately enables the PWM output in
hardware. It is also saved to st->odr_wf, which is never read anywhere else
in the driver.

Because devm_pwm_get() only releases the PWM handle on teardown without
disabling the hardware output, and neither a devm_add_action_or_reset()
callback nor ad4134_offload_buffer_predisable() disables st->odr_pwm, the PWM
continues toggling the AD4134 ODR pin while the IIO buffer is idle, if a
subsequent step in ad4134_offload_setup() or ad4134_probe() fails, or after
driver unbind when the ADC regulators and clocks have been disabled.

Should st->odr_pwm be disabled via a devm action (or only enabled between
ad4134_offload_buffer_postenable() and ad4134_offload_buffer_predisable()
using the saved st->odr_wf)?

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