| Message ID | 20171128164822.29218-1-clabbe.montjoie@gmail.com |
|---|---|
| State | Accepted |
| Commit | 1c08ac0c4bd8e9d66c4dde29bc496c3b430dd028 |
| Headers | show |
| Series | net: stmmac: dwmac-sun8i: fix allwinner,leds-active-low handling | expand |
On Tue, Nov 28, 2017 at 05:48:22PM +0100, Corentin Labbe wrote: > The driver expect "allwinner,leds-active-low" to be in PHY node, but > the binding doc expect it to be in MAC node. > > Since all board DT use it also in MAC node, the driver need to search > allwinner,leds-active-low in MAC node. Hi Corentin I'm having trouble working out how this worked before. This is code you moved around, when adding external/internal MDIOs. But the very first version of this driver code used priv->plat->phy_node. Did that somehow point to the MAC node when the internal PHY is used? Or has it been broken all the time? Andrew
On Tue, Nov 28, 2017 at 06:38:26PM +0100, Andrew Lunn wrote: > On Tue, Nov 28, 2017 at 05:48:22PM +0100, Corentin Labbe wrote: > > The driver expect "allwinner,leds-active-low" to be in PHY node, but > > the binding doc expect it to be in MAC node. > > > > Since all board DT use it also in MAC node, the driver need to search > > allwinner,leds-active-low in MAC node. > > Hi Corentin > > I'm having trouble working out how this worked before. This is code > you moved around, when adding external/internal MDIOs. But the very > first version of this driver code used priv->plat->phy_node. Did that > somehow point to the MAC node when the internal PHY is used? Or has it > been broken all the time? > Hello Since this feature control only when the activity LED need to blink, nobody see that it was broken. It is indepedant of other internal PHY stuff. Regards
On Wed, Nov 29, 2017 at 10:02:40AM +0100, Corentin Labbe wrote: > On Tue, Nov 28, 2017 at 06:38:26PM +0100, Andrew Lunn wrote: > > On Tue, Nov 28, 2017 at 05:48:22PM +0100, Corentin Labbe wrote: > > > The driver expect "allwinner,leds-active-low" to be in PHY node, but > > > the binding doc expect it to be in MAC node. > > > > > > Since all board DT use it also in MAC node, the driver need to search > > > allwinner,leds-active-low in MAC node. > > > > Hi Corentin > > > > I'm having trouble working out how this worked before. This is code > > you moved around, when adding external/internal MDIOs. But the very > > first version of this driver code used priv->plat->phy_node. Did that > > somehow point to the MAC node when the internal PHY is used? Or has it > > been broken all the time? > > > > Hello > > Since this feature control only when the activity LED need to blink, > nobody see that it was broken. Hi Corentin So it never worked? If it never worked, moving the DT properties into the PHY node, where they belong, won't introduce a regression :-) Andrew
On Wed, Nov 29, 2017 at 11:37 PM, Andrew Lunn <andrew@lunn.ch> wrote: > On Wed, Nov 29, 2017 at 10:02:40AM +0100, Corentin Labbe wrote: >> On Tue, Nov 28, 2017 at 06:38:26PM +0100, Andrew Lunn wrote: >> > On Tue, Nov 28, 2017 at 05:48:22PM +0100, Corentin Labbe wrote: >> > > The driver expect "allwinner,leds-active-low" to be in PHY node, but >> > > the binding doc expect it to be in MAC node. >> > > >> > > Since all board DT use it also in MAC node, the driver need to search >> > > allwinner,leds-active-low in MAC node. >> > >> > Hi Corentin >> > >> > I'm having trouble working out how this worked before. This is code >> > you moved around, when adding external/internal MDIOs. But the very >> > first version of this driver code used priv->plat->phy_node. Did that >> > somehow point to the MAC node when the internal PHY is used? Or has it >> > been broken all the time? >> > >> >> Hello >> > >> Since this feature control only when the activity LED need to blink, >> nobody see that it was broken. > > Hi Corentin > > So it never worked? > > If it never worked, moving the DT properties into the PHY node, where > they belong, won't introduce a regression :-) It worked at one point. During some previous iteration, they lit up as they were supposed to. ChenYu
Hi ChenYu > It worked at one point. During some previous iteration, they lit up as > they were supposed to. For a released version of the kernel? Or during development work? If they did work, but broken, it would be good to know which commit broke it. We can then add a fixes: tag to the patch as proposed. Andrew
On Wed, Nov 29, 2017 at 04:37:12PM +0100, Andrew Lunn wrote: > On Wed, Nov 29, 2017 at 10:02:40AM +0100, Corentin Labbe wrote: > > On Tue, Nov 28, 2017 at 06:38:26PM +0100, Andrew Lunn wrote: > > > On Tue, Nov 28, 2017 at 05:48:22PM +0100, Corentin Labbe wrote: > > > > The driver expect "allwinner,leds-active-low" to be in PHY node, but > > > > the binding doc expect it to be in MAC node. > > > > > > > > Since all board DT use it also in MAC node, the driver need to search > > > > allwinner,leds-active-low in MAC node. > > > > > > Hi Corentin > > > > > > I'm having trouble working out how this worked before. This is code > > > you moved around, when adding external/internal MDIOs. But the very > > > first version of this driver code used priv->plat->phy_node. Did that > > > somehow point to the MAC node when the internal PHY is used? Or has it > > > been broken all the time? > > > > > > > Hello > > > > > Since this feature control only when the activity LED need to blink, > > nobody see that it was broken. > > Hi Corentin > > So it never worked? > > If it never worked, moving the DT properties into the PHY node, where > they belong, won't introduce a regression :-) That's even truer since it's been queued for 4.15 which hasn't been released yet. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
On Wed, Nov 29, 2017 at 11:46 PM, Andrew Lunn <andrew@lunn.ch> wrote: > Hi ChenYu > >> It worked at one point. During some previous iteration, they lit up as >> they were supposed to. > > For a released version of the kernel? Or during development work? If > they did work, but broken, it would be good to know which commit broke > it. We can then add a fixes: tag to the patch as proposed. During development work. The bindings / driver was never released. ChenYu
From: Corentin Labbe <clabbe.montjoie@gmail.com> Date: Tue, 28 Nov 2017 17:48:22 +0100 > The driver expect "allwinner,leds-active-low" to be in PHY node, but > the binding doc expect it to be in MAC node. > > Since all board DT use it also in MAC node, the driver need to search > allwinner,leds-active-low in MAC node. > > Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> I've read over this discussion and I think the thing to do is for me to simply apply this to 'net' for now. Thanks.
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c index e5ff734d4f9b..9eb7f65d8000 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c @@ -808,8 +808,7 @@ static int sun8i_dwmac_set_syscon(struct stmmac_priv *priv) val, reg); if (gmac->variant->soc_has_internal_phy) { - if (of_property_read_bool(priv->plat->phy_node, - "allwinner,leds-active-low")) + if (of_property_read_bool(node, "allwinner,leds-active-low")) reg |= H3_EPHY_LED_POL; else reg &= ~H3_EPHY_LED_POL;
The driver expect "allwinner,leds-active-low" to be in PHY node, but the binding doc expect it to be in MAC node. Since all board DT use it also in MAC node, the driver need to search allwinner,leds-active-low in MAC node. Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com> --- drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) -- 2.13.6