diff mbox series

[3/6] net: stmmac: sun8i: Use devm_regulator_get for PHY regulator

Message ID 20190820145343.29108-4-megous@megous.com
State Changes Requested
Delegated to: David Miller
Headers show
Series Add ethernet support for Orange Pi 3 | expand

Commit Message

Ondřej Jirman Aug. 20, 2019, 2:53 p.m. UTC 
From: Ondrej Jirman <megous@megous.com>

Use devm_regulator_get instead of devm_regulator_get_optional and rely
on dummy supply. This avoids NULL checks before regulator_enable/disable
calls.

This path also improves error reporting, because we now report both
use of dummy supply and error during registration with more detail,
instead of generic info level message "No regulator found" that
was reported previously on errors and lack of regulator property in DT.

Finally, we'll be adding further optional regulators, and the overall
code will be simpler.

Signed-off-by: Ondrej Jirman <megous@megous.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 23 ++++++++-----------
 1 file changed, 10 insertions(+), 13 deletions(-)

Comments

Andrew Lunn Aug. 20, 2019, 3:39 p.m. UTC | #1 
On Tue, Aug 20, 2019 at 04:53:40PM +0200, megous@megous.com wrote:
> From: Ondrej Jirman <megous@megous.com>
> 
> Use devm_regulator_get instead of devm_regulator_get_optional and rely
> on dummy supply. This avoids NULL checks before regulator_enable/disable
> calls.

Hi Ondrej

What do you mean by a dummy supply? I'm just trying to make sure you
are not breaking backwards compatibility.

     Thanks
	Andrew
Ondřej Jirman Aug. 20, 2019, 3:47 p.m. UTC | #2 
Hi Andrew,

On Tue, Aug 20, 2019 at 05:39:39PM +0200, Andrew Lunn wrote:
> On Tue, Aug 20, 2019 at 04:53:40PM +0200, megous@megous.com wrote:
> > From: Ondrej Jirman <megous@megous.com>
> > 
> > Use devm_regulator_get instead of devm_regulator_get_optional and rely
> > on dummy supply. This avoids NULL checks before regulator_enable/disable
> > calls.
> 
> Hi Ondrej
> 
> What do you mean by a dummy supply? I'm just trying to make sure you
> are not breaking backwards compatibility.

Sorry, I mean dummy regulator. See:

https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L1874

On systems that use DT (i.e. have_full_constraints() == true), when the
regulator is not found (ENODEV, not specified in DT), regulator_get will return
a fake dummy regulator that can be enabled/disabled, but doesn't do anything
real.

This can be used to avoid NULL checks and make the code simpler.

regards,
	Ondrej

>      Thanks
> 	Andrew
Ondřej Jirman Aug. 20, 2019, 3:56 p.m. UTC | #3 
On Tue, Aug 20, 2019 at 05:39:39PM +0200, Andrew Lunn wrote:
> On Tue, Aug 20, 2019 at 04:53:40PM +0200, megous@megous.com wrote:
> > From: Ondrej Jirman <megous@megous.com>
> > 
> > Use devm_regulator_get instead of devm_regulator_get_optional and rely
> > on dummy supply. This avoids NULL checks before regulator_enable/disable
> > calls.
> 
> Hi Ondrej
> 
> What do you mean by a dummy supply? I'm just trying to make sure you
> are not breaking backwards compatibility.

I have tested it on Orange Pi PC 2, that uses only phy-supply, but not
phy-io-supply, and the kernel now prints:

[    1.410137] dwmac-sun8i 1c30000.ethernet: 1c30000.ethernet supply phy-io not found, using dummy regulator

I have also tested it on Orange Pi PC, that doesn't use external phy, and
instead of:

[    1.081378] dwmac-sun8i 1c30000.ethernet: No regulator found

The kernel now prints:

[    1.112752] dwmac-sun8i 1c30000.ethernet: 1c30000.ethernet supply phy not found, using dummy regulator
[    1.112814] dwmac-sun8i 1c30000.ethernet: 1c30000.ethernet supply phy-io not found, using dummy regulator

Ethernet works in both cases, so that should cover all existing combinations. :)

regards,
	Ondrej


