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

[David Lechner <dlechner@baylibre.com>]

On 6/16/26 9:03 PM, Marcelo Schmitt wrote:
> Support for LTC2378-20 and similar analog-to-digital converters.
> 

...

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

add blank line here

> +	st->info = spi_get_device_match_data(spi);
> +	if (!st->info)
> +		return -EINVAL;
> +
> +	indio_dev->name = st->info->name;
> +	indio_dev->info = &ltc2378_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	st->cnv_gpio = devm_gpiod_get(dev, "cnv", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->cnv_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->cnv_gpio),
> +				     "failed to get CNV GPIO");
> +
> +	st->num_iio_chans = 0;
> +	st->chans[st->num_iio_chans++] = (struct iio_chan_spec) {

Why can't we make this static const (part of chip info) like we do in most
drivers?

> +		.type = IIO_VOLTAGE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = 0,
> +		.scan_type = {
> +			.format = st->info->bipolar ? IIO_SCAN_FORMAT_SIGNED_INT :
> +						      IIO_SCAN_FORMAT_UNSIGNED_INT,
> +			.realbits = st->info->resolution,
> +			/*
> +			 * Buffer elements could be 16-bit for low precision
> +			 * parts. Though, using more storage bits allows keeping
> +			 * the same scan_type configuration for both types of
> +			 * buffer support.
> +			 */

Won't this make non-SPI offload buffered reads more complicated later?
I.e. have to shift the value or not before pushing to buffers depending
on CPU endianness.

> +			.storagebits = 32,
> +		},
> +	};
> +
> +	st->xfer.rx_buf = &st->scan.data;
> +	st->xfer.len = st->info->resolution > 16 ? 4 : 2;

Can use spi_bpw_to_bytes() here.

> +	st->xfer.bits_per_word = st->info->resolution;
> +
> +	indio_dev->channels = st->chans;
> +	indio_dev->num_channels = st->num_iio_chans;
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +

...

> diff --git a/drivers/iio/adc/ltc2378.h b/drivers/iio/adc/ltc2378.h
> new file mode 100644
> index 000000000000..a3a69351de6c
> --- /dev/null
> +++ b/drivers/iio/adc/ltc2378.h
> @@ -0,0 +1,63 @@
> +/* 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@analog.com>
> + */
> +
> +#ifndef __DRIVERS_IIO_ADC_LTC2378_H__
> +#define __DRIVERS_IIO_ADC_LTC2378_H__
> +
> +#include <linux/iio/iio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.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 bipolar;
> +};
> +
> +struct ltc2378_state {
> +	const struct ltc2378_chip_info *info;
> +	struct gpio_desc *cnv_gpio;
> +	struct spi_device *spi;
> +	struct spi_transfer xfer;
> +	unsigned int num_iio_chans;
> +	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);
> +};
> +
> +static inline 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);
> +	ret = spi_sync_transfer(st->spi, &st->xfer, 1);
> +	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> +
> +	return ret;
> +}
> +
> +#endif /* __DRIVERS_IIO_ADC_LTC2378_H__ */

Why do we need a header for this stuff? If there is a good reason
the commit message should explain it. Otherwise, it makes the driver
harder to read.