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

[Jonathan Cameron]

On Thu, 28 May 2026 12:03:52 -0300
Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx> wrote:

> Support for LTC2378-20 and similar analog-to-digital converters.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>

As normal, our friendly AI had something to say:
https://sashiko.dev/#/patchset/cover.1779976379.git.marcelo.schmitt%40analog.com


A few other things inline from a fresh read.

>  M:	Michael Hennerich <michael.hennerich@xxxxxxxxxx>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 8550917226a1..70fec8e3e891 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -939,6 +939,18 @@ 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 || PWM

Triggered sashiko:
https://sashiko.dev/#/patchset/cover.1779976379.git.marcelo.schmitt%40analog.com
This dependency is odd enough that I think I'd add a comment on why.
Also bring in the dependency on PWM in the patch that needs it not this one.

Will it build without gpiolib? If so I'd prefer that to be a || COMPILE_TEST
just to get a little more coverage.


> +	select IIO_BUFFER
This doesn't seem to be needed yet either.

> +	help
> +	  Say yes here to build support for Analog Devices LTC2378-20 and
> +	  similar analog to digital converters.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called ltc2378.
> +

> diff --git a/drivers/iio/adc/ltc2378.c b/drivers/iio/adc/ltc2378.c
> new file mode 100644
> index 000000000000..bdff98157979
> --- /dev/null
> +++ b/drivers/iio/adc/ltc2378.c
> @@ -0,0 +1,342 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Analog Devices LTC2378 ADC series driver
> + *
> + * Copyright (C) 2026 Analog Devices Inc.
> + * Author: Ioan-Daniel Pop <pop.ioan-daniel@xxxxxxxxxx>
> + * Author: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>

dev_prink.h missing for dev_err_probe()

> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>
> +
> +#include "ltc2378.h"

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

Should we check for errors on setting the gpio?

> +	fsleep(4);
> +	ret = spi_sync_transfer(st->spi, &st->xfer, 1);
> +	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> +
> +	return ret;
> +}
> +
> +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;

Sashikio has an interesting theory here that you need a shift. I can't immediately
spot why it is wrong.  Given we are clocking in 32bits I assume the first
res_bits are valid then we get a bunch of garbage after that. Given it's MSB first
the SPI controller should put those in the top bits of the resulting u32 with the
garbage at the bottom. So shouldn't we shift them down by 32 - resolution or something
like that? + in buffered case provide the relevant shift value (in later patches)

> +	else
> +		sample = st->scan.data.sample_buf16;
> +
> +	if (scan_type->format == IIO_SCAN_FORMAT_SIGNED_INT)
> +		*val = sign_extend32(sample, scan_type->realbits - 1);
> +	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:

case IIO_CHAN_INFO_RAW: {

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

As that IIO_DEV_ACQUIRE_DIRECT_MODE() is declaring a local variable
so there is something go out of scope and result in cleanup.
LLVM moans about this, GCC just does the wrong thing which can in theory
at least result in an underflow.
You'll get build bot warnings on this - bug sashiko caught it.

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



> +
> +MODULE_AUTHOR("Ioan-Daniel Pop <pop.ioan-daniel@xxxxxxxxxx>");

I'm a bit curious on this - listing an module author but not tags?  Maybe
provide some context in the patch description or a tag if appropriate.

> +MODULE_AUTHOR("Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("Analog Devices LTC2378 ADC series driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/iio/adc/ltc2378.h b/drivers/iio/adc/ltc2378.h
> new file mode 100644
> index 000000000000..399e8f67cd0e
> --- /dev/null
> +++ b/drivers/iio/adc/ltc2378.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Analog Devices LTC2378 and similar ADCs common definitions and properties
> + * Copyright (C) 2026 Analog Devices, Inc.
> + * Author: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
> + */
> +
> +#ifndef __DRIVERS_IIO_ADC_LTC2378_H__
> +#define __DRIVERS_IIO_ADC_LTC2378_H__
> +
> +#include <linux/iio/iio.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#define LTC2378_TDSDOBUSYL_NS		5
> +#define LTC2378_TBUSYLH_NS		13
> +#define LTC2378_TCNV_HIGH_NS		20
> +
> +struct ltc2378_chip_info {
> +	const char *name;
> +	int resolution;
> +	bool twos_comp; /* Output code is 2's complement or straight binary */

I think this would be more intuitive if was the terminology of the datasheet
so bipolar (vs unipolar).  Or is there a case where something more complex is
going on like offset binary rather than 2s comp?


> +};
> +
> +struct ltc2378_state {
> +	const struct ltc2378_chip_info *info;
> +	struct gpio_desc *cnv_gpio;
> +	struct spi_device *spi;
> +	struct spi_transfer xfer;
> +	struct iio_chan_spec chans[2]; /* 1 physical chan + 1 timestamp chan */
> +	int ref_uV;
> +
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the
> +	 * transfer buffers to live in their own cache lines.
> +	 */
> +	struct {
> +		union {
> +			u16 sample_buf16;
> +			u32 sample_buf32;
> +		} data;
> +		aligned_s64 timestamp;
> +	} scan __aligned(IIO_DMA_MINALIGN);
> +};
> +
> +#endif /* __DRIVERS_IIO_ADC_LTC2378_H__ */