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

[Andrew Davis]

On 9/7/26 3:23 PM, Mohd Ayaan Anwar wrote:
> Some embedded board designs use GPIO-controlled regulators for the
> DP83867 power rails. Add dp83867_power_on() to enable all four supply
> domains at probe time. Absent supplies are silently skipped, so boards
> that do not describe them are unaffected.
>
> When any supply is newly enabled the driver sleeps for 200 ms before
> returning.  This satisfies the post power-up stabilisation requirement
> mentioned in section 6.6 of the DP83867E/IS/CS datasheet.
>
> Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@xxxxxxxxxxxxxxxx>
> ---
>   drivers/net/phy/dp83867.c | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
>
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index 88255e92b4cdbd6da2e2c1d10f9c72348d28dbc3..dbeee7cad6f0cbfeb127bfd28f43ee2a16c78879 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -15,6 +15,7 @@
>   #include <linux/etherdevice.h>
>   #include <linux/bitfield.h>
>   #include <linux/nvmem-consumer.h>
> +#include <linux/regulator/consumer.h>
>   
>   #include <dt-bindings/net/ti-dp83867.h>
>   
> @@ -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
> +	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]);

Would be nice to have an API for getting all regulators without needing to
manually list all of their names, since we don't really care about them
individually. The of_regulator_bulk_get_all() is close but it would need
a "get_enable" variant, plus _optional, then made into a devm_ version.. :)

Well until such a thing comes along this looks fine,

Acked-by: Andrew Davis <afd@xxxxxx>

> +		if (!ret)
> +			count++;
> +		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;
> +}
> +
>   static int dp83867_probe(struct phy_device *phydev)
>   {
>   	struct dp83867_private *dp83867;
> +	int ret;
> +
> +	ret = dp83867_power_on(phydev);
> +	if (ret)
> +		return ret;
>   
>   	dp83867 = devm_kzalloc(&phydev->mdio.dev, sizeof(*dp83867),
>   			       GFP_KERNEL);