Re: [PATCH v6 3/4] iio: adc: ltc2378: Enable high-speed data capture

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `st->offload_trigger_config` structure is partially mutated on the error path, leading to out-of-sync hardware configuration.
- [Medium] Spurious failure in sample frequency configuration due to asymmetric period-to-frequency mathematical conversions.
- [Low] Missing MODULE_IMPORT_NS for the SPI_OFFLOAD namespace.
--

--- Patch [3]: [PATCH v6 3/4] iio: adc: ltc2378: Enable high-speed data capture ---
commit 3324fe468bed00820abda81953b558c8f8c9f5e4
Author: Marcelo Schmitt <marcelo.schmitt@analog.com>

iio: adc: ltc2378: Enable high-speed data capture

This commit updates the ltc2378 ADC driver to support high-speed data
capture. It utilizes SPI transfer offloading to increase data acquisition
rates up to 2 MSPS.

> diff --git a/drivers/iio/adc/ltc2378.c b/drivers/iio/adc/ltc2378.c
> index c9f8b19e0298c..10f1a0c4534be 100644
> --- a/drivers/iio/adc/ltc2378.c
> +++ b/drivers/iio/adc/ltc2378.c
[ ... ]
> @@ -260,7 +359,136 @@ static int ltc2378_read_raw(struct iio_dev *indio_dev,
[ ... ]
> +static int ltc2378_update_conversion_rate(struct ltc2378_state *st, int freq_Hz)
> +{
> +	struct spi_offload_trigger_config *config = &st->offload_trigger_config;
> +	unsigned int min_read_offset, offload_period_ns;
> +	struct pwm_waveform cnv_wf = { };
> +	u64 target = LTC2378_TCNV_HIGH_NS;
> +	unsigned int count;
> +	u64 offload_offset_ns;
> +	int ret;
> +
> +	if (freq_Hz == 0)
> +		return -EINVAL;
> +
> +	if (!in_range(freq_Hz, 1, st->info->max_sample_rate_Hz))
> +		return -ERANGE;
> +
> +	/* Configure CNV PWM waveform */
> +	cnv_wf.period_length_ns = DIV_ROUND_CLOSEST(NSEC_PER_SEC, freq_Hz);
> +
[ ... ]
> +	/*
> +	 * Configure SPI offload PWM trigger.
> +	 * The trigger should fire after tBUSYLH + tCONV + tDSDOBUSYL.
> +	 * Minimum time needed: TBUSYLH (13ns) + TCONV (part-specific) + TDSDOBUSYL (5ns)
> +	 *
> +	 * Use the same period as CNV PWM to avoid timing issues.
> +	 * Convert back from period to frequency for the SPI offload API.
> +	 */
> +	offload_period_ns = cnv_wf.period_length_ns;
> +	config->periodic.frequency_hz = DIV_ROUND_UP(HZ_PER_GHZ, offload_period_ns);
> +	min_read_offset = LTC2378_TBUSYLH_NS + st->info->tconv_ns + LTC2378_TDSDOBUSYL_NS;
> +	offload_offset_ns = min_read_offset;
> +	count = 100;
> +	do {
> +		config->periodic.offset_ns = offload_offset_ns;
> +		ret = spi_offload_trigger_validate(st->offload_trigger, config);
> +		if (ret)
> +			return ret;

[Severity: High]
Does modifying config directly leave the device state out of sync if
validation fails?

Here config aliases to &st->offload_trigger_config, and its frequency_hz and
offset_ns fields are mutated. If spi_offload_trigger_validate() returns an
error, the function returns without restoring the old configuration.

However, st->cnv_wf is only updated at the end of the function. If the
buffer is later enabled, could this result in the PWM running with the old
frequency while the SPI offload engine tries to use the new, rejected
frequency?

> +		offload_offset_ns += 10;
> +	} while (count-- && config->periodic.offset_ns < min_read_offset);
> +
> +	/* Check the minimum CNV to SCLK delay is met */
> +	if (config->periodic.offset_ns < min_read_offset)
> +		return -EDOM;
> +
> +	/* Check the PWM periods remain the same */
> +	offload_period_ns = div64_u64(HZ_PER_GHZ, config->periodic.frequency_hz);
> +	if (cnv_wf.period_length_ns != offload_period_ns)
> +		return -EDOM;

[Severity: Medium]
Could this strict equality check unintentionally reject valid sample
frequencies due to integer math rounding?

If a requested frequency produces a period (cnv_wf.period_length_ns) that
does not perfectly divide 1,000,000,000, DIV_ROUND_UP() might round the
frequency up. Converting that frequency back to a period via div64_u64()
would yield a slightly smaller period.

For example, 133,333 Hz yields a 7500 ns period. 10^9 / 7500 = 133333.33,
which rounds up to 133334. Then 10^9 / 133334 = 7499 ns. Since 7500 != 7499,
this check would fail and return -EDOM.

> +
> +	st->cnv_wf = cnv_wf;
> +	st->cnv_Hz = DIV_ROUND_CLOSEST_ULL(HZ_PER_GHZ, cnv_wf.period_length_ns);
> +
> +	return 0;
> +}
[ ... ]
> @@ -412,3 +789,4 @@ module_spi_driver(ltc2378_driver);
>  MODULE_AUTHOR("Marcelo Schmitt <marcelo.schmitt@analog.com>");
>  MODULE_DESCRIPTION("Analog Devices LTC2378 ADC series driver");
>  MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("IIO_DMAENGINE_BUFFER");

[Severity: Low]
Does this driver also need to import the SPI_OFFLOAD namespace?

Since the driver uses symbols from the SPI offload core (like
devm_spi_offload_get), it seems it should also include
MODULE_IMPORT_NS("SPI_OFFLOAD"); to avoid a modpost warning during strict
module builds.

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