On 6/22/26 3:50 AM, Rodrigo Alencar wrote:
> 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.
OK, but it could still grow to capacity, adding one xfer each
time this function is called with bus_data->size > 0, right?
>
>>
>>> +
>>> + 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.
Yes, I see that. This is why I suggested that it should be based on
sizeof(...d8) - 1 rather than sizeof(...d32) - 1. It makes more sense
to use the same field for both the size and the buffer pointer.
>
>>> + .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,
>>> };
>>>
>
