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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 9 Jul 2026 17:49:53 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> wrote:

> Support for LTC2378-20 and similar analog-to-digital converters.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> ---
> Change log v5 -> v6:
> - Added comment to clarify CPU endianness is used when device native BE is not.
> - Protected single sample read procedure with a mutex.
> - Dropped mod_devicetable.h, include device-id/spi.h device-id/of.h instead.
> - Used iwyu to add missing #includes and dropped superfluous ones.
> - Used pahole to minimize memory holes in LTC2378 data structures.

A small comment about maybe splitting the channel macro into offload and
non offload variants. That would also let you only bring the offload
version in as part of the next patch.

Jonathan

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

> +#define LTC2378_DIFF_CHANNEL(_sign, _real_bits, _storage_bits, _offload)	\
> +{										\
> +	.type = IIO_VOLTAGE,							\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |				\
> +			      BIT(IIO_CHAN_INFO_SCALE) |			\
> +			      (_offload ? BIT(IIO_CHAN_INFO_SAMP_FREQ) : 0),	\
> +	.info_mask_separate_available = _offload ? BIT(IIO_CHAN_INFO_SAMP_FREQ) : 0,\
> +	.scan_index = 0,							\
> +	.scan_type = {								\
> +		.format = _sign ? IIO_SCAN_FORMAT_SIGNED_INT :			\
> +				  IIO_SCAN_FORMAT_UNSIGNED_INT,			\
> +		.realbits = _real_bits,						\
> +		.storagebits = _storage_bits,					\
> +		.shift = (_offload ? 0 : _storage_bits - _real_bits),		\
> +		.endianness = _offload ? IIO_CPU : IIO_BE			\

As below. I think, in my ideal world the offload parts of this would have
been in patch 3.

Given how much changes in here to me it's a bit marginal on whether having
LTC2378_DIFF_CHANNEL() and LTC2378_DIFF_CHANNEL_OFFLOAD() would have been simpler.



> +	},									\
> +}

> +
> +struct ltc2378_state {
> +	const struct ltc2378_chip_info *info;
> +	struct gpio_desc *cnv_gpio;
> +	struct spi_device *spi;
> +	struct mutex lock; /* Protect data acquisition cycle */
> +	int ref_uV;
> +	struct spi_transfer xfer;
> +
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the
> +	 * transfer buffers to live in their own cache lines.

Trivial but I'd wrap nearer 80 chars.

> +	 */
> +	struct {
> +		union {
> +			__be16 sample_buf16_be;
> +			__be32 sample_buf32_be;
> +			u16 sample_buf16;
> +			u32 sample_buf32;
I guess for churn reduction it is fine to introduce some stuff for
offload before the patch that does the majority of it.. 

> +		} data;
> +		aligned_s64 timestamp;
> +	} scan __aligned(IIO_DMA_MINALIGN);
> +};



> +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;
Maybe
		return 0;
as done
> +	else if (ret < 0)
	if (ret < 0)
		return
> +		return dev_err_probe(dev, ret, "failed to read refin regulator\n");
> +	else

and then no else needed?  It's a marginal improvement so I don't mind
if you ignore this suggestion.

> +		st->ref_uV = ret * 2;
> +
> +	return 0;
> +}