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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unconditional propagation of -ENODEV breaks driver probe on systems without SPI offload
- [High] Silent failure in SPI offload trigger offset validation loop risks data corruption
- [Medium] Incomplete state rollback on failed conversion rate validation corrupts driver state
- [Low] Missing source file causes build failure
- [Low] Accumulative drift of PWM period due to reusing modified waveform structure
--

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

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

Make use of SPI transfer offloading to speed up data capture, enabling data
acquisition at faster sample rates (up to 2 MSPS).

> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 2b82034513673..f96d9262b891d 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -944,6 +944,9 @@ config LTC2378
>  	tristate "Analog Devices LTC2378 ADC driver"
>  	depends on SPI
>  	depends on GPIOLIB
> +	select LTC2378_LIB
> +	select LTC2378_LIB_OFFLOAD_BUFFER if SPI_OFFLOAD && PWM && SPI_OFFLOAD_TRIGGER_PWM && IIO_BUFFER && IIO_BUFFER_DMAENGINE
> +	select LTC2378_LIB_TRIGGERED_BUFFER if IIO_BUFFER

[ ... ]

> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 1814fb78dde30..109cd39237c9d 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -82,6 +82,12 @@ obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>  obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
>  obj-$(CONFIG_LTC2309) += ltc2309.o
>  obj-$(CONFIG_LTC2378) += ltc2378.o
> +
> +ltc2378_lib-y += ltc2378-lib-core.o
> +ltc2378_lib-$(CONFIG_LTC2378_LIB_OFFLOAD_BUFFER) += ltc2378-offload-buffer.o
> +ltc2378_lib-$(CONFIG_LTC2378_LIB_TRIGGERED_BUFFER) += ltc2378-triggered-buffer.o

[Severity: Low]
Will this break the build since ltc2378-triggered-buffer.c is not included
in this patch?

The patch adds CONFIG_LTC2378_LIB_TRIGGERED_BUFFER and references
ltc2378-triggered-buffer.o in the Makefile, which is automatically selected if
IIO_BUFFER is enabled. However, the corresponding ltc2378-triggered-buffer.c
file is missing.

