Re: [PATCH v12 4/6] iio: adc: ad4691: add SPI offload support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Invalid field '.format' in struct iio_scan_type initializer.
- [Low] Comment claims scan_tx_reset length differs between paths, but code uses length 4 in both.
--

commit 7490ce5c597707ce10e439240d92b62db62380e0
Author: Radu Sabau <radu.sabau@analog.com>

iio: adc: ad4691: add SPI offload support

Add SPI offload support to enable DMA-based, CPU-independent data
acquisition using the SPI Engine offload framework.

> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> index ed60ed5b488c2..a6588292f3c18 100644
> --- a/drivers/iio/adc/ad4691.c
> +++ b/drivers/iio/adc/ad4691.c
[ ... ]
> @@ -139,6 +150,30 @@ struct ad4691_chip_info {
>  		},							\
>  	}
>  
> +/*
> + * Offload path (bits_per_word=16): the SPI Engine assembles received
> + * bits into native 16-bit words before DMA, so samples are in
> + * CPU-native byte order (IIO_CPU). storagebits=16 matches the 16-bit
> + * DMA word size.
> + */
> +#define AD4691_OFFLOAD_CHANNEL(ch)					\
> +	{								\
> +		.type = IIO_VOLTAGE,					\
> +		.indexed = 1,						\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)		\
> +				    | BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +		.info_mask_separate_available =				\
> +				      BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),	\
> +		.channel = ch,						\
> +		.scan_index = ch,					\
> +		.scan_type = {						\
> +			.format = 'u',					\

Does this cause a build failure? It looks like the member in struct
iio_scan_type is called sign, not format. Initializing .format here
will likely cause the compiler to complain about a missing member.

> +			.realbits = 16,					\
> +			.storagebits = 16,				\
> +		},							\
> +	}
> +
[ ... ]
> @@ -876,6 +1005,218 @@ static const struct iio_buffer_setup_ops ad4691_cnv_burst_buffer_setup_ops = {
[ ... ]
> +static int ad4691_cnv_burst_offload_buffer_postenable(struct iio_dev *indio_dev)
> +{
[ ... ]
> +	/*
> +	 * State reset: single 4-byte write [addr_hi, addr_lo, STATE_RESET_ALL,
> +	 * OSC_EN=1]. ADDR_DESCENDING writes byte[3]=1 to OSC_EN_REG (0x180) as
> +	 * a deliberate side-write, keeping the oscillator enabled.
> +	 * scan_tx_reset is shared with the non-offload path (len=4 here vs
> +	 * len=3 there) since the two paths are mutually exclusive at probe.
> +	 */

Is this comment accurate regarding the lengths?

The non-offload path in ad4691_cnv_burst_buffer_preenable() also appears
to use a 4-byte transfer for scan_tx_reset, setting up all 4 bytes before
passing it to the transfer.

> +	put_unaligned_be16(AD4691_STATE_RESET_REG, st->scan_tx_reset);
> +	st->scan_tx_reset[2] = AD4691_STATE_RESET_ALL;
> +	st->scan_tx_reset[3] = 1;
> +	st->scan_xfers[2 * k].tx_buf = st->scan_tx_reset;
> +	st->scan_xfers[2 * k].len = sizeof(st->scan_tx_reset);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-ad4692-multichannel-sar-adc-driver-v12-0-5b335162aa51@analog.com?part=4