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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The driver fails to right-shift >16-bit ADC data read via SPI into a 32-bit word, corrupting both the raw value and its sign extension.
- [Medium] The Kconfig specifies a spurious dependency on `PWM`, which allows the driver to compile without `GPIOLIB` and silently fail to trigger ADC conversions in direct mode.
- [Medium] The driver explicitly forces `bits_per_word` to 32 or 16, which arbitrarily restricts compatibility with many SPI controllers.
- [Low] Using the `IIO_DEV_ACQUIRE_DIRECT_MODE` cleanup macro inside a `switch` case without a block scope causes a compiler error.
--

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

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

This commit adds support for the Analog Devices LTC2378-20 and similar
analog-to-digital converters to the IIO subsystem.

> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -939,6 +939,18 @@ config LTC2309
>  
> +config LTC2378
> +	tristate "Analog Devices LTC2378 ADC driver"
> +	depends on SPI
> +	depends on GPIOLIB || PWM

[Severity: Medium]
Does this spurious dependency on PWM allow the driver to compile without
GPIOLIB? 

If GPIOLIB is disabled, devm_gpiod_get_optional() will return NULL. When 
ltc2378_convert_and_acquire() subsequently calls gpiod_set_value_cansleep(),
it will silently do nothing.

Will this prevent the ADC conversions from being initiated in direct mode, 
causing the SPI transfer to silently read stale data or zeroes?

[ ... ]
> diff --git a/drivers/iio/adc/ltc2378.c b/drivers/iio/adc/ltc2378.c
> --- /dev/null
> +++ b/drivers/iio/adc/ltc2378.c
[ ... ]
> +static int ltc2378_channel_single_read(const struct iio_chan_spec *chan,
> +				       struct ltc2378_state *st, int *val)
> +{
> +	const struct iio_scan_type *scan_type = &chan->scan_type;
> +	u32 sample;
> +	int ret;
> +
> +	ret = ltc2378_convert_and_acquire(st);
> +	if (ret)
> +		return ret;
> +
> +	if (scan_type->realbits > 16)
> +		sample = st->scan.data.sample_buf32;
> +	else
> +		sample = st->scan.data.sample_buf16;
> +
> +	if (scan_type->format == IIO_SCAN_FORMAT_SIGNED_INT)
> +		*val = sign_extend32(sample, scan_type->realbits - 1);

[Severity: High]
Are we missing a right-shift for >16-bit ADCs? 

When bits_per_word is set to 32, the SPI controller clocks in 32 bits
MSB-first and stores them in CPU native endianness. The valid 18 or 20 bits
will occupy the upper bits of the 32-bit word, leaving trailing zeroes.

Doesn't passing this unshifted sample directly to sign_extend32() evaluate
the wrong bit for the sign extension, and also magnify the returned value
by a factor of 2^14?

Should we also populate scan_type.shift in the probe function so userspace
buffer consumers don't end up reading unshifted garbage?

> +	else
> +		*val = sample;
> +
> +	return 0;
> +}
> +
> +static int ltc2378_read_raw(struct iio_dev *indio_dev,
> +			    const struct iio_chan_spec *chan,
> +			int *val, int *val2, long info)
> +{
> +	struct ltc2378_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);

[Severity: Low]
Does this missing block scope cause a compiler error? 

The IIO_DEV_ACQUIRE_DIRECT_MODE macro expands to a cleanup variable
declaration. Because the switch case lacks an enclosing block, jumping to
case IIO_CHAN_INFO_SCALE skips the variable's initialization, which compilers
like GCC reject.

While I noticed this is fixed in a later patch in the series, doesn't this
break git bisectability by causing this specific commit to fail compilation?

> +		if (IIO_DEV_ACQUIRE_FAILED(claim))
> +			return -EBUSY;
> +
> +		ret = ltc2378_channel_single_read(chan, st, val);
[ ... ]
> +static int ltc2378_probe(struct spi_device *spi)
> +{
[ ... ]
> +	st->chans[0].scan_type.realbits = st->info->resolution;
> +	st->chans[0].scan_type.storagebits = st->info->resolution > 16 ? 32 : 16;
> +
> +	st->xfer.rx_buf = &st->scan.data;
> +	st->xfer.len = BITS_TO_BYTES(st->chans[0].scan_type.storagebits);
> +	st->xfer.bits_per_word = st->info->resolution > 16 ? 32 : 16;

[Severity: Medium]
Does forcing bits_per_word to 32 or 16 arbitrarily restrict compatibility
with standard SPI controllers?

Some standard SPI controllers (like the bcm2835 SPI) only support 8-bit
transfers. When 32-bit or 16-bit words are requested, the SPI core's
__spi_validate() checks the controller capabilities and fails the transfer
with -EINVAL.

Would it be safer to use standard 8-bit transfers and reconstruct the word
using get_unaligned_be32()?

> +
> +	indio_dev->channels = st->chans;

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