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

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

On Tue, Apr 07, 2026 at 11:39:45AM +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

Mostly okay, see minor comments below, the main one is about treating void * as
__be16/__be32 * without any validation.

...

> +static int ad5706r_regmap_write(void *context, const void *data, size_t count)
> +{
> +	struct ad5706r_state *st = context;
> +	unsigned int num_bytes, val;
> +	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,
> +	};

> +	val = get_unaligned_be32(data);

Is it safe? The data is void *, no size of it is counted here...

> +	put_unaligned_be32(val, st->tx_buf);
> +
> +	/* 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, val;
> +	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);

For the consistency's sake use &st->tx_buf[0].

> +	put_unaligned_be16(0, &st->tx_buf[2]);
> +
> +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> +	if (ret)
> +		return ret;
> +
> +	/* Extract value from response (skip 2-byte command echo) */
> +	if (num_bytes == AD5706R_SINGLE_BYTE_LEN)
> +		val = st->rx_buf[2];
> +	else if (num_bytes == AD5706R_DOUBLE_BYTE_LEN)
> +		val = get_unaligned_be16(&st->rx_buf[2]);
> +	else
> +		return -EINVAL;
> +
> +	put_unaligned_be16(val, val_buf);
> +
> +	return 0;
> +}

...

> +#define AD5706R_CHAN(_channel) {				\
> +	.type = IIO_CURRENT,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> +			      BIT(IIO_CHAN_INFO_SCALE),	\

Missing indentation at the end with tabs.

> +	.output = 1,						\
> +	.indexed = 1,						\
> +	.channel = _channel,					\
> +}

-- 
With Best Regards,
Andy Shevchenko