Re: [PATCH net-next v2 2/2] net: phy: qcom: qca808x: Add regulator management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: phy: qca808x: missing PHY reset pulse after enabling regulators
--

commit e2b47da72c4cf5c697d4c2078b3f0f943b751123
Author: Daniel Thompson <daniel@riscstar.com>

net: phy: qcom: qca808x: Add regulator management

This commit adds logic to power on GPIO-controlled regulators during the
probe of the QCA8081 PHY device. It uses an always-on approach, turning on
regulators if present and sleeping for the reset assert delay.

> diff --git a/drivers/net/phy/qcom/qca808x.c b/drivers/net/phy/qcom/qca808x.c
> index bab26e4c61408..4aba513b9a512 100644
> --- a/drivers/net/phy/qcom/qca808x.c
> +++ b/drivers/net/phy/qcom/qca808x.c

[ ... ]

> @@ -181,15 +182,43 @@ static void qca808x_fill_possible_interfaces(struct phy_device *phydev)
>  		__set_bit(PHY_INTERFACE_MODE_2500BASEX, possible);
>  }
>  
> +static int qca808x_power_on(struct phy_device *phydev)
> +{
> +	static const char * const regulator_names[] = {
> +		"avdd18", "vdd", "vdd18", "vdd125"
> +	};
> +	struct device *dev = &phydev->mdio.dev;
> +	u32 count = 0;
> +	int ret;
> +
> +	for (u32 i = 0; i < ARRAY_SIZE(regulator_names); i++) {
> +		ret = devm_regulator_get_enable_optional(dev, regulator_names[i]);
> +		if (!ret)
> +			count++;
> +		else if (ret != -ENODEV)
> +			return ret;
> +	}
> +
> +	if (count)
> +		fsleep(phydev->mdio.reset_assert_delay);

[Severity: High]
Does this sequence successfully initialize the PHY hardware? The driver applies
power here and sleeps for the reset delay, but doesn't seem to explicitly
toggle the reset line.

Since the MDIO core deasserts the reset line before invoking the driver's
probe() callback, the PHY appears to power up with the reset line already
deasserted. Could this regression result in the hardware failing to initialize
properly and leaving a dead link?

Perhaps an explicit assert and deassert (e.g., via phy_device_reset()) is
needed here to provide a valid reset pulse after the power is stable.

> +
> +	return 0;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015143849.483339-1-elder@riscstar.com?part=2