Re: [PATCH 09/12] iio: dac: ad5686: implement new sync() op for the spi bus

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 02 Jun 2026 17:33:56 +0100
Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:

> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> Use of local SPI bus data to manage a collection of SPI transfers and
> flush them to the SPI platform driver with the sync() operation. This
> allows for faster handling of multiple channel DAC writes, avoiding kernel
> overhead per spi_sync() call, which will be helpful when enabling
> triggered buffer support.
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
Some minor stuff inline.  Maybe it does make sense to adapt for a generic
solution in the spi core, maybe not but most of these comments apply anyway!

Jonathan

> ---
>  drivers/iio/dac/ad5686-spi.c | 109 +++++++++++++++++++++++++++++++------------
>  drivers/iio/dac/ad5686.c     |   4 +-
>  drivers/iio/dac/ad5686.h     |   8 +++-
>  drivers/iio/dac/ad5696-i2c.c |   2 +-
>  4 files changed, 88 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5686-spi.c b/drivers/iio/dac/ad5686-spi.c
> index 6b6ef1d7071f..66a5a2164395 100644
> --- a/drivers/iio/dac/ad5686-spi.c
> +++ b/drivers/iio/dac/ad5686-spi.c
> @@ -12,59 +12,81 @@
>  #include <linux/errno.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> +#include <linux/overflow.h>
>  #include <linux/spi/spi.h>
>  
>  #include <asm/byteorder.h>
>  
>  #include "ad5686.h"
>  
> +struct ad5686_spi_data {
> +	struct spi_message msg;
> +	unsigned int size;

Not obvious to me what size is from the naming. Maybe this
structure needs some documentation.

> +	unsigned int capacity;
> +	struct spi_transfer xfers[] __counted_by(capacity);
> +};
> +
>  static int ad5686_spi_write(struct ad5686_state *st,
>  			    u8 cmd, u8 addr, u16 val)
>  {
> -	struct spi_device *spi = to_spi_device(st->dev);
> -	u8 tx_len, *buf;
> +	struct ad5686_spi_data *bus_data = st->bus_data;
> +	struct spi_transfer *xfer;
> +
> +	if (bus_data->size >= bus_data->capacity)
> +		return -E2BIG;
> +
> +	if (bus_data->size)
> +		bus_data->xfers[bus_data->size - 1].cs_change = 1;
> +	else
> +		spi_message_init(&bus_data->msg);
> +
> +	xfer = &bus_data->xfers[bus_data->size];
> +	xfer->rx_buf = NULL;
> +	xfer->cs_change = 0;

These are both 'resets' to defaults and you are heavily relying
on other fields in that rather complex structure never being set.
Maybe initializing all the fields is simpler?  

>  
>  	switch (st->chip_info->regmap_type) {
>  	case AD5310_REGMAP:
> -		st->data[0].d16 = cpu_to_be16(AD5310_CMD(cmd) |
> -					      val);
> -		buf = &st->data[0].d8[0];
(why not use d16 here? Obviously this is original code, but applies
below)
> -		tx_len = 2;
> +		st->data[bus_data->size].d16 =
> +			cpu_to_be16(AD5310_CMD(cmd) | val);
> +		xfer->tx_buf = &st->data[bus_data->size].d8[0];

> +		xfer->len = 2;
Following on from above on resetting xfer fields.

		*xfer = (struct spi_transfers) {
			.tx_buf = &st->data[bus_data->size].d16, 
			.len = sizeof(&st->data[bus_data->size.d16),
		};
>  		break;
>  	case AD5683_REGMAP:
> -		st->data[0].d32 = cpu_to_be32(AD5686_CMD(cmd) |
> -					      AD5683_DATA(val));
> -		buf = &st->data[0].d8[1];
> -		tx_len = 3;
> +		st->data[bus_data->size].d32 =
> +			cpu_to_be32(AD5686_CMD(cmd) | AD5683_DATA(val));
> +		xfer->tx_buf = &st->data[bus_data->size].d8[1];
> +		xfer->len = 3;
Similar use of designated initializer at small cost of clearing stuff
that is clear.  I doubt that matters.

>  		break;
>  	case AD5686_REGMAP:
> -		st->data[0].d32 = cpu_to_be32(AD5686_CMD(cmd) |
> -					      AD5686_ADDR(addr) |
> -					      val);
> -		buf = &st->data[0].d8[1];
> -		tx_len = 3;
> +		st->data[bus_data->size].d32 =
> +			cpu_to_be32(AD5686_CMD(cmd) | AD5686_ADDR(addr) | val);
> +		xfer->tx_buf = &st->data[bus_data->size].d8[1];
> +		xfer->len = 3;
same again. 
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
> -	return spi_write(spi, buf, tx_len);
> +	spi_message_add_tail(xfer, &bus_data->msg);
> +	bus_data->size++;
> +
> +	return 0;
> +}
> +
> +static int ad5686_spi_sync(struct ad5686_state *st)
> +{
> +	struct spi_device *spi = to_spi_device(st->dev);
> +	struct ad5686_spi_data *bus_data = st->bus_data;
> +
> +	bus_data->size = 0; /* always reset, even on sync failure */
> +	return spi_sync(spi, &bus_data->msg);
>  }
>  
>  static int ad5686_spi_read(struct ad5686_state *st, u8 addr)
>  {
> -	struct spi_transfer t[] = {
> -		{
> -			.tx_buf = &st->data[0].d8[1],
> -			.len = 3,
> -			.cs_change = 1,
> -		}, {
> -			.tx_buf = &st->data[1].d8[1],
> -			.rx_buf = &st->data[2].d8[1],
> -			.len = 3,
> -		},
> -	};
>  	struct spi_device *spi = to_spi_device(st->dev);
> +	struct ad5686_spi_data *bus_data = st->bus_data;
> +	struct spi_transfer *xfer = &bus_data->xfers[0];
>  	u8 cmd = 0;
>  	int ret;
>  
> @@ -85,8 +107,18 @@ static int ad5686_spi_read(struct ad5686_state *st, u8 addr)
>  				      AD5686_ADDR(addr));
>  	st->data[1].d32 = cpu_to_be32(AD5686_CMD(AD5686_CMD_NOOP));
>  
> -	ret = spi_sync_transfer(spi, t, ARRAY_SIZE(t));
> -	if (ret < 0)
> +	xfer[0].tx_buf = &st->data[0].d8[1];
> +	xfer[0].len = 3;
> +	xfer[0].cs_change = 1;
> +	xfer[1].tx_buf = &st->data[1].d8[1];
> +	xfer[1].rx_buf = &st->data[2].d8[1];
> +	xfer[1].len = 3;
> +	xfer[1].cs_change = 0;

Similar to above - I'd initialize the lot as suggested up there.
Saves on effort thinking about it. Maybe the cost matters but I doubt it.


> +
> +	spi_message_init_with_transfers(&bus_data->msg, xfer, 2);
> +
> +	ret = spi_sync(spi, &bus_data->msg);
> +	if (ret)
>  		return ret;
>  
>  	return be32_to_cpu(st->data[2].d32);
> @@ -95,12 +127,27 @@ static int ad5686_spi_read(struct ad5686_state *st, u8 addr)
>  static const struct ad5686_bus_ops ad5686_spi_ops = {
>  	.write = ad5686_spi_write,
>  	.read = ad5686_spi_read,
> +	.sync = ad5686_spi_sync,
>  };
>  
>  static int ad5686_spi_probe(struct spi_device *spi)
>  {
> -	return ad5686_probe(&spi->dev, spi_get_device_match_data(spi),
> -			    spi->modalias, &ad5686_spi_ops);
> +	const struct ad5686_chip_info *info = spi_get_device_match_data(spi);
> +	struct ad5686_spi_data *bus_data;
> +	unsigned int capacity;
> +
> +	/* read operation requires at least 2 transfers */
> +	capacity = max(info->num_channels, 2);
> +	bus_data = devm_kzalloc(&spi->dev,
> +				struct_size(bus_data, xfers, capacity),
> +				GFP_KERNEL);
> +	if (!bus_data)
> +		return -ENOMEM;
> +
> +	bus_data->capacity = capacity;
> +
> +	return ad5686_probe(&spi->dev, info, spi->modalias, &ad5686_spi_ops,
> +			    bus_data);
>  }
>  
>  static const struct spi_device_id ad5686_spi_id[] = {