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

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

On 07/02, sashiko-bot@kernel.org wrote:
> 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?
Makes sense. 
> 
> Should the conversion-and-read sequence be wrapped in spi_bus_lock()
> and spi_bus_unlock()?
Because these chips don't have CS line, it seems that locking the bus actually
makes sense.

> 
> > +	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?
No.
> 
> 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.
See 'Using a Single LTC23... in Normal Mode' diagrams (usually Figure 10, 13, or 15
in datasheets). CNV can be either low or high during data transfers.

> 
> > +	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.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/iio/iio.h?h=v7.2-rc1#n785
Outdated LLM?

> 
> > +
> > +		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?
That's taken care of in ltc2378_refin_setup().

> 
> > +		/*
> > +		 * 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.
Indeed,
-       .internal_ref_uV = 4096000,
+       .internal_ref_uV = 2048000,
will solve the issue.

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