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

Rodrigo Alencar
2026-04-24T08:58:47Z

On 26/04/23 07:18PM, Jonathan Cameron wrote:
> 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?

Short answer is no, I can try to produce some benchmarks, but I am not sure it
is needed. The implementation here prepares the ground for triggered buffer support,
which adds the capability of implementing a control loop in userspace that updates
all the enabled channels at once. That will happen in a trigger handler, then we want
that to be as fast as possible, so it can be prepared for the next trigger event.
It is not really about calling a function once rather than multiple times, but the
problem happens when the device is sharing the spi bus with other devices and how busy the
system is in general, so there may be context switches in the spi platform driver with
one or two mutex acquire/release operations, which may add bigger delays between
transfers, which depends on what the other devices are doing. I suppose that devices with
high channel count like 8 or 16 can benefit with this.

> Otherwise a few minor comments inline.

...

> > @@ -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.

True, it would be around 200 extra bytes in the stack, and read operations
would not happen concurrently with writes, so why not reuse resources
(message and transfer structs) that is now available in the bus data?
To me, it allows for consistency in this implementation.

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

--
Kind regards,

Rodrigo Alencar