On 20/06/26 11:33, David Lechner wrote:
> On 6/16/26 3:21 AM, Rodrigo Alencar via B4 Relay 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.
...
> > +/**
> > + * struct ad5686_spi_data - SPI bus specific data
> > + * @msg: SPI message used for transfers
> > + * @size: number of transfers currently in the message
> > + * @capacity: maximum number of transfers that can be added to the message
> > + * @xfers: array of SPI transfers, allocated with the provided capacity
> > + */
> > +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);
>
> Seems odd that spi_message_init() is called conditionally. What
> prevents spi_message_add_tail() from growing the message unbounded
> on repeated calls?
The "size >= capacity" check right above this.
>
> > +
> > + xfer = &bus_data->xfers[bus_data->size];
> > 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];
> > - tx_len = 2;
> > + st->data[bus_data->size].d16 =
> > + cpu_to_be16(AD5310_CMD(cmd) | val);
> > + *xfer = (struct spi_transfer) {
> > + .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 = (struct spi_transfer) {
> > + .tx_buf = &st->data[bus_data->size].d8[1],
> > + .len = sizeof(st->data[bus_data->size].d32) - 1,
> > + };
> > 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 = (struct spi_transfer) {
> > + .tx_buf = &st->data[bus_data->size].d8[1],
> > + .len = sizeof(st->data[bus_data->size].d32) - 1,
> > + };
> > break;
> > default:
> > return -EINVAL;
> > }
> >
> > - return spi_write(spi, buf, tx_len);
>
> If this function no longer writes, should we change the name of
> the function to something like ad5686_spi_write_prepare_msg()?
>
> > + 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 +117,21 @@ 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] = (struct spi_transfer) {
> > + .tx_buf = &st->data[0].d8[1],
> > + .len = sizeof(st->data[0].d32) - 1,
>
> Would make more sense to say `sizeof(st->data[0].d8) - 1` since
> the buffer is &st->data[0].d8[1].
the buffer is d8[1], d8[2] and d8[3], skipping d8[0], for a len = 3.
> > + .cs_change = 1,
> > + };
> > + xfer[1] = (struct spi_transfer) {
> > + .tx_buf = &st->data[1].d8[1],
> > + .rx_buf = &st->data[2].d8[1],
> > + .len = sizeof(st->data[1].d32) - 1,
>
> And here.
>
> > + };
> > +
> > + 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 +140,26 @@ 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,
> > };
> >
--
Kind regards,
Rodrigo Alencar
