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

[Torreno, Alexis Czezar]

> > +
> > +struct ad5706r_state {
> > +	struct spi_device *spi;
> > +	struct mutex lock; /* Protects SPI transfers */
> I assume it actually protects the buffers below. Where possible talk about what
> 'data' is being protected. It's easier to reason about than transfers which will be
> protected anyway by internal bus driver locking.

Ah yes, I'll edit the comment to something like

struct mutex lock; /* Protects tx_buf and rx_buf from concurrent access */

> 
> > +
> > +	ret = spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer));
> 
> Can you use spi_write_the_read()?
> Has the added advantage that it bounces the data so doesn't need DMA safe.
> Can also use more meaningful types like
> __be16 tx;
> __be16 rx16;
> u8 rx8;

Ok, will shift to spi_write_the_read(). There was another place to use this above,
just trimmed it off here. Noted on the more meaningful variables.

 ...

> 
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		scoped_guard(mutex, &st->lock) {
> Why?
> 
> 	case IIO_CHAN_INFO_RAW: {
> 		guard(mutex)(&st->lock);
> 
> 		ret = ....
> 
> 		return IIO_VAL_INT;
> 	}
> 
> is easier to read and reduces the huge indent.
> 
> Only use scoped_guard() when you can't do it in a more readable fashion.

Will edit to simpler guard()

> 

...

> > +	}
> > +
> I'd prefer an explicit
> 	defualt:
> 		return -EINVAL;
> 	}
> to end the switch statement and make it clear within the switch that all other
> options
> are errors.

Wil add the default

> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		if (val < 0 || val >= AD5706R_DAC_MAX_CODE)
> > +			return -EINVAL;
> > +
> > +		guard(mutex)(&st->lock);
> 
> What's the scope? add {} to the case block to make it clearly defined.

Just the raw, I'll add the {}

> > +static const struct of_device_id ad5706r_of_match[] = {
> > +	{ .compatible = "adi,ad5706r" },
> > +	{}
> 
> As below.
> 
> > +};
> > +MODULE_DEVICE_TABLE(of, ad5706r_of_match);
> > +
> > +static const struct spi_device_id ad5706r_id[] = {
> > +	{ "ad5706r" },
> > +	{}
> 
> Trivial style thing, but we've standardized on
> 	{ }
> for IIO.  Had to pick one of the two choices and that one looked
> nicer to me ;)
> 

Nice, I did wonder if either style was ok as long as consistent, noted on the {}

Regards,
Alexis