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

[David Lechner]

On 4/10/26 1:48 AM, 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.
> 

...

> +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;
> +
> +	if (count != 4)
> +		return -EINVAL;
> +
> +	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);
> +	put_unaligned_be32(val, &st->tx_buf[0]);

Can't we just do memcpy() instead of swapping the byte order twice?

> +
> +	/* 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);

There isn't any special paramters in the xfer struct, so spi_write() should
work here and save a bit of code.

	return spi_write(st->spi, data, num_bytes);


> +}
> +
> +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;
> +
> +	if (reg_size != 2 || val_size != 2)
> +		return -EINVAL;
> +
> +	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[0]);
> +	put_unaligned_be16(0, &st->tx_buf[2]);

Do we actually need to write 0s while reading? 

Usually, we would just do a spi_write_then_read for something like
this.

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

Can't this all be simplified to memcpy(val_buf, &st->rx_buf[2], num_bytes); ?

Or the whole thing simplified to:

	return spi_write_then_read(st->spi, reg_buf, 2, val_buf, num_bytes);

> +
> +	return 0;
> +}
> +