Re: [PATCH net-next 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] Applying power to the PHY with its reset line already deasserted violates the DP83867 hardware initialization sequence.
- [Medium] Unconditional 200ms boot delay per PHY on systems without regulator support (CONFIG_REGULATOR=n) or with already-enabled regulators.
--

commit c4d914d3e47d5ead4a045bf92dab75bb76e60c07
Author: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

net: phy: dp83867: add regulator supply management

This commit adds regulator supply management to the DP83867 PHY driver,
enabling four supply domains at probe time. It also introduces a 200ms
sleep after powering on the supplies to satisfy the stabilization requirement
from the datasheet.

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

[ ... ]

> @@ -719,9 +720,40 @@ static int dp83867_resume(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int dp83867_power_on(struct phy_device *phydev)
> +{
> +	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: Medium]
Does this increment count even when devm_regulator_get_enable_optional()
returns 0 from the dummy API (for instance, when CONFIG_REGULATOR is not set),
or when the supplies were already enabled by the bootloader?

If so, could this cause an unintended and cumulative 200ms boot delay per PHY
on systems without regulator support, as count will be positive even if no
regulator actually changed state?

> +		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);
> +
> +	return 0;
> +}
> +
>  static int dp83867_probe(struct phy_device *phydev)
>  {
>  	struct dp83867_private *dp83867;
> +	int ret;
> +
> +	ret = dp83867_power_on(phydev);

[Severity: High]
Does calling dp83867_power_on() from dp83867_probe() result in power
being applied while the hardware reset line is already deasserted?

The core PHY framework unconditionally deasserts the hardware reset before the
driver's probe callback is executed:

phy_probe()
  phy_device_reset(phydev, 0)
  ...
  dp83867_probe()
    dp83867_power_on()

Applying power to the DP83867 PHY while the RESET_N pin is deasserted (high)
violates the power-up sequence defined in section 6.6 of the datasheet, which
can lead to erratic behavior or initialization failure.

Should the driver assert phy_device_reset(phydev, 1) to hold the chip in
reset during the power ramp before enabling the regulators?

> +	if (ret)
> +		return ret;
>  
>  	dp83867 = devm_kzalloc(&phydev->mdio.dev, sizeof(*dp83867),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-shikra_ethernet-v1-0-a50765996035@oss.qualcomm.com?part=2