Re: [PATCH v5 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] Hardware protocol violation: CNV is held high during SPI read
- [High] SPI bus not locked during ADC conversion phase
- [Medium] Incorrect scale calculation for LTC2338-18 and similar ADCs
- [Low] Undefined direct mode macros lead to build failure
--

commit b74b536d18af0084ea31bd1e1e8cf9454da5e29c
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. It introduces a new IIO driver using the
SPI subsystem.

> diff --git a/drivers/iio/adc/ltc2378.c b/drivers/iio/adc/ltc2378.c
> new file mode 100644
> index 0000000000000..b5cf2e974daca
> --- /dev/null
> +++ b/drivers/iio/adc/ltc2378.c
> @@ -0,0 +1,404 @@

[ ... ]

> +static const struct ltc2378_chip_info ltc2338_18_chip_info = {
> +	.name = "ltc2338-18",
> +	.internal_ref_uV = 4096000,
> +	.chan = LTC2378_BIPOLAR_DIFF_CHANNEL(18),
> +};

[ ... ]

> +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);
> +	fsleep(4);

[Severity: High]
Can this sequence cause data corruption on a shared SPI bus?

The CNV pin is toggled and an fsleep(4) occurs before spi_sync_transfer()
locks the bus. If another SPI transaction occurs during this sleep window,
could it toggle the SCK line and prematurely clock data out of the ADC?

Should the conversion-and-read sequence be wrapped in spi_bus_lock()
and spi_bus_unlock()?

> +	ret = spi_sync_transfer(st->spi, &st->xfer, 1);

[Severity: High]
Does reading from the SPI bus while CNV is held high violate the hardware
protocol?

The ADC requires the CNV pin to be brought low to enable the SDO
output before clocking out data via SPI, but spi_sync_transfer() happens
before gpiod_set_value_cansleep(st->cnv_gpio, 0), which might result in
reading high-impedance garbage data.

> +	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> +
> +	return ret;
> +}

[ ... ]

> +static int ltc2378_read_raw(struct iio_dev *indio_dev,
> +			    const struct iio_chan_spec *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct ltc2378_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +		if (IIO_DEV_ACQUIRE_FAILED(claim))
> +			return -EBUSY;

[Severity: Low]
Will this cause a build failure?

The macros IIO_DEV_ACQUIRE_DIRECT_MODE and IIO_DEV_ACQUIRE_FAILED appear
to be undeclared in the kernel tree.

> +
> +		ret = ltc2378_channel_single_read(chan, st, val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	}
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->ref_uV / MILLI;

[Severity: Medium]
Does this calculate an incorrect IIO scale for the LTC2338-18?

Does this miss the 2.5x input span multiplier unique to the LTC2338-18
when calculating the IIO scale here?

> +		/*
> +		 * For all LTC2378-like devices, the amount of bits that express
> +		 * voltage magnitude depend on the polarity / output code format:
> +		 * - straight binary: All precision/resolution bits are used.
> +		 * - 2's complement: One of the precision bits is used for sign.
> +		 */
> +		if (chan->scan_type.format == IIO_SCAN_FORMAT_SIGNED_INT)
> +			*val2 = chan->scan_type.realbits - 1;
> +		else
> +			*val2 = chan->scan_type.realbits;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}

[ ... ]

> +static int ltc2378_refin_setup(struct device *dev, struct ltc2378_state *st)
> +{
> +	int ret;
> +
> +	/*
> +	 * The internal reference buffer amplifies both the internal reference
> +	 * and REFIN by a factor of 2.
> +	 */
> +	ret = devm_regulator_get_enable_read_voltage(dev, "refin");
> +	if (ret == -ENODEV) /* refin is optional */
> +		st->ref_uV = st->info->internal_ref_uV * 2;

[Severity: Medium]
Does this incorrectly double the internal reference voltage?

The ltc2338_18_chip_info already defines internal_ref_uV as the post-buffer
value of 4096000 (4.096V). Multiplying it by 2 here would result in an
incorrect scale value.

> +	else if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to read refin regulator\n");
> +	else
> +		st->ref_uV = ret * 2;
> +
> +	return 0;
> +}

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