[PATCH RFC net-next] net: phy: always set polarity_modes if op is supported

[PATCH RFC net-next] net: phy: always set polarity_modes if op is supported

[Daniel Golle]

Some PHYs drive LEDs active-low by default and polarity needs to be
configured in case the 'active-low' property is NOT set.
One way to achieve this without introducing an additional 'active-high'
property would be to always call the led_polarity_set operation if it
is supported by the phy driver.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/phy/phy_device.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 560e338b307a..d25c61223e83 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3362,11 +3362,11 @@ static int of_phy_led(struct phy_device *phydev,
 	if (of_property_read_bool(led, "inactive-high-impedance"))
 		set_bit(PHY_LED_INACTIVE_HIGH_IMPEDANCE, &modes);
 
-	if (modes) {
-		/* Return error if asked to set polarity modes but not supported */
-		if (!phydev->drv->led_polarity_set)
-			return -EINVAL;
+	/* Return error if asked to set polarity modes but not supported */
+	if (modes && !phydev->drv->led_polarity_set)
+		return -EINVAL;
 
+	if (phydev->drv->led_polarity_set) {
 		err = phydev->drv->led_polarity_set(phydev, index, modes);
 		if (err)
 			return err;
-- 
2.46.2

Re: [PATCH RFC net-next] net: phy: always set polarity_modes if op is supported

[Andrew Lunn]

On Fri, Oct 04, 2024 at 04:46:33PM +0100, Daniel Golle wrote:
> Some PHYs drive LEDs active-low by default and polarity needs to be
> configured in case the 'active-low' property is NOT set.
> One way to achieve this without introducing an additional 'active-high'
> property would be to always call the led_polarity_set operation if it
> is supported by the phy driver.

It is a good idea to state why it is RFC. What comments do you want?

So this potentially causes regressions/change in behaviour. Strapping
or the bootloader could set the polarity, and Linux leaves it alone up
until this patch.

The device tree binding says:

  active-low:
    type: boolean
    description:
      Makes LED active low. To turn the LED ON, line needs to be
      set to low voltage instead of high.

There is no mention that the absence of this property means it is
active-high.

In effect, i think we have a tristate, active-low, active-high, don't
touch.

I think adding an active-high property is probably the safest bet,
even if it is more work.

	Andrew

Re: [PATCH RFC net-next] net: phy: always set polarity_modes if op is supported

[Daniel Golle]

On Fri, Oct 04, 2024 at 10:59:45PM +0200, Andrew Lunn wrote:
> On Fri, Oct 04, 2024 at 04:46:33PM +0100, Daniel Golle wrote:
> > Some PHYs drive LEDs active-low by default and polarity needs to be
> > configured in case the 'active-low' property is NOT set.
> > One way to achieve this without introducing an additional 'active-high'
> > property would be to always call the led_polarity_set operation if it
> > is supported by the phy driver.
> 
> It is a good idea to state why it is RFC. What comments do you want?
> 
> [...]
> I think adding an active-high property is probably the safest bet,
> even if it is more work.

Thank you. Exactly that was the clarification I was looking for:
If absence of "active-low" would mean "active-high" or should be
interpreted as "don't touch".

I'll add "active-high" as an additional property then, as I found out
that both, Aquantia and Intel/MaxLinear are technically speaking
active-low by default (ie. after reset) and what we need to set is a
property setting the LED to be driven active-high (ie. driving VDD
rather than GND) instead. I hope it's not too late to make this change
also for the Aquantia driver.

Re: [PATCH RFC net-next] net: phy: always set polarity_modes if op is supported

[Andrew Lunn]

> I'll add "active-high" as an additional property then, as I found out
> that both, Aquantia and Intel/MaxLinear are technically speaking
> active-low by default (ie. after reset) and what we need to set is a
> property setting the LED to be driven active-high (ie. driving VDD
> rather than GND) instead. I hope it's not too late to make this change
> also for the Aquantia driver.

Adding a new property should not affect backwards compatibility, so it
should be safe to merge at any time.

	Andrew

Re: [PATCH RFC net-next] net: phy: always set polarity_modes if op is supported

[Daniel Golle]

On Sat, Oct 05, 2024 at 04:17:56PM +0200, Andrew Lunn wrote:
> > I'll add "active-high" as an additional property then, as I found out
> > that both, Aquantia and Intel/MaxLinear are technically speaking
> > active-low by default (ie. after reset) and what we need to set is a
> > property setting the LED to be driven active-high (ie. driving VDD
> > rather than GND) instead. I hope it's not too late to make this change
> > also for the Aquantia driver.
> 
> Adding a new property should not affect backwards compatibility, so it
> should be safe to merge at any time.

Ok, I will proceed in that direction then and post a patch shortly.
My intial assumption that absence of 'active-low' would always imply
the LED being driven active-high was due to the commit description of
the introduction of the active-low property:

commit c94d1783136eb66f2a464a6891a32eeb55eaeacc
Author: Christian Marangi <ansuelsmth@gmail.com>
Date:   Thu Jan 25 21:36:57 2024 +0100

    dt-bindings: net: phy: Make LED active-low property common

    Move LED active-low property to common.yaml. This property is currently
    defined multiple times by bcm LEDs. This property will now be supported
    in a generic way for PHY LEDs with the use of a generic function.

    With active-low bool property not defined, active-high is always
    assumed.

    Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
    Reviewed-by: Andrew Lunn <andrew@lunn.ch>
    Acked-by: Lee Jones <lee@kernel.org>
    Reviewed-by: Rob Herring <robh@kernel.org>
    Link: https://lore.kernel.org/r/20240125203702.4552-2-ansuelsmth@gmail.com
    Signed-off-by: Jakub Kicinski <kuba@kernel.org>


However, that's not what the code did in the end, it would be either
"set active-low" or "don't touch".

Re: [PATCH RFC net-next] net: phy: always set polarity_modes if op is supported

[Andrew Lunn]

On Sat, Oct 05, 2024 at 04:51:30PM +0100, Daniel Golle wrote:
> On Sat, Oct 05, 2024 at 04:17:56PM +0200, Andrew Lunn wrote:
> > > I'll add "active-high" as an additional property then, as I found out
> > > that both, Aquantia and Intel/MaxLinear are technically speaking
> > > active-low by default (ie. after reset) and what we need to set is a
> > > property setting the LED to be driven active-high (ie. driving VDD
> > > rather than GND) instead. I hope it's not too late to make this change
> > > also for the Aquantia driver.
> > 
> > Adding a new property should not affect backwards compatibility, so it
> > should be safe to merge at any time.
> 
> Ok, I will proceed in that direction then and post a patch shortly.
> My intial assumption that absence of 'active-low' would always imply
> the LED being driven active-high was due to the commit description of
> the introduction of the active-low property:
> 
> commit c94d1783136eb66f2a464a6891a32eeb55eaeacc
> Author: Christian Marangi <ansuelsmth@gmail.com>
> Date:   Thu Jan 25 21:36:57 2024 +0100
> 
>     dt-bindings: net: phy: Make LED active-low property common
> 
>     Move LED active-low property to common.yaml. This property is currently
>     defined multiple times by bcm LEDs. This property will now be supported
>     in a generic way for PHY LEDs with the use of a generic function.
> 
>     With active-low bool property not defined, active-high is always
>     assumed.

So we have a difference between the commit message and what the
binding actually says. I would go by what the binding says.

However, what about the actual implementations? Do any do what the
commit message says?

	Andrew

Re: [PATCH RFC net-next] net: phy: always set polarity_modes if op is supported

[Daniel Golle]

On Sat, Oct 05, 2024 at 06:35:58PM +0200, Andrew Lunn wrote:
> On Sat, Oct 05, 2024 at 04:51:30PM +0100, Daniel Golle wrote:
> > On Sat, Oct 05, 2024 at 04:17:56PM +0200, Andrew Lunn wrote:
> > > > I'll add "active-high" as an additional property then, as I found out
> > > > that both, Aquantia and Intel/MaxLinear are technically speaking
> > > > active-low by default (ie. after reset) and what we need to set is a
> > > > property setting the LED to be driven active-high (ie. driving VDD
> > > > rather than GND) instead. I hope it's not too late to make this change
> > > > also for the Aquantia driver.
> > > 
> > > Adding a new property should not affect backwards compatibility, so it
> > > should be safe to merge at any time.
> > 
> > Ok, I will proceed in that direction then and post a patch shortly.
> > My intial assumption that absence of 'active-low' would always imply
> > the LED being driven active-high was due to the commit description of
> > the introduction of the active-low property:
> > 
> > commit c94d1783136eb66f2a464a6891a32eeb55eaeacc
> > Author: Christian Marangi <ansuelsmth@gmail.com>
> > Date:   Thu Jan 25 21:36:57 2024 +0100
> > 
> >     dt-bindings: net: phy: Make LED active-low property common
> > 
> >     Move LED active-low property to common.yaml. This property is currently
> >     defined multiple times by bcm LEDs. This property will now be supported
> >     in a generic way for PHY LEDs with the use of a generic function.
> > 
> >     With active-low bool property not defined, active-high is always
> >     assumed.
> 
> So we have a difference between the commit message and what the
> binding actually says. I would go by what the binding says.

+1

> 
> However, what about the actual implementations? Do any do what the
> commit message says?

The current implementation for PHY LEDs:
 - 'active-low' property is present: Change LED polarity (in many cases
   wrongly from initially being active-low to active-high).
 - 'active-low' property is not set: Don't touch polarity settings.

See drivers/net/phy/phy_device.c, from line 3360:
        if (of_property_read_bool(led, "active-low"))
                set_bit(PHY_LED_ACTIVE_LOW, &modes);
        if (of_property_read_bool(led, "inactive-high-impedance"))
                set_bit(PHY_LED_INACTIVE_HIGH_IMPEDANCE, &modes);
 
        if (modes) {
                /* Return error if asked to set polarity modes but not supported */
                if (!phydev->drv->led_polarity_set)
                        return -EINVAL;
 
                err = phydev->drv->led_polarity_set(phydev, index, modes);
                if (err)
                        return err;
        }

led_polarity_set() is not called if neither 'active-low' nor
'inactive-high-impedance' are set.