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

Jonathan Cameron
2026-04-23T18:18:44Z

On Wed, 22 Apr 2026 15:45:54 +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>
Interesting approach to potentially optimise for SPI. Do you have
perf numbers or similar to support it being worth the effort?

Otherwise a few minor comments inline.

Thanks,

J
> ---
> drivers/iio/dac/ad5686-spi.c | 110 +++++++++++++++++++++++++++++++------------
> drivers/iio/dac/ad5686.c | 4 +-
> drivers/iio/dac/ad5686.h | 8 +++-
> drivers/iio/dac/ad5696-i2c.c | 2 +-
> 4 files changed, 89 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/iio/dac/ad5686-spi.c b/drivers/iio/dac/ad5686-spi.c
> index ebfc40efa679..bacfb1deab31 100644
> --- a/drivers/iio/dac/ad5686-spi.c
> +++ b/drivers/iio/dac/ad5686-spi.c
> @@ -13,57 +13,80 @@
> #include <linux/err.h>
> #include <linux/mod_devicetable.h>
> #include <linux/module.h>
> +#include <linux/overflow.h>
> #include <linux/spi/spi.h>
>
> #include "ad5686.h"
>
> +struct ad5686_spi_data {
> + struct spi_message msg;
> + unsigned int size;
> + 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;
>
> switch (st->chip_info->regmap_type) {
> case AD5310_REGMAP:
> - st->data[0].d16 = cpu_to_be16(AD5310_CMD(cmd) |
> - val);
Fits on oneline.

> - buf = &st->data[0].d8[0];
> - tx_len = 2;
> + st->data[bus_data->size].d16 = cpu_to_be16(AD5310_CMD(cmd) |
> + val);

Even these I'd put on one line to improve readability, or split as:
st->data[bus_data->size].d16 =
cpu_to_be16(...)

> + xfer->tx_buf = &st->data[bus_data->size].d8[0];
> + xfer->len = 2;
> 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;
> 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;
> 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_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;
>
> @@ -84,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;
> +
> + spi_message_init_with_transfers(&bus_data->msg, xfer, 2);

Why not carry on using spi_sync_transfer() here?
We'll end up with an spi message the stack but I suspect that's
not a significant performance cost.

> +
> + ret = spi_sync(spi, &bus_data->msg);
> + if (ret)
> return ret;
>
> return be32_to_cpu(st->data[2].d32);