On Fri, 24 Apr 2026 09:58:39 +0100 Rodrigo Alencar <[email protected]> wrote:
> On 26/04/23 07:18PM, Jonathan Cameron wrote: > > On Wed, 22 Apr 2026 15:45:54 +0100 > > Rodrigo Alencar via B4 Relay > > <[email protected]> wrote: > > > > > From: Rodrigo Alencar <[email protected]> > > > > > > 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 <[email protected]> > > 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); >
