Re: [PATCH v3 5/5] iio: dac: ad5504: fix scale via output-range-microvolt

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] UAPI/ABI Breakage: existing Device Trees rely on the 'vcc' regulator to configure the IIO scale, but the driver now silently defaults to 60V.
- [Medium] Silent error suppression for malformed 'output-range-microvolt' device tree properties.
- [Low] Incomplete removal of legacy platform data header.
--

commit b02770a220cb671b4745934fb6d03abfd8d88fd3
Author: Taha Ed-Dafili <0rayn.dev@gmail.com>

iio: dac: ad5504: fix scale via output-range-microvolt

This patch updates the AD5504 driver to read the output-range-microvolt DT
property for setting the scale, rather than querying the vcc regulator. It
also removes legacy pdata support and sets the default scale to 60V if the
property is absent.

> diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
> index 9e95da6e49d61..040f580b82828 100644
> --- a/drivers/iio/dac/ad5504.c
> +++ b/drivers/iio/dac/ad5504.c
> @@ -14,10 +14,12 @@
>  #include <linux/kstrtox.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> +#include <linux/property.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/spi/spi.h>
>  #include <linux/sysfs.h>
>  #include <linux/types.h>
> +#include <linux/units.h>
>  
>  #include <linux/iio/dac/ad5504.h>

The commit message states legacy pdata support is removed. Is it intentional
to leave the #include <linux/iio/dac/ad5504.h> header in place, as it appears
to only contain the unused legacy structs?

[ ... ]

> @@ -285,16 +287,19 @@ static int ad5504_probe(struct spi_device *spi)
>  
>  	st = iio_priv(indio_dev);
>  
> -	ret = devm_regulator_get_enable_read_voltage(dev, "vcc");
> -	if (ret < 0 && ret != -ENODEV)
> +	ret = devm_regulator_get_enable(dev, "vcc");
> +	if (ret && ret != -ENODEV)
>  		return ret;
> -	if (ret == -ENODEV) {
> -		if (pdata->vref_mv)
> -			st->vref_mv = pdata->vref_mv;
> -		else
> -			dev_warn(dev, "reference voltage unspecified\n");
> -	} else {
> -		st->vref_mv = ret / 1000;
> +
> +	st->vref_mv = 60 * MILLI;

Does dropping the regulator voltage read cause compatibility issues with
existing device trees?

Prior to this patch, existing setups could provide a vcc regulator with a
30V voltage, which would properly configure st->vref_mv. By defaulting
directly to 60V when the new output-range-microvolt property is absent, older
device trees might silently change their exposed scale attribute to userspace.

Would it be safer to fallback to reading the regulator voltage and issue a
deprecation warning if the new property is missing?

> +	ret = device_property_read_u32_array(dev, "output-range-microvolt",
> +					     range, ARRAY_SIZE(range));
> +	if (!ret) {
> +		if (range[0] != 0 || (range[1] != 30 * MICRO && range[1] != 60 * MICRO))
> +			return -EINVAL;
> +
> +		if (range[1] == 30 * MICRO)
> +			st->vref_mv = 30 * MILLI;
>  	}

If the device tree property exists but is malformed, such as only containing
a single value instead of an array, device_property_read_u32_array() might
return an error code like -EOVERFLOW or -ENODATA.

Since the code only checks if (!ret), does this silently suppress parsing
errors and assume the 60V default?

Should this explicitly check for ret && ret != -EINVAL and return the error
code instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509142047.30302-1-0rayn.dev@gmail.com?part=5