>      Thanks
> 	Andrew
Andrew Lunn Aug. 20, 2019, 3:57 p.m. UTC | #4 
On Tue, Aug 20, 2019 at 05:47:14PM +0200, Ondřej Jirman wrote:
> Hi Andrew,
> 
> On Tue, Aug 20, 2019 at 05:39:39PM +0200, Andrew Lunn wrote:
> > On Tue, Aug 20, 2019 at 04:53:40PM +0200, megous@megous.com wrote:
> > > From: Ondrej Jirman <megous@megous.com>
> > > 
> > > Use devm_regulator_get instead of devm_regulator_get_optional and rely
> > > on dummy supply. This avoids NULL checks before regulator_enable/disable
> > > calls.
> > 
> > Hi Ondrej
> > 
> > What do you mean by a dummy supply? I'm just trying to make sure you
> > are not breaking backwards compatibility.
> 
> Sorry, I mean dummy regulator. See:
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L1874
> 
> On systems that use DT (i.e. have_full_constraints() == true), when the
> regulator is not found (ENODEV, not specified in DT), regulator_get will return
> a fake dummy regulator that can be enabled/disabled, but doesn't do anything
> real.

Hi Ondrej

But we also gain a new warning:

	dev_warn(dev,
		 "%s supply %s not found, using dummy regulator\n",
	         devname, id);

This regulator is clearly optional, so there should not be a warning.

Maybe you can add a new get_type, OPTIONAL_GET, which does not issue
the warning, but does give back a dummy regulator.

Thanks
	Andrew
Ondřej Jirman Aug. 20, 2019, 4:20 p.m. UTC | #5 
Hi,

On Tue, Aug 20, 2019 at 05:57:44PM +0200, Andrew Lunn wrote:
> On Tue, Aug 20, 2019 at 05:47:14PM +0200, Ondřej Jirman wrote:
> > Hi Andrew,
> > 
> > On Tue, Aug 20, 2019 at 05:39:39PM +0200, Andrew Lunn wrote:
> > > On Tue, Aug 20, 2019 at 04:53:40PM +0200, megous@megous.com wrote:
> > > > From: Ondrej Jirman <megous@megous.com>
> > > > 
> > > > Use devm_regulator_get instead of devm_regulator_get_optional and rely
> > > > on dummy supply. This avoids NULL checks before regulator_enable/disable
> > > > calls.
> > > 
> > > Hi Ondrej
> > > 
> > > What do you mean by a dummy supply? I'm just trying to make sure you
> > > are not breaking backwards compatibility.
> > 
> > Sorry, I mean dummy regulator. See:
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L1874
> > 
> > On systems that use DT (i.e. have_full_constraints() == true), when the
> > regulator is not found (ENODEV, not specified in DT), regulator_get will return
> > a fake dummy regulator that can be enabled/disabled, but doesn't do anything
> > real.
> 
> Hi Ondrej
> 
> But we also gain a new warning:
> 
> 	dev_warn(dev,
> 		 "%s supply %s not found, using dummy regulator\n",
> 	         devname, id);
> 
> This regulator is clearly optional, so there should not be a warning.
> 
> Maybe you can add a new get_type, OPTIONAL_GET, which does not issue
> the warning, but does give back a dummy regulator.

We already had a info message. See my other e-mail with the dmesg output.

IMO, that warning is useful during development, and more informative than the
previous one.

regards,
	o.

> Thanks
> 	Andrew
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index 4083019c547a..3e951a11aec3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -528,12 +528,10 @@  static int sun8i_dwmac_init(struct platform_device *pdev, void *priv)
 	struct sunxi_priv_data *gmac = priv;
 	int ret;
 
-	if (gmac->regulator) {
-		ret = regulator_enable(gmac->regulator);
-		if (ret) {
-			dev_err(&pdev->dev, "Fail to enable regulator\n");
-			return ret;
-		}
+	ret = regulator_enable(gmac->regulator);
+	if (ret) {
+		dev_err(&pdev->dev, "Fail to enable regulator\n");
+		return ret;
 	}
 
 	ret = clk_prepare_enable(gmac->tx_clk);
@@ -992,8 +990,7 @@  static void sun8i_dwmac_exit(struct platform_device *pdev, void *priv)
 
 	clk_disable_unprepare(gmac->tx_clk);
 
-	if (gmac->regulator)
-		regulator_disable(gmac->regulator);
+	regulator_disable(gmac->regulator);
 }
 
 static void sun8i_dwmac_set_mac_loopback(void __iomem *ioaddr, bool enable)
@@ -1129,12 +1126,12 @@  static int sun8i_dwmac_probe(struct platform_device *pdev)
 	}
 
 	/* Optional regulator for PHY */
-	gmac->regulator = devm_regulator_get_optional(dev, "phy");
+	gmac->regulator = devm_regulator_get(dev, "phy");
 	if (IS_ERR(gmac->regulator)) {
-		if (PTR_ERR(gmac->regulator) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
-		dev_info(dev, "No regulator found\n");
-		gmac->regulator = NULL;
+		ret = PTR_ERR(gmac->regulator);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get PHY regulator (%d)\n", ret);
+		return ret;
 	}
 
 	/* The "GMAC clock control" register might be located in the