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

[Daniel Thompson]

On Wed, Oct 14, 2026 at 03:41:20PM -0500, Alex Elder wrote:
> On 10/14/26 12:36 PM, Andrew Lunn wrote:
> > On Wed, Oct 14, 2026 at 12:22:59PM -0500, Alex Elder wrote:
> > > From: Daniel Thompson <daniel@xxxxxxxxxxxx>
> > >
> > > QCA8081 appears in embedded board designs paired with GPIO controlled
> > > regulators for its power rails. Add logic to allow these regulators to
> > > be turned on during a probe.
> > >
> > > In order to avoid the complexity of tracking state for suspend with and
> > > without WoL we take a tremendously simple "always-on" approach to
> > > regulator management, essentially relying on BMCR_PDOWN to conserve
> > > power when the phy device exists.
> > >
> > > Signed-off-by: Daniel Thompson <daniel@xxxxxxxxxxxx>
> > > Co-developed-by: Alex Elder <elder@xxxxxxxxxxxx>
> > > Signed-off-by: Alex Elder <elder@xxxxxxxxxxxx>
> > > ---
> > >   drivers/net/phy/qcom/qca808x.c | 30 ++++++++++++++++++++++++++++++
> > >   1 file changed, 30 insertions(+)
> > >
> > > diff --git a/drivers/net/phy/qcom/qca808x.c b/drivers/net/phy/qcom/qca808x.c
> > > index bab26e4c61408..6e2bd753257d9 100644
> > > --- a/drivers/net/phy/qcom/qca808x.c
> > > +++ b/drivers/net/phy/qcom/qca808x.c
> > > @@ -1,6 +1,7 @@
> > >   // SPDX-License-Identifier: GPL-2.0+
> > >   #include <linux/phy.h>
> > > +#include <linux/regulator/consumer.h>
> > >   #include <linux/module.h>
> > >   #include "qcom.h"
> > > @@ -181,15 +182,44 @@ 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)
> > > +{
> > > +#ifdef CONFIG_OF
> >
> > Does this need to be conditional? It seems like
> > devm_regulator_get_enable_optional() should return ENODEV, which you
> > already handle.
> >
> > Making code this like conditional makes build testing less useful,
> > more combinations which need testing.
>
> This is a really good point.
>
> > Another option is
> >
> > 	if (IS_ENABLED(CONFIG_OF)) {
> >
> >
> > 	}
> >
> > That at least compiles the code, and then the optimizier throws it
> > away when it sees  if (0) { }
>
> Yes I like doing it this way better.  I'll modify it
> to be similar to this for v2, which I'll plan to send
> tomorrow.

Reading things back, I think Andrew's first suggestion of having no
conditional might be better.

devm_regulator_get_enable_optional() is log-silent on failure. I suspect
the reason this code was conditional is that an early version used
devm_regulator_get_enable() which is not log-silent when it falls back
to a dummy reg.

I suspect I was more influenced by the log-silence than the marginal
code size cost on a CONFIG_REGULATOR && !CONFIG_OF system.


Daniel.