Re: [PATCH v1 2/4] iio: adc: 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] Incorrect bit shift for 18-bit and 20-bit resolutions causes data corruption.
- [High] Missing wait for conversion completion before SPI read leads to hardware timing violation.
- [High] Regulator error code is overridden, breaking probe deferral.
- [Low] Un-scoped guard declaration inside switch statement causes a compilation error.
--

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

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

Initial support for LTC2378-20 and similar analog-to-digital converters.

> diff --git a/drivers/iio/adc/ltc2378.c b/drivers/iio/adc/ltc2378.c
> new file mode 100644
> index 0000000000000..7916500c470cf
> --- /dev/null
> +++ b/drivers/iio/adc/ltc2378.c

[ ... ]

> +static 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);
> +	ret = spi_sync_transfer(st->spi, &st->xfer, 1);

The ADC requires a minimum conversion time after the rising edge of the
CNV signal before data can be clocked out.

If spi_sync_transfer() is called immediately, could the SPI controller start
clocking the SDO line while the ADC is still converting? Should there be an
explicit delay here (e.g., using ndelay()) to ensure the conversion is
complete?

[ ... ]

> +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);

IIO_DEV_ACQUIRE_DIRECT_MODE() declares a scoped variable using the __cleanup__
attribute. Because it is placed directly under the case label without an
enclosing block, its scope extends to the entire switch statement. 

If info evaluates to IIO_CHAN_INFO_SCALE, the jump bypasses the variable
initialization. Does this cause a compilation error? Wrapping the case block
in braces would resolve this.

> +		if (IIO_DEV_ACQUIRE_FAILED(claim))
> +			return -EBUSY;
> +
> +		ret = ltc2378_channel_single_read(chan, st, val);

[ ... ]

> +static int ltc2378_probe(struct spi_device *spi)
> +{
> +	struct iio_chan_spec *ltc2378_chan;
> +	struct device *dev = &spi->dev;
> +	unsigned int num_iio_chans = 1;
> +	struct iio_dev *indio_dev;
> +	struct ltc2378_state *st;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +
> +	st->ref_uV = devm_regulator_get_enable_read_voltage(dev, "ref");
> +	if (st->ref_uV < 0)
> +		return dev_err_probe(dev, -ENODEV, "failed to read ref regulator\n");

devm_regulator_get_enable_read_voltage() can return -EPROBE_DEFER if the
regulator is not yet initialized. By explicitly passing -ENODEV to
dev_err_probe() instead of propagating st->ref_uV, does this break the probe
deferral mechanism for the regulator?

> +
> +	st->info = spi_get_device_match_data(spi);

[ ... ]

> +	*ltc2378_chan = (struct iio_chan_spec) {
> +		.type = IIO_VOLTAGE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_type = {
> +			.format = st->info->out_format,
> +			.realbits = st->info->resolution,
> +			.storagebits = st->info->resolution > 16 ? 32 : 16,
> +			.shift = st->info->resolution > 16 ? 8 : 0,

For ADCs with >16 bits resolution, storagebits is 32 (4 bytes). Because the
ADC clocks out data MSB-first, the 18 or 20 bits of data are left-aligned in
the 32-bit word.

To properly right-align the data, shouldn't the shift be 32 - realbits
(14 for 18-bit, 12 for 20-bit) instead of a hardcoded 8? Does leaving the data
misaligned cause sign_extend32() to evaluate the wrong bit as the sign bit
later on, corrupting the sample values?

> +			.endianness = IIO_BE,
> +		},
> +	};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1779117444.git.marcelo.schmitt1@gmail.com?part=2