Re: [PATCH v3 6/6] iio: adc: ad7768-1: add support for multiple chip aggregation

[Andy Shevchenko]

On Mon, Aug 17, 2026 at 08:33:41PM -0300, Jonathan Santos wrote:
> The AD7768-1 family is a single-channel ADC, but it is designed to allow
> connecting multiple devices to the same SPI controller, each on a
> dedicated CS and data lane, clocked synchronously, and sharing SDO and
> SCLK. The number of aggregated devices are derived from
> spi->num_rx_lanes, assuming all parts are single lane.
> 
> The DRDY pins are combined to trigger the data interrupt when all
> devices are ready, and since the synchronization pins are tied, they
> stay in synchrony.
> 
> To reflect the multidevice setup, IIO channels are dynamically set
> based on the number of devices. Restrict buffered capture to the
> all-channels scan mask, since all devices sample in lockstep. Register
> an ancillary SPI device per lane for individual direct reads, and enable
> SPI_MULTI_LANE_MODE_STRIPE in offload mode to interleave samples from all
> lanes into the DMA stream.
> 
> Since all devices must be in sync, all configurations that affects the
> sampling rate are unified, so they always have the same sampling
> frequency.

...

> +	struct spi_device *spi_anc[AD7768_MAX_AGGR_DEVICES];

> +	struct regmap *regmap24_anc[AD7768_MAX_AGGR_DEVICES];

Yeah, this will be enormous memory consumption...

...

> +static int ad7768_probe_multidevices(struct iio_dev *indio_dev)
> +{
> +	struct ad7768_state *st = iio_priv(indio_dev);
> +	struct device *dev = indio_dev->dev.parent;
> +	struct iio_chan_spec *channels;
> +	unsigned long *masks;
> +	u8 cs;
> +	int i;

Why is 'i' signed?

> +	indio_dev->num_channels = st->num_devices * st->chip->num_channels;

This wants to have size_mul() from overflow.h to follow what allocator is using
beneath.

> +	channels = devm_kcalloc(dev, indio_dev->num_channels, sizeof(*channels), GFP_KERNEL);
> +	if (!channels)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < st->num_devices; i++) {
> +		struct iio_chan_spec *chan = &channels[i];
> +
> +		*chan = *st->chip->channel_spec;
> +		chan->channel = i;
> +		chan->scan_index = i;
> +	}
> +
> +	indio_dev->channels = channels;
> +
> +	/* One mask entry, considering single channel ADCs, plus a zero terminator */
> +	masks = devm_kcalloc(dev, 2, sizeof(*masks), GFP_KERNEL);
> +	if (!masks)
> +		return -ENOMEM;

Isn't this devm_bitmap_zalloc()? Yes, it might require to reconsider the design
of the masks.

> +	masks[0] = GENMASK(st->num_devices - 1, 0);

bitmap_set() (needs bitmap.h)

> +	indio_dev->available_scan_masks = masks;
> +
> +	/* Setup ancillary SPI devices for single device access  */
> +	for (i = 0; i < st->num_devices; i++) {
> +		cs = spi_get_chipselect(st->spi, i);
> +		st->spi_anc[i] = devm_spi_new_ancillary_device_with_lane(st->spi,
> +									 cs, i, 0);
> +		if (IS_ERR(st->spi_anc[i]))
> +			return dev_err_probe(dev, PTR_ERR(st->spi_anc[i]),
> +					     "failed to register ancillary device\n");
> +
> +		st->regmap24_anc[i] = devm_regmap_init_spi(st->spi_anc[i],
> +							   &ad7768_regmap24_config);
> +		if (IS_ERR(st->regmap24_anc[i]))
> +			return PTR_ERR(st->regmap24_anc[i]);
> +	}
> +
> +	return 0;
> +}

...

> +	if (st->num_devices > 1) {
> +		ret = ad7768_probe_multidevices(indio_dev);
> +		if (ret)
> +			return dev_err_probe(&spi->dev, ret,

Having

	struct device *dev = &spi->dev;

may help here and elsewhere to make code neater.

> +					     "Failed to configure multidevice\n");
> +	} else {
> +		indio_dev->channels = st->chip->channel_spec;
> +		indio_dev->num_channels = st->chip->num_channels;
> +	}

-- 
With Best Regards,
Andy Shevchenko