diff mbox

net: stmmac: dwmac-sun8i: fix allwinner, leds-active-low handling

Message ID 20171128164822.29218-1-clabbe.montjoie@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Corentin Labbe Nov. 28, 2017, 4:48 p.m. UTC
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(-)

Comments

Andrew Lunn Nov. 28, 2017, 5:38 p.m. UTC | #1
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
Corentin Labbe Nov. 29, 2017, 9:02 a.m. UTC | #2
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
Andrew Lunn Nov. 29, 2017, 3:37 p.m. UTC | #3
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
Chen-Yu Tsai Nov. 29, 2017, 3:41 p.m. UTC | #4
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
Andrew Lunn Nov. 29, 2017, 3:46 p.m. UTC | #5
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
Maxime Ripard Nov. 29, 2017, 3:47 p.m. UTC | #6
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
Chen-Yu Tsai Nov. 29, 2017, 3:50 p.m. UTC | #7
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
David Miller Nov. 30, 2017, 2:45 p.m. UTC | #8
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 mbox

Patch

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;