Re: [PATCH v4 5/6] iio: dac: ad5504: strictly separate ACPI and DT probe paths

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Bypassing the regulator framework on ACPI platforms breaks PRP0001-based configurations.
- [Low] Missing inclusion of `<linux/acpi.h>` for `ACPI_COMPANION`.
--

--- Patch [5]: [PATCH v4 5/6] iio: dac: ad5504: strictly separate ACPI and DT probe paths ---
commit 6ac93c8870295bc478f5b446654ca1771bdebc78
Author: Taha Ed-Dafili <[email protected]>

iio: dac: ad5504: strictly separate ACPI and DT probe paths

Refactor the ad5504_probe() function to explicitly separate the ACPI
and Device Tree execution paths.

> diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
> index 7b52ec95150de..e124e7804f3fb 100644
> --- a/drivers/iio/dac/ad5504.c
> +++ b/drivers/iio/dac/ad5504.c
> @@ -19,6 +19,7 @@
>  #include <linux/spi/spi.h>
>  #include <linux/sysfs.h>
>  #include <linux/types.h>
> +#include <linux/units.h>

[Severity: Low]
This isn't a bug, but should <linux/acpi.h> be explicitly included here to
satisfy the 'Include What You Use' (IWYU) principle for the ACPI_COMPANION
macro used below? Building under configurations where it is not transitively
included could lead to a build failure.

>  #include <linux/iio/events.h>
>  #include <linux/iio/iio.h>

[ ... ]

> @@ -300,11 +305,16 @@ static int ad5504_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> -	ret = devm_regulator_get_enable_read_voltage(dev, "vcc");
> -	if (ret < 0)
> -		return ret;
> +	if (ACPI_COMPANION(dev)) {

[Severity: Medium]
Does bypassing the regulator framework on ACPI platforms break PRP0001-based
configurations?

On ACPI systems that use standard PRP0001 (Device Tree bindings wrapped in
ACPI) to provide a vcc regulator, this unconditional branch skips enabling
the regulator and reading its voltage. The device might remain unpowered or
use an incorrect hardcoded reference voltage.

Before this patch, ad5504_probe() attempted to fetch the regulator and fell
back to defaults only if it returned -ENODEV. Should the regulator fetch
be preserved for ACPI platforms to avoid defeating the unified device
property frameworks?

> +		st->vref_mv = AD5504_VA_MV_ACPI_DEFAULT;
> +	} else {
> +		ret = devm_regulator_get_enable_read_voltage(dev, "vcc");
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to get vcc regulator\n");
>  
> -	st->vref_mv = ret / 1000;
> +		st->vref_mv = ret / MILLI;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5