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

[Torreno, Alexis Czezar]

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

This was memcpy before, was changed to this for consistency as per Andy's
suggestion. It was during v5.

Removal of this plus the other mem* commands allowed removal of string.h
in the headers.

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

Back in v3-v4, Joathan suggested this to be spi_write_then_read() as it was much safer,
but I requested to keep it this way since I would be adding more features to this driver
like changing spi_speeds, which cannot be done if using spi_write_then_read
 
> 
> > +}
> > +
> > +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.

Technically it's a don't care data, zero just makes it cleaner signals during debug

Discussed spi_write_then_read in a comment above

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

as discussed above about mem* and spi_write_then_read