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

[Torreno, Alexis Czezar]

> > +#define AD5706R_DAC_RESOLUTION		16
> > +#define AD5706R_DAC_MAX_CODE		BIT(16)
> 
> Trivial but I'd expect something called MAX_CODE to be GENMASK(15, 0) not
> BIT(16). E.g. inclusive limit.

Since it's dac code I guess this makes better sense, will edit.

> 
> > +#define AD5706R_MULTIBYTE_REG_START	0x14
> > +#define AD5706R_MULTIBYTE_REG_END	0x71
> > +#define AD5706R_MAX_REG			0x77
> > +#define AD5706R_SINGLE_BYTE_LEN		1
> > +#define AD5706R_DOUBLE_BYTE_LEN		2
> 
> See below. I'm not sure these two defines bring us anything.
> 
> > +
> > +struct ad5706r_state {
> > +	struct spi_device *spi;
> > +	struct regmap *regmap;
> > +
> > +	u8 tx_buf[4] __aligned(IIO_DMA_MINALIGN);
> > +	u8 rx_buf[4];
> > +};
> > +
> > +static int ad5706r_reg_len(unsigned int reg) {
> > +	if (reg >= AD5706R_MULTIBYTE_REG_START && reg <=
> AD5706R_MULTIBYTE_REG_END)
> > +		return AD5706R_DOUBLE_BYTE_LEN;
> 
> What do the defines for 2 == 2-bytes and 1 == 1-byte bring us over using
> numbers directly?  E.g.
> 
> 	if (reg >= AD5706R_MULTIBYTE_REG_START && reg <=
> AD5706R_MULTIBYTE_REG_END)
> 		return 2;
> 	return 1;
> 

I suppose not much, it won't change in the future so a hard "1" or "2" can work.
Will remove the defines and just put the actual number

> > +
> > +	return AD5706R_SINGLE_BYTE_LEN;
> > +}
> > +
> > +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]);
> > +
> > +	/* 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];
> 
> This does feel a bit odd vs using if / else if as you do in the read case.  Also,
> same as above wrt to single bytes having a length of
> 1 meaning that just using a 1 might be easier to read.

Unlike in the read, I thought it would be simpler to copy the whole data of 4bytes,
then just correct the buffer during single_byte.
Should I make it similarly coded with the read()?

> 
> > +
> > +	return spi_sync_transfer(st->spi, &xfer, 1); }
> 
> > +static int ad5706r_write_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int val, int val2, long mask) {
> > +	struct ad5706r_state *st = iio_priv(indio_dev);
> > +	unsigned int reg;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		if (!in_range(val, 0, AD5706R_DAC_MAX_CODE))
> 
> I'm not seeing a strong reason to use in_range() here (hopefully I didn't
> suggest it in an earlier review ;)  It make sense when we have a  val >= base &&
> val < base + length. With base as 0 and MAX_CODE not 'obviously' from it's
> name being the length (it only is becauset he base is 0) this seems odd.
> 
> 		if (val < 0 || val >= AD5706R_DAC_MAX_CODE) Though see
> above on MAX_CODE not being the maximum code...
> 

I think around v3 Andy suggested the use of in_range, as the function itself helps
document what the line does. Is this a style preference?

Regards,
Alexis