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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iio: adc: ad7768-1: incomplete multi-device read in ad7768_trigger_handler()
--

commit 093b4a79a74e19badb41a08df17fd753e9cf37ec
Author: Jonathan Santos <Jonathan.Santos@analog.com>

iio: adc: ad7768-1: add support for multiple chip aggregation

This commit adds support for aggregating multiple ad7768-1 devices connected
to the same spi controller using dedicated chip selects and data lanes. It
dynamically configures the channels, registers per-lane ancillary devices for
direct reads, and enables striped multi-lane transfers in offload mode.

> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 3972821c295a1..667c09326f92f 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
[ ... ]
> @@ -335,7 +341,7 @@ struct ad7768_state {
>  	 */
>  	union {
>  		struct {
> -			__be32 chan;
> +			__be32 chan[AD7768_MAX_AGGR_DEVICES];
>  			aligned_s64 timestamp;
>  		} scan;
>  		__be32 d32;

[Severity: High]
While struct ad7768_state expands st->data.scan.chan[] for non-offload
triggered buffer capture, does ad7768_trigger_handler() in
drivers/iio/adc/ad7768-1.c also need to be updated when multiple devices are
aggregated?

When st->num_devices > 1 and spi offload is not present, ad7768_probe() still
falls back to registering the triggered buffer path via
ad7768_triggered_buffer_alloc():

drivers/iio/adc/ad7768-1.c:ad7768_probe() {
	...
	st->offload = devm_spi_offload_get(dev, spi, &ad7768_spi_offload_config);
	ret = PTR_ERR_OR_ZERO(st->offload);
	if (ret == -ENODEV) {
		/* If not using SPI offload, fall back to low speed usage */
		ret = ad7768_triggered_buffer_alloc(indio_dev);
	...
}

However, when buffered capture is enabled and the interrupt fires,
ad7768_trigger_handler() still performs a single-lane, single-sample read:

drivers/iio/adc/ad7768-1.c:ad7768_trigger_handler() {
	...
	ret = spi_read(st->spi, &st->data.scan.chan,
		       BITS_TO_BYTES(scan_type->realbits));
	if (ret < 0)
		goto out;

	iio_push_to_buffers_with_ts(indio_dev, &st->data.scan,
				    sizeof(st->data.scan),
				    iio_get_time_ns(indio_dev));
	...
}

Because spi_read() only reads a single sample of scan_type->realbits bits on
one lane into the start of st->data.scan.chan[0] while asserting all chip
selects on st->spi, the samples from devices 1 through st->num_devices - 1
are clocked out and lost on the bus, pushing unpopulated channel data to the
buffer.

Furthermore, with multiple channels enabled, a 3-byte read in 24-bit mode
(realbits = 24, storagebits = 32) or a 2-byte read in 16-bit mode
(st->oversampling_ratio == 8, realbits = 16, storagebits = 16) does not match
the 4-byte __be32 element stride of st->data.scan.chan[].

Should ad7768_trigger_handler() be updated to read and pack all
st->num_devices channels according to scan_type->storagebits, or is
non-offload buffered capture unsupported when st->num_devices > 1?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792435248.git.Jonathan.Santos@analog.com?part=9