Re: [PATCH v3 2/5] iio: adc: ltc2378: Add support for LTC2378-20 and similar ADCs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unsupported SPI bits_per_word values for 18-bit and 20-bit ADCs.
- [High] Logic Error: Keeping CNV high during SPI transfer outputs BUSY flags instead of data.
- [High] Data Corruption: Buffered reads for 16-bit parts always return 0 on big-endian architectures.
- [Medium] ABI Violation: Non-standard sysfs attribute name without channel index.
- [Medium] Missing Kconfig dependency on REGULATOR causes unconditional probe failure.
--

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

iio: adc: ltc2378: Add support for LTC2378-20 and similar ADCs

Support for LTC2378-20 and similar analog-to-digital converters. Co-developed
by Ioan-Daniel Pop.

> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index a3a93a47b43d9..2b82034513673 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
[ ... ]
> @@ -940,6 +940,17 @@ config LTC2309
>  	  This driver can also be built as a module. If so, the module will
>  	  be called ltc2309.
>  
> +config LTC2378
> +	tristate "Analog Devices LTC2378 ADC driver"
> +	depends on SPI
> +	depends on GPIOLIB

[Severity: Medium]
Should this Kconfig entry also depend on REGULATOR?

The probe function unconditionally calls devm_regulator_get_enable_read_voltage()
and fails if it returns an error. If the kernel is built with CONFIG_REGULATOR
disabled, the regulator API is stubbed out to return -ENODEV, making it
structurally impossible for the driver to successfully probe.

> diff --git a/drivers/iio/adc/ltc2378.c b/drivers/iio/adc/ltc2378.c
> new file mode 100644
> index 0000000000000..88582bdcd6a62
> --- /dev/null
> +++ b/drivers/iio/adc/ltc2378.c
[ ... ]
> +static int ltc2378_probe(struct spi_device *spi)
> +{
[ ... ]
> +	st->chans[st->num_iio_chans++] = (struct iio_chan_spec) {
> +		.type = IIO_VOLTAGE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = 0,

[Severity: Medium]
Does this channel specification need .indexed = 1 and .channel = 0?

Without the index, the IIO core exposes the sysfs attribute as in_voltage_raw
instead of in_voltage0_raw. The IIO ABI documentation strictly mandates that
the number must always be specified and unique to allow association with event
codes, so this non-standard naming breaks compatibility with generic userspace
clients.

> +		.scan_type = {
> +			.format = st->info->bipolar ? IIO_SCAN_FORMAT_SIGNED_INT :
> +						      IIO_SCAN_FORMAT_UNSIGNED_INT,
> +			.realbits = st->info->resolution,
> +			/*
> +			 * Buffer elements could be 16-bit for low precision
> +			 * parts. Though, using more storage bits allows keeping
> +			 * the same scan_type configuration for both types of
> +			 * buffer support.
> +			 */
> +			.storagebits = 32,
> +		},

[Severity: High]
Will this configuration cause buffered reads for 16-bit parts to return 0 on
big-endian architectures?

For 16-bit parts, the SPI transfer writes 2 bytes to the 32-bit union. On
big-endian architectures, these 2 bytes are written to the most significant
bytes of the 32-bit space. However, setting storagebits = 32 with an implicit
shift of 0 causes the IIO core to extract the lower 16 bits (which remain 0).
This causes the IIO core to persistently push 0 to the buffers.

> +	};
> +
> +	st->xfer.rx_buf = &st->scan.data;
> +	st->xfer.len = st->info->resolution > 16 ? 4 : 2;
> +	st->xfer.bits_per_word = st->info->resolution;

[Severity: High]
Is it safe to set bits_per_word directly to 18 or 20?

The SPI core validates bits_per_word against the controller's bits_per_word_mask
during transfer. Since the vast majority of SPI controllers only support 8, 16,
or 32 bits per word, setting this to 18 or 20 will cause spi_sync_transfer() to
immediately fail with -EINVAL, breaking data capture on standard hardware.

> diff --git a/drivers/iio/adc/ltc2378.h b/drivers/iio/adc/ltc2378.h
> new file mode 100644
> index 0000000000000..a3a69351de6ce
> --- /dev/null
> +++ b/drivers/iio/adc/ltc2378.h
[ ... ]
> +static inline int ltc2378_convert_and_acquire(struct ltc2378_state *st)
> +{
> +	int ret;
> +
> +	/* Cause a rising edge of CNV to initiate a new ADC conversion */
> +	gpiod_set_value_cansleep(st->cnv_gpio, 1);
> +	fsleep(4);
> +	ret = spi_sync_transfer(st->spi, &st->xfer, 1);
> +	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> +
> +	return ret;
> +}

[Severity: High]
Does keeping the CNV pin high during the SPI transfer cause the ADC to output
BUSY flags instead of conversion data?

For the LTC2378 series, bringing the Chip Select low while CNV is still high
places the ADC into a BUSY indicator mode instead of shifting out the sampled
data. Should CNV be brought low before initiating the SPI transfer?

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