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

[Torreno, Alexis Czezar]

> On Wed, Apr 01, 2026 at 06:20:04PM +0800, Alexis Czezar Torreno wrote:
> > Add support for the Analog Devices AD5706R, a 4-channel 16-bit current
> > output digital-to-analog converter with SPI interface.
> >
> > Features:
> >   - 4 independent DAC channels
> >   - Hardware and software LDAC trigger
> >   - Configurable output range
> >   - PWM-based LDAC control
> >   - Dither and toggle modes
> >   - Dynamically configurable SPI speed
> 
> ...
> 
> > ---
> > Changes since v1:
> >   - Removed PWM, GPIO, clock generator, debugfs, regmap, IIO_BUFFER
> >   - Removed all custom ext_info sysfs attributes
> >   - Simplified to basic raw read/write and read-only scale
> >   - SPI read/write can handle multibyte registers
> > ---
> 
> A bit confusing to have this changelog w/o having v3..v4 ones.

Will add the others

> 
> ...
> 
> > +config AD5706R
> > +	tristate "Analog Devices AD5706R DAC driver"
> > +	depends on SPI
> 
> Shouldn't you select REGMAP?

Oh yeah, I should've. Will add

> 
> > +	help
> > +	  Say yes here to build support for Analog Devices AD5706R 4-channel,
> > +	  16-bit current output DAC.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called ad5706r.
> 
> ...
> 
> > +#include <linux/array_size.h>
> > +#include <linux/bits.h>
> 
> > +#include <linux/device.h>
> 
> Not used, but dev_printk.h is missing.
> 
> > +#include <linux/dma-mapping.h>
> > +#include <linux/err.h>
> 
> > +#include <linux/errno.h>
> 
> No need (in most cases) when err.h is included.
> 
> > +#include <linux/iio/iio.h>
> > +#include <linux/minmax.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <linux/unaligned.h>
> 
> Based on the above comments, please revisit the header block.

Will do, thanks for the suggestions above.

> 
> ...
> 
> > +struct ad5706r_state {
> > +	struct spi_device *spi;
> > +	struct regmap *regmap;
> > +
> > +	u8 tx_buf[4] __aligned(ARCH_DMA_MINALIGN);
> 
> Don't we have specific IIO macro for that?

Ah yeah I think it was IIO_DMA_MINALIGN, will change, will also remove the
header where ARCH_DMA_MINALIGN comes from

> 
> > +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

> 
> ...
> 
> > +static int ad5706r_read_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan, int *val,
> > +			    int *val2, long mask)
> 
> Better to use logical split (here and elsewhere where appropriate)
> 
> static int ad5706r_read_raw(struct iio_dev *indio_dev,
> 			    struct iio_chan_spec const *chan,
> 			    int *val, int *val2, long mask)
> 
> ...
> 
> > +	st->regmap = devm_regmap_init(&spi->dev, &ad5706r_regmap_bus,
> > +				      st, &ad5706r_regmap_config);
> 
> Use
> 
> 	struct device *dev = &spi->dev;
> 
> at the top of the function to make this look better.
> 
> > +	if (IS_ERR(st->regmap))
> > +		return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
> > +				     "Failed to init regmap");
> 
> Missing \n.
> 

Will apply the three minor edits above