Re: [PATCH v7 4/7] iio: dac: ad5686: implement new sync() op for the spi bus

On 11/07/26 10:07, David Lechner wrote:
> On 7/10/26 6:20 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.

...

> >  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;
> >  
> 
> This could use some comments similar to the explanation in the
> commit message that this function is just queuing writes to
> be sent over the bus when sync() is called.
> 
> > +	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);
> > +
> > +	xfer = &bus_data->xfers[bus_data->size];
> 
> Why not a local variable for st->data[bus_data->size] too so we don't
> have to write it so many times?

The union is anonymous.. is it fine to use:

	typeof(st->data[0]) *buf;
	...
	buf = &st->data[bus_data->size];

> 
> >  	switch (st->chip_info->regmap_type) {
> >  	case AD5310_REGMAP:
> > -		st->data[0].d16 = cpu_to_be16(FIELD_PREP(AD5310_CMD_MSK, cmd) |
> > -					      FIELD_PREP(AD5310_DATA_MSK, val));
> > -		buf = &st->data[0].d8[0];
> > -		tx_len = 2;
> > +		st->data[bus_data->size].d16 =
> > +			cpu_to_be16(FIELD_PREP(AD5310_CMD_MSK, cmd) |
> > +				    FIELD_PREP(AD5310_DATA_MSK, 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(FIELD_PREP(AD5686_CMD_MSK, cmd) |
> > -					      FIELD_PREP(AD5683_DATA_MSK, val));
> > -		buf = &st->data[0].d8[1];
> > -		tx_len = 3;
> > +		st->data[bus_data->size].d32 =
> > +			cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, cmd) |
> > +				    FIELD_PREP(AD5683_DATA_MSK, val));
> > +		*xfer = (struct spi_transfer) {
> > +			.tx_buf = &st->data[bus_data->size].d8[1],
> > +			.len = sizeof(st->data[bus_data->size].d8) - 1,
> > +		};
> >  		break;
> >  	case AD5686_REGMAP:
> > -		st->data[0].d32 = cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, cmd) |
> > -					      FIELD_PREP(AD5686_ADDR_MSK, addr) |
> > -					      FIELD_PREP(AD5686_DATA_MSK, val));
> > -		buf = &st->data[0].d8[1];
> > -		tx_len = 3;
> > +		st->data[bus_data->size].d32 =
> > +			cpu_to_be32(FIELD_PREP(AD5686_CMD_MSK, cmd) |
> > +				    FIELD_PREP(AD5686_ADDR_MSK, addr) |
> > +				    FIELD_PREP(AD5686_DATA_MSK, val));
> > +		*xfer = (struct spi_transfer) {
> > +			.tx_buf = &st->data[bus_data->size].d8[1],
> > +			.len = sizeof(st->data[bus_data->size].d8) - 1,
> > +		};
> >  		break;
> >  	default:
> >  		return -EINVAL;
> >  	}

...

-- 
Kind regards,

Rodrigo Alencar