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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The AD5529R Kconfig symbol selects `REGMAP_SPI` but incorrectly depends on `SPI_MASTER` (a boolean) instead of `SPI` (a tristate), which can cause unmet direct dependency warnings and linker errors.
- [Medium] The driver enables the `vref` regulator but does not query its voltage, using hardcoded scaling values that assume a 4.096V reference.
--

--- Patch [3]: [PATCH v9 3/3] iio: dac: Add AD5529R DAC driver support ---
commit 93be93e4756accead91864ac70303ad8ccc9e4da
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
from Analog Devices.

> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 657c68e75542e..6a7109f42f2a3 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -134,6 +134,22 @@ config AD5449
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad5449.
>  
> +config AD5529R
> +	tristate "Analog Devices AD5529R High Voltage DAC driver"
> +	depends on SPI_MASTER
> +	select REGMAP_SPI

[Severity: High]
Should this depend on SPI rather than SPI_MASTER?

If SPI is built as a module (=m), SPI_MASTER evaluates to y. This allows
AD5529R to be configured as built-in (=y), which forces REGMAP_SPI to y
through the select statement.

Since REGMAP_SPI depends on SPI, this can produce a Kconfig unmet direct
dependency warning and cause linker errors for SPI core functions during
randconfig testing.

[ ... ]

> diff --git a/drivers/iio/dac/ad5529r.c b/drivers/iio/dac/ad5529r.c
> new file mode 100644
> index 0000000000000..6982840232681
> --- /dev/null
> +++ b/drivers/iio/dac/ad5529r.c

[ ... ]

> +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:
> +		range_idx = st->output_range_idx[chan->channel];
> +
> +		/*
> +		 * The datasheet specifies a 4.096 V external reference,
> +		 * matching the nominal output voltage of the internal
> +		 * reference.
> +		 */
> +		span_mv = ad5529r_output_ranges_mV[range_idx][1] -
> +			  ad5529r_output_ranges_mV[range_idx][0];
> +		*val = span_mv;

[ ... ]

> +static int ad5529r_probe(struct spi_device *spi)
> +{

[ ... ]

> +	ret = devm_regulator_get_enable_optional(dev, "vref");

[Severity: Medium]
Is it possible to query the actual regulator voltage here rather than using
devm_regulator_get_enable_optional()?

Since this helper doesn't return the regulator pointer, the driver cannot read
the actually supplied voltage. Looking at ad5529r_read_raw() above, the IIO
scale is calculated using hardcoded millivolt arrays that assume the external
reference is exactly 4.096V.

If a board provides a different reference voltage, will the IIO scaling
metadata exposed to userspace be incorrect?

> +	if (ret == -ENODEV)
> +		external_vref = false;
> +	else if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get and enable vref regulator\n");
> +	else
> +		external_vref = true;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820-ad5529r-driver-v9-0-ba62e0b2a816@analog.com?part=3