Re: [PATCH v2 2/3] iio: dac: Add AD5529R DAC driver support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Incorrect SPI `read_flag_mask` causes all regmap reads to fail.
- [High] Incomplete hardware reset sequence fails to properly reset the device.
- [High] Hardcoded output scale ignores device configuration and internal reference.
- [High] Missing LDAC (Load DAC) trigger prevents analog output updates.
--

commit 02e32fb72beab3fdbff7c500ce586fab5c255e89
Author: Janani Sunil <janani.sunil@analog.com>

iio: dac: Add AD5529R DAC driver support

Add support for AD5529R 16-channel, 12/16 bit Digital to Analog Converter

> diff --git a/drivers/iio/dac/ad5529r.c b/drivers/iio/dac/ad5529r.c
> new file mode 100644
> index 0000000000000..3676956f6eff7
> --- /dev/null
> +++ b/drivers/iio/dac/ad5529r.c
> @@ -0,0 +1,564 @@
[ ... ]
> +#define   AD5529R_RESET_DELAY_US		10000
> +#define   AD5529R_SPI_BUF_SIZE			4
> +#define   AD5529R_NUM_SUPPLIES			4
> +#define   AD5529R_SPI_READ_FLAG			0x80
[ ... ]
> +static const struct regmap_config ad5529r_regmap_8bit_config = {
> +	.name = "ad5529r-8bit",
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +	.max_register = AD5529R_8BIT_REG_MAX,
> +	.read_flag_mask = AD5529R_SPI_READ_FLAG,

Is this mask correct for a 16-bit register address? Since reg_bits is set
to 16 in the regmap configuration, setting read_flag_mask to 0x80 will
set bit 7 instead of the most-significant bit (bit 15). Should this be
0x8000 so the device properly interprets read commands?

> +	.rd_table = &ad5529r_8bit_readable_table,
> +	.wr_table = &ad5529r_8bit_writeable_table,
> +};
[ ... ]
> +static int ad5529r_reset(struct ad5529r_state *st)
> +{
> +	struct reset_control *rst;
> +	int ret;
> +
> +	rst = devm_reset_control_get_optional_exclusive(&st->spi->dev, NULL);
> +	if (IS_ERR(rst))
> +		return PTR_ERR(rst);
> +
> +	if (rst) {
> +		ret = reset_control_deassert(rst);

Will this properly reset the device if the reset line was already left in
a deasserted state by the bootloader? There is an unused
AD5529R_RESET_PULSE_US macro defined earlier. Does the driver need to
call reset_control_assert() and wait before calling
reset_control_deassert() to ensure the hardware sees a valid reset pulse?

> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = regmap_write(st->regmap_8bit, AD5529R_REG_INTERFACE_CONFIG_A,
[ ... ]
> +static int ad5529r_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
[ ... ]
> +	case IIO_CHAN_INFO_SCALE:
> +		/*
> +		 * Using default 0-5V range: VOUTn = A × D/2^N + B
> +		 * where A = 5V, B = 0V, D = digital code, N = resolution
> +		 * Scale = 5000mV / 2^resolution
> +		 */
> +		*val = 5000;

The commit message mentions the device features multiple output voltage
ranges from -20V to +20V and a built-in 4.096V reference. However, the
scale is hardcoded to assume a 0-5V range.

Will this report incorrect physical voltages to userspace if the device
defaults to a different range, uses the internal reference, or was
configured for a bipolar range by the bootloader?

> +		*val2 = st->model_data->resolution;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad5529r_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct ad5529r_state *st = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (val < 0 || val > GENMASK(st->model_data->resolution - 1, 0))
> +			return -EINVAL;
> +
> +		return regmap_write(st->regmap_16bit, AD5529R_REG_DAC_INPUT_A(chan->channel), val);

Does this code actually trigger an update to the physical analog output?

The value is written to the DAC_INPUT_A register, but the driver does not
appear to issue a software LDAC command or pulse a hardware LDAC GPIO.
If the device doesn't default to a transparent mode, is an explicit LDAC
trigger needed here?

> +	default:
> +		return -EINVAL;
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-ad5529r-driver-v2-0-e315441685d7@analog.com?part=2