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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 24 Apr 2026 09:58:39 +0100
Rodrigo Alencar <455.rodrigo.alencar@gmail.com> wrote:

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

Fair enough.  There is enough complexity here that maybe a perf
number would strengthen the argument.  I'm reasonably happy with
that complexity anyway so this would be if anyone else pushed back.

> 
> > 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. 
> 
Reusing transfers makes sense.  I was a little less clear on reusing the
message because we can't then use the helper (which is all spi_sync_transfer()
really is).  Not particularly important either way.
> > > +
> > > +	ret = spi_sync(spi, &bus_data->msg);
> > > +	if (ret)
> > >  		return ret;
> > >  
> > >  	return be32_to_cpu(st->data[2].d32);  
>