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

[Andy Shevchenko]

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.

...

> +config AD5706R
> +	tristate "Analog Devices AD5706R DAC driver"
> +	depends on SPI

Shouldn't you select REGMAP?

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

...

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

> +	u8 rx_buf[4];
> +};

...

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

> +	u16 reg;
> +
> +	reg = get_unaligned_be16(data);
> +	num_bytes = ad5706r_reg_len(reg);
> +
> +	struct spi_transfer xfer = {
> +		.tx_buf = st->tx_buf,
> +		.len = num_bytes + 2,
> +	};
> +
> +	memcpy(st->tx_buf, data, count);
> +
> +	/* For single byte, copy the data to the correct position */
> +	if (num_bytes == AD5706R_SINGLE_BYTE_LEN)
> +		st->tx_buf[2] = st->tx_buf[3];
> +
> +	return spi_sync_transfer(st->spi, &xfer, 1);
> +}
> +
> +static int ad5706r_regmap_read(void *context, const void *reg_buf,
> +			       size_t reg_size, void *val_buf, size_t val_size)
> +{
> +	struct ad5706r_state *st = context;
> +	unsigned int num_bytes;
> +	u16 reg, cmd;
> +	int ret;
> +
> +	reg = get_unaligned_be16(reg_buf);
> +	num_bytes = ad5706r_reg_len(reg);
> +
> +	/* Full duplex, device responds immediately after command */
> +	struct spi_transfer xfer = {
> +		.tx_buf = st->tx_buf,
> +		.rx_buf = st->rx_buf,
> +		.len = 2 + num_bytes,
> +	};
> +
> +	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_*().

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

> +	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_*()?

> +	return 0;
> +}

...

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

-- 
With Best Regards,
Andy Shevchenko