| Message ID | 20190820145343.29108-1-megous@megous.com |
|---|---|
| Headers | show |
| Series | Add ethernet support for Orange Pi 3 | expand |
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
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
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
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
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
It looks like there will be some updates to this series either involving adding -supply to the property names or adjusting some of the kernel log messages. Seriously, I would prefer less verbiage in the logs rather than more.
From: Ondrej Jirman <megous@megous.com> This series implements ethernet support for Xunlong Orange Pi 3 board, by: - making small cleanups of existing dwmac-sun8i code - adding DT bindings docummentation - adding support for phy-io-supply to dwmac-sun8i code - adding DT configuration for Orange Pi 3 board For some people, ethernet doesn't work after reboot because u-boot doesn't support AXP805 PMIC, and will not turn off the etherent PHY regulators. So the regulator controlled by gpio will be shut down, but the other one controlled by the AXP PMIC will not. This is a problem only when running with a builtin driver. This needs to be fixed in u-boot and should not prevent these patches from being merged. This evolved out of the Orange Pi 3 patches series, as I didn't want to stretch that out any longer. Please take a look. thank you and regards, Ondrej Jirman Ondrej Jirman (6): dt-bindings: net: sun8i-a83t-emac: Add phy-supply property dt-bindings: net: sun8i-a83t-emac: Add phy-io-supply property net: stmmac: sun8i: Use devm_regulator_get for PHY regulator net: stmmac: sun8i: Rename PHY regulator variable to regulator_phy net: stmmac: sun8i: Add support for enabling a regulator for PHY I/O pins arm64: dts: allwinner: orange-pi-3: Enable ethernet .../net/allwinner,sun8i-a83t-emac.yaml | 8 ++ .../dts/allwinner/sun50i-h6-orangepi-3.dts | 40 ++++++++++ .../net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 74 ++++++++++++------- 3 files changed, 96 insertions(+), 26 deletions(-)