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

[Torreno, Alexis Czezar]

> On Wed, Mar 18, 2026 at 01:13:36PM +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
> 
> 
> > ---
> > Changes since v1:
> >   - Removed PWM, GPIO, clock generator, debugfs, regmap, IIO_BUFFER
> 
> Why was regmap removed?! Was it not used?

As far as I understand it, regmap also gives access to debugfs. When I removed
debugfs I also added regmap as removed.

For the spi write/read I am not using regmap as the device has some features
that I think regmap_read/write couldn't support. Namely the variable data width,
as the device only accepts exact amount of clock cycles. Future patches will also add
variable SPI speed.

> 
> >   - Removed all custom ext_info sysfs attributes
> >   - Simplified to basic raw read/write and read-only scale
> >   - SPI read/write can handle multibyte registers
> 
> ...
> 
> > +#include <linux/array_size.h>
> > +#include <linux/bits.h>
> > +#include <linux/cleanup.h>
> 
> + errno.h
> 
> > +#include <linux/iio/iio.h>
> 
> + mod_devicetable.h
> 
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/spi/spi.h>
> 
> + types.h
> 
> > +#include <linux/unaligned.h>
> 
> Follow IWYU principle.

I did miss errno.h but it seems the IWYU is stricter than pragmatic. 
Will adhere to it better.

> 
> ...
> 
> > +static int ad5706r_read_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);
> > +	u16 reg_val;
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		scoped_guard(mutex, &st->lock) {
> 
> 
> Can't it be simply guard()() ?

Can be, yes, will edit.

> 
> > +			ret = ad5706r_spi_read(st,
> > +AD5706R_REG_DAC_DATA_READBACK_CH(chan->channel),
> 
> It's too long line.

Will simplify this and other similar lines

> 

 ...

> 
> > +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);
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		if (val < 0 || val >= AD5706R_DAC_MAX_CODE)
> 
> in_range()?
> 
> (will need minmax.h)
> 

Will replace, much more readable that way

> > +			return -EINVAL;
> > +
> > +		guard(mutex)(&st->lock);
> > +		return ad5706r_spi_write(st,
> > +