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

[Andy Shevchenko]

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?

>   - 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.

...

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

> +			ret = ad5706r_spi_read(st, AD5706R_REG_DAC_DATA_READBACK_CH(chan->channel),

It's too long line.

> +					       &reg_val);
> +
> +			if (ret)
> +				return ret;
> +
> +			*val = reg_val;
> +		}
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 50;
> +		*val2 = AD5706R_DAC_RESOLUTION;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	}
> +
> +	return -EINVAL;
> +}

...

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

> +			return -EINVAL;
> +
> +		guard(mutex)(&st->lock);
> +		return ad5706r_spi_write(st,
> +					 AD5706R_REG_DAC_INPUT_A_CH(chan->channel),
> +					 val);
> +	default:
> +		return -EINVAL;
> +	}
> +}

-- 
With Best Regards,
Andy Shevchenko