Re: [PATCH net-next v2 2/9] net: phy: dp83867: add regulator supply management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect usage of devm_regulator_get_enable_optional() causes unconditional 200ms boot delays.
- [Low] The `#ifdef CONFIG_OF` guard improperly restricts regulator management to device-tree platforms.
--

commit 6d9389913bb995393df4699729d85576835ff23d
Author: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
net: phy: dp83867: add regulator supply management

This commit adds regulator supply management during probe for the DP83867
PHY. It enables required power rails and sleeps 200ms if any supply was
enabled to meet datasheet requirements.

> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index 88255e92b4cdb..dbeee7cad6f0c 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c

[ ... ]

> @@ -719,9 +720,41 @@ static int dp83867_resume(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int dp83867_power_on(struct phy_device *phydev)
> +{
> +#ifdef CONFIG_OF

[Severity: Low]
Does this restrict the regulator management to device tree platforms
unnecessarily?

The Linux regulator subsystem is firmware-agnostic and safely returns
-ENODEV when supplies are not found via standard mappings, so wrapping this
in an OF-specific guard appears to skip the initialization for platforms
using other configurations like ACPI or board files.

> +	static const char * const supply_names[] = {
> +		"vdda-2p5", "vdd-1p0", "vdda-1p8", "vddio",
> +	};
> +	struct device *dev = &phydev->mdio.dev;
> +	u32 count = 0;
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(supply_names); i++) {
> +		ret = devm_regulator_get_enable_optional(dev, supply_names[i]);
> +		if (!ret)
> +			count++;

[Severity: High]
Does this logic accurately detect newly-enabled regulators?

If CONFIG_REGULATOR is disabled, the dummy implementation of
devm_regulator_get_enable_optional() unconditionally returns 0, which would
falsely increment count to 4.

Furthermore, even if regulator support is enabled, it returns 0 for
regulators that are already enabled (e.g. shared supplies or boot-on
regulators). This contradicts the commit message's claim that it only sleeps
when a supply is "newly enabled".

Would it be better to open-code devm_regulator_get_optional() and check
regulator_is_enabled() before enabling the supply? That way the driver can
accurately detect if it was just turned on, avoiding an unconditional 200ms
boot delay per PHY.

> +		else if (ret != -ENODEV)
> +			return dev_err_probe(dev, ret,
> +					     "failed to enable %s supply\n",
> +					     supply_names[i]);
> +	}
> +
> +	/* Datasheet section 6.6 suggests a 200ms post power-up stabilization */
> +	if (count)
> +		fsleep(200000);
> +#endif
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-shikra_ethernet-v2-0-bbe3389d0652@oss.qualcomm.com?part=2