Re: [PATCH v4 2/2] iio: dac: ad5706r: Add support for AD5706R DAC

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Apr 06, 2026 at 07:04:44AM +0000, Torreno, Alexis Czezar wrote:
> > On Wed, Apr 01, 2026 at 06:20:04PM +0800, Alexis Czezar Torreno wrote:

...

> > > +static int ad5706r_regmap_write(void *context, const void *data,
> > > +size_t count) {
> > > +	struct ad5706r_state *st = context;
> > > +	unsigned int num_bytes;
> > 
> > Currently only 1 and 2 bytes are supported, right? Any updates are planned on
> > this in the future?
> 
> Yes only 1 and 2 bytes, no future extension. Should I make num_bytes a 'u8'?
> 
> > > +	u16 reg;

> > > +	cmd = AD5706R_RD_MASK | (reg & AD5706R_ADDR_MASK);
> > > +	put_unaligned_be16(cmd, st->tx_buf);
> > 
> > > +	memset(st->tx_buf + 2, 0, num_bytes);
> > 
> > I would use &st->tx_buf[2] here and below for the sake of consistency with
> > put_unaligned_*().
> 
> Will edit for consistnency.
> 
> > > +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* Ignore the first two bytes (echo during command) */
> > > +	if (num_bytes == AD5706R_SINGLE_BYTE_LEN)
> > > +		put_unaligned_be16(st->rx_buf[2], val_buf);
> > 
> > The comment wants to explain why it's required to put 2 bytes anyway.
> 
> Will add clearer comments for this
> 
> > > +	else
> > > +		memcpy(val_buf, st->rx_buf + 2, num_bytes);
> > 
> > However with the above question in mind, if it's all about 1 or 2 bytes, can't we
> > simply use the same approach everywhere, like put_unaligned_*()?
> 
> For consistency, put_unaligned_*() can work. 
> 
> Although since rx_buf is u8, this line:
> 
>      memcpy(val_buf, &st->rx_buf[2], num_bytes);
> 
> Will look like this for 2 bytes:
> 
>     x = get_unaligned_be16( &st->rx_buf[2] )
>     put_unaligned_be16( x, val_buf )
> 
> I suppose the mem* commands looks cleaner

We optimise it a bit, so what you are going to have is something like

	if (num_bytes == 1)
		x = &st->rx_buf[2];
	else if (num_bytes == 2)
		x = get_unaligned...(...);
	else
		return -EINVAL;

	put_unaligned...(...);

-- 
With Best Regards,
Andy Shevchenko