This regression was fixed later in the series in commit
975300646e8809a19fe7c482edd698d128476317 ("iio: adc: ltc2378: Enable triggered
buffer data capture"), but shouldn't each patch in the series compile
independently to preserve bisectability?

[ ... ]

> diff --git a/drivers/iio/adc/ltc2378-lib-core.c b/drivers/iio/adc/ltc2378-lib-core.c
> new file mode 100644
> index 0000000000000..1160f4324d019
> --- /dev/null
> +++ b/drivers/iio/adc/ltc2378-lib-core.c
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Analog Devices LTC2378 ADC series driver
> + *
> + * Copyright (C) 2026 Analog Devices Inc.
> + * Author: Marcelo Schmitt <marcelo.schmitt@analog.com>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/iio/iio.h>
> +
> +#include "ltc2378.h"
> +
> +int ltc2378_lib_buffer_setup(struct iio_dev *indio_dev, struct ltc2378_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	int ret;
> +
> +	ret = __ltc2378_set_offload_ops(st);
> +	if (ret == -EOPNOTSUPP)
> +		return 0; /* Let device setup complete without buffer support */
> +
> +	if (!ret)
> +		ret = st->ops->buffer_setup(indio_dev, st);
> +
> +	if (ret)
> +		return dev_err_probe(dev, ret, "error on SPI offload setup\n");
> +
> +	return 0;
> +}

[ ... ]

> diff --git a/drivers/iio/adc/ltc2378-offload-buffer.c b/drivers/iio/adc/ltc2378-offload-buffer.c
> new file mode 100644
> index 0000000000000..3e8c7ab64a54b
> --- /dev/null
> +++ b/drivers/iio/adc/ltc2378-offload-buffer.c
> @@ -0,0 +1,305 @@

[ ... ]

> +static int ltc2378_offload_buffer_setup(struct iio_dev *indio_dev,
> +					struct ltc2378_state *st)
> +{
> +	struct spi_device *spi = st->spi;
> +	struct device *dev = &spi->dev;
> +	int ret;
> +
> +	st->offload = devm_spi_offload_get(dev, spi, &ltc2378_offload_config);
> +	ret = PTR_ERR_OR_ZERO(st->offload);
> +	if (ret)
> +		return ret;

[Severity: High]
Does this unconditionally propagate -ENODEV and break the driver probe on
systems without SPI offload?

When CONFIG_LTC2378_LIB_OFFLOAD_BUFFER is enabled, the driver attempts to set
up the SPI offload buffer. If the system hardware does not support SPI offload,
devm_spi_offload_get() returns -ENODEV.

Because ltc2378_offload_buffer_setup() returns this error,
ltc2378_lib_buffer_setup() will pass it to dev_err_probe() and fail the entire
ADC probe. Should the driver treat -ENODEV as a signal to bypass offload setup
and gracefully fall back to triggered buffer or direct mode?

[ ... ]

> +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 = 0;
> +	u64 offload_offset_ns;
> +	int ret;
> +
> +	if (freq_Hz == 0)
> +		return -EINVAL;
> +
> +	if (freq_Hz < 1 || freq_Hz > 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);
> +
> +	/*
> +	 * Ensure CNV high time meets minimum requirement (20ns). The PWM
> +	 * hardware may round the duty cycle, so iterate until we get at least
> +	 * the minimum required high time.
> +	 */
> +	do {
> +		cnv_wf.duty_length_ns = target;
> +		ret = pwm_round_waveform_might_sleep(st->cnv_trigger, &cnv_wf);
> +		if (ret)
> +			return ret;
> +		target += 10;  /* Increment by PWM duty cycle period */
> +	} while (count++ < 100 && cnv_wf.duty_length_ns < LTC2378_TCNV_HIGH_NS);

[Severity: Low]
Could this cause an accumulative drift of the PWM period?

In this loop, pwm_round_waveform_might_sleep(&cnv_wf) can modify fields in
cnv_wf based on hardware rounding capabilities. Because cnv_wf.period_length_ns
is not reset to the original target period at the beginning of each iteration,
any rounding applied to the period in one iteration is passed into the next,
which might drift the PWM period away from the requested frequency.

> +
> +	/* Double check the minimum CNV high time is met */
> +	if (cnv_wf.duty_length_ns < LTC2378_TCNV_HIGH_NS)
> +		return -EIO;
> +
> +	/*
> +	 * 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 = 0;
> +	do {
> +		config->periodic.offset_ns = offload_offset_ns;
> +		ret = spi_offload_trigger_validate(st->offload_trigger, config);
> +		if (ret)
> +			return ret;

[Severity: Medium]
Does returning here leave the driver in an inconsistent state?

The pointer config directly points to the device state
st->offload_trigger_config. If validation fails, returning an error immediately
leaves st->offload_trigger_config containing the rejected values, while
st->cnv_wf and st->cnv_Hz retain their old values.

Subsequent buffer enable operations might try to pair the old PWM waveform
with the corrupted offload trigger config. Shouldn't the state be modified
on a local copy and only committed upon success?

> +		offload_offset_ns += 10;
> +	} while (count++ < 100 && config->periodic.offset_ns < min_read_offset);
> +
> +	st->cnv_wf = cnv_wf;
> +	st->cnv_Hz = DIV_ROUND_CLOSEST_ULL(HZ_PER_GHZ, cnv_wf.period_length_ns);
> +
> +	return 0;

[Severity: High]
Is it possible for this to silently return 0 on a failed loop condition?

The loop iterates up to 100 times to find an SPI offload trigger offset >=
min_read_offset. If the underlying hardware driver continually rounds the
offset down such that it never reaches min_read_offset, the loop drops out
after 100 iterations.

The function then unconditionally returns 0 (success). This would configure
the SPI offload to trigger an SPI read before the ADC finishes its conversion,
leading to corrupted data. Should this check if
config->periodic.offset_ns >= min_read_offset after the loop and return an
error if it fails?

> +}

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