[PATCH net v2] net: phy: qca808x: handle the active-high LED polarity mode

[PATCH net v2] net: phy: qca808x: handle the active-high LED polarity mode

[Donggeun Yoo]

Commit a274465cc3be ("net: phy: support 'active-high' property for PHY
LEDs") added PHY_LED_ACTIVE_HIGH but did not cover qca808x, so
qca808x_led_polarity_set() returns -EINVAL for it. phy_probe() propagates
that, leaving the mdio device unbound and phy_attach_direct() falling back
to the genphy driver.

Accepting the mode alone is not enough: led_polarity_mode records
active_low, so an explicit 'active-high' node stores 0, while
qca808x_config_init() re-asserts QCA808X_LED_ACTIVE_HIGH only for -1. The
bit does not survive the reset phy_init_hw() runs first, so the LED would
come up active-low.

An 'active-low' node needs nothing here: the bit is clear after the reset,
which is the polarity it asks for.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908105959.70453-1-donggeunyoo.kernel%40gmail.com
Cc: stable@vger.kernel.org
Fixes: a274465cc3be ("net: phy: support 'active-high' property for PHY LEDs")
Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
Assisted-by: Claude:claude-fable-5 sparse
---
v2:
  - No code change.  Rebased onto net/main.
  - Cc Rosen Penev and Christian Marangi, per Andrew Lunn -- they may have
    QCA808x hardware.  Christian wrote f203c8c77c76 ("net: phy: qcom:
    qca808x: default to LED active High if not set"), the default this
    patch extends.  If either of you can put an 'active-high' LED node on
    a QCA8081 and confirm the LED comes up the right way round, I would
    appreciate it.
  - Say why an 'active-low' node needs nothing in qca808x_config_init().
v1: https://lore.kernel.org/netdev/20260912012819.2072067-1-donggeunyoo.kernel@gmail.com/
  - Supersedes [PATCH net v3 3/3],
    20260908105959.70453-4-donggeunyoo.kernel@gmail.com, whose
    Reviewed-by from Andrew Lunn is dropped: that version only accepted
    the mode, this one also makes it survive the reset.

Compile-tested only (W=1, sparse).  I have no QCA808x hardware, and there is
no KUnit suite or MDIO mock under drivers/net/phy to exercise the MMD7 write
without it.

 drivers/net/phy/qcom/qca808x.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/qcom/qca808x.c b/drivers/net/phy/qcom/qca808x.c
index 8eb51b1a006c..68fd4d0fe31b 100644
--- a/drivers/net/phy/qcom/qca808x.c
+++ b/drivers/net/phy/qcom/qca808x.c
@@ -200,8 +200,8 @@ static int qca808x_config_init(struct phy_device *phydev)
 	struct qca808x_priv *priv = phydev->priv;
 	int ret;
 
-	/* Default to LED Active High if active-low not in DT */
-	if (priv->led_polarity_mode == -1) {
+	/* Set LED Active High unless active-low was requested in DT */
+	if (priv->led_polarity_mode != 1) {
 		ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN,
 				       QCA808X_MMD7_LED_POLARITY_CTRL,
 				       QCA808X_LED_ACTIVE_HIGH);
@@ -603,6 +603,9 @@ static int qca808x_led_polarity_set(struct phy_device *phydev, int index,
 		case PHY_LED_ACTIVE_LOW:
 			active_low = true;
 			break;
+		case PHY_LED_ACTIVE_HIGH:
+			active_low = false;
+			break;
 		default:
 			return -EINVAL;
 		}

base-commit: e6b6078ea1731b05b3b552497b3bce4bf8b014ae
-- 
2.53.0

Re: [PATCH net v2] net: phy: qca808x: handle the active-high LED polarity mode

[Rosen Penev]

On Mon, Sep 14, 2026 at 2:47 PM Donggeun Yoo
<donggeunyoo.kernel@gmail.com> wrote:
>
> Commit a274465cc3be ("net: phy: support 'active-high' property for PHY
> LEDs") added PHY_LED_ACTIVE_HIGH but did not cover qca808x, so
> qca808x_led_polarity_set() returns -EINVAL for it. phy_probe() propagates
> that, leaving the mdio device unbound and phy_attach_direct() falling back
> to the genphy driver.
>
> Accepting the mode alone is not enough: led_polarity_mode records
> active_low, so an explicit 'active-high' node stores 0, while
> qca808x_config_init() re-asserts QCA808X_LED_ACTIVE_HIGH only for -1. The
> bit does not survive the reset phy_init_hw() runs first, so the LED would
> come up active-low.
>
> An 'active-low' node needs nothing here: the bit is clear after the reset,
> which is the polarity it asks for.
>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908105959.70453-1-donggeunyoo.kernel%40gmail.com
> Cc: stable@vger.kernel.org
> Fixes: a274465cc3be ("net: phy: support 'active-high' property for PHY LEDs")
> Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
> Assisted-by: Claude:claude-fable-5 sparse
> ---
> v2:
>   - No code change.  Rebased onto net/main.
>   - Cc Rosen Penev and Christian Marangi, per Andrew Lunn -- they may have
>     QCA808x hardware.

In my case, qca8k controls the LEDs. I don't think I have any devices
that control LEDs through qca808x.

>     Christian wrote f203c8c77c76 ("net: phy: qcom:
>     qca808x: default to LED active High if not set"), the default this
>     patch extends.  If either of you can put an 'active-high' LED node on
>     a QCA8081 and confirm the LED comes up the right way round, I would
>     appreciate it.
>   - Say why an 'active-low' node needs nothing in qca808x_config_init().
> v1: https://lore.kernel.org/netdev/20260912012819.2072067-1-donggeunyoo.kernel@gmail.com/
>   - Supersedes [PATCH net v3 3/3],
>     20260908105959.70453-4-donggeunyoo.kernel@gmail.com, whose
>     Reviewed-by from Andrew Lunn is dropped: that version only accepted
>     the mode, this one also makes it survive the reset.
>
> Compile-tested only (W=1, sparse).  I have no QCA808x hardware, and there is
> no KUnit suite or MDIO mock under drivers/net/phy to exercise the MMD7 write
> without it.
>
>  drivers/net/phy/qcom/qca808x.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/qcom/qca808x.c b/drivers/net/phy/qcom/qca808x.c
> index 8eb51b1a006c..68fd4d0fe31b 100644
> --- a/drivers/net/phy/qcom/qca808x.c
> +++ b/drivers/net/phy/qcom/qca808x.c
> @@ -200,8 +200,8 @@ static int qca808x_config_init(struct phy_device *phydev)
>         struct qca808x_priv *priv = phydev->priv;
>         int ret;
>
> -       /* Default to LED Active High if active-low not in DT */
> -       if (priv->led_polarity_mode == -1) {
> +       /* Set LED Active High unless active-low was requested in DT */
> +       if (priv->led_polarity_mode != 1) {
>                 ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN,
>                                        QCA808X_MMD7_LED_POLARITY_CTRL,
>                                        QCA808X_LED_ACTIVE_HIGH);
> @@ -603,6 +603,9 @@ static int qca808x_led_polarity_set(struct phy_device *phydev, int index,
>                 case PHY_LED_ACTIVE_LOW:
>                         active_low = true;
>                         break;
> +               case PHY_LED_ACTIVE_HIGH:
> +                       active_low = false;
> +                       break;
>                 default:
>                         return -EINVAL;
>                 }
>
> base-commit: e6b6078ea1731b05b3b552497b3bce4bf8b014ae
> --
> 2.53.0
>

Re: [PATCH net v2] net: phy: qca808x: handle the active-high LED polarity mode

[Christian Marangi (Ansuel)]

Il giorno lun 14 set 2026 alle ore 23:47 Donggeun Yoo
<donggeunyoo.kernel@gmail.com> ha scritto:
>
> Commit a274465cc3be ("net: phy: support 'active-high' property for PHY
> LEDs") added PHY_LED_ACTIVE_HIGH but did not cover qca808x, so
> qca808x_led_polarity_set() returns -EINVAL for it. phy_probe() propagates
> that, leaving the mdio device unbound and phy_attach_direct() falling back
> to the genphy driver.
>
> Accepting the mode alone is not enough: led_polarity_mode records
> active_low, so an explicit 'active-high' node stores 0, while
> qca808x_config_init() re-asserts QCA808X_LED_ACTIVE_HIGH only for -1. The
> bit does not survive the reset phy_init_hw() runs first, so the LED would
> come up active-low.
>
> An 'active-low' node needs nothing here: the bit is clear after the reset,
> which is the polarity it asks for.
>

The commit description looks a bit "dense" and took me well 2-3 minutes to
parse the english and understand the change. Was also the commit description
assisted by LLM?

Anyway more comments below for the patch.

> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908105959.70453-1-donggeunyoo.kernel%40gmail.com
> Cc: stable@vger.kernel.org
> Fixes: a274465cc3be ("net: phy: support 'active-high' property for PHY LEDs")
> Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
> Assisted-by: Claude:claude-fable-5 sparse
> ---
> v2:
>   - No code change.  Rebased onto net/main.
>   - Cc Rosen Penev and Christian Marangi, per Andrew Lunn -- they may have
>     QCA808x hardware.  Christian wrote f203c8c77c76 ("net: phy: qcom:
>     qca808x: default to LED active High if not set"), the default this
>     patch extends.  If either of you can put an 'active-high' LED node on
>     a QCA8081 and confirm the LED comes up the right way round, I would
>     appreciate it.
>   - Say why an 'active-low' node needs nothing in qca808x_config_init().
> v1: https://lore.kernel.org/netdev/20260912012819.2072067-1-donggeunyoo.kernel@gmail.com/
>   - Supersedes [PATCH net v3 3/3],
>     20260908105959.70453-4-donggeunyoo.kernel@gmail.com, whose
>     Reviewed-by from Andrew Lunn is dropped: that version only accepted
>     the mode, this one also makes it survive the reset.
>
> Compile-tested only (W=1, sparse).  I have no QCA808x hardware, and there is
> no KUnit suite or MDIO mock under drivers/net/phy to exercise the MMD7 write
> without it.
>
>  drivers/net/phy/qcom/qca808x.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/qcom/qca808x.c b/drivers/net/phy/qcom/qca808x.c
> index 8eb51b1a006c..68fd4d0fe31b 100644
> --- a/drivers/net/phy/qcom/qca808x.c
> +++ b/drivers/net/phy/qcom/qca808x.c
> @@ -200,8 +200,8 @@ static int qca808x_config_init(struct phy_device *phydev)
>         struct qca808x_priv *priv = phydev->priv;
>         int ret;
>
> -       /* Default to LED Active High if active-low not in DT */
> -       if (priv->led_polarity_mode == -1) {
> +       /* Set LED Active High unless active-low was requested in DT */

Why the comment was changed if it does say exactly the same thing?

> +       if (priv->led_polarity_mode != 1) {

This change is O.K but I would ask you to better clarify this. -1, 0 and 1
is very confusing here.

I would introduce a simple define like
#define QCA808X_PHY_LED_UNSET -1

And set this in probe and change the condition here to directly check
for the macro PHY_LED_ACTIVE_LOW.

>                 ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN,
>                                        QCA808X_MMD7_LED_POLARITY_CTRL,
>                                        QCA808X_LED_ACTIVE_HIGH);
> @@ -603,6 +603,9 @@ static int qca808x_led_polarity_set(struct phy_device *phydev, int index,
>                 case PHY_LED_ACTIVE_LOW:
>                         active_low = true;
>                         break;
> +               case PHY_LED_ACTIVE_HIGH:
> +                       active_low = false;
> +                       break;

The ACTIVE_HIGH wasn't a thing when this was introduced if I'm not wrong.
Anyway this change looks good. The only problem is that I feel it would be
better to split this patch in 2 different commits.

This really addresses 2 different problems and splitting also makes the commit
description easier to understand.

One doesn't account the case where phy is reset, the other doesn't account
the mode in led_polarity set.

>                 default:
>                         return -EINVAL;
>                 }
>

Re: [PATCH net v2] net: phy: qca808x: handle the active-high LED polarity mode

[Donggeun Yoo]

On Tue, Sep 15, 2026 at 12:25:54PM +0200, Christian Marangi (Ansuel) wrote:
> The commit description looks a bit "dense" and took me well 2-3 minutes to
> parse the english and understand the change. Was also the commit description
> assisted by LLM?

Yes, drafted with AI help - that is what the Assisted-by: trailer records -
and reviewed carefully before sending. Which part cost you the most to get
through? I would rather fix that than guess at it.

> > -       /* Default to LED Active High if active-low not in DT */
> > +       /* Set LED Active High unless active-low was requested in DT */
>
> Why the comment was changed if it does say exactly the same thing?

Because after this patch it no longer does. Until now that branch ran only
when DT said nothing about polarity, so "Default to" was exact. It now also
runs when DT asks for active-high explicitly, and there the driver is
following an instruction rather than applying a default.

> This change is O.K but I would ask you to better clarify this. -1, 0 and 1
> is very confusing here.
>
> I would introduce a simple define like
> #define QCA808X_PHY_LED_UNSET -1
>
> And set this in probe and change the condition here to directly check
> for the macro PHY_LED_ACTIVE_LOW.

Done in v3. The >= 0 test in qca808x_led_polarity_set() took the define
too.

> The only problem is that I feel it would be better to split this patch in 2
> different commits.
>
> This really addresses 2 different problems and splitting also makes the commit
> description easier to understand.
>
> One doesn't account the case where phy is reset, the other doesn't account
> the mode in led_polarity set.

Agreed, and v3 is split that way:

  1/2 qca808x_led_polarity_set(): accept PHY_LED_ACTIVE_HIGH, so the PHY
      binds at all.
  2/2 qca808x_config_init(): re-assert the bit for an explicit
      active-high, carrying QCA808X_PHY_LED_UNSET and the comment.

Both Fixes: a274465cc3be and Cc: stable, so they backport together. After
1/2 alone an 'active-high' node binds and the LED comes up active-low,
which is still ahead of today, where led_polarity_set() returns -EINVAL,
phy_probe() fails and the PHY falls back to genphy.

It is no longer compile-tested only. I put a synthetic MDIO bus behind
phylib that answers as a QCA8081 and emulates MMD7 0x901a, with the reset
clearing BIT(6) as your f203c8c77c76 describes, and ran the same harness
over all three arms:

  DT node            base      1/2 only   1/2 + 2/2
  active-high        -EINVAL   inverted   correct
  active-low         correct   correct    correct
  no polarity node   correct   correct    correct
  high-impedance     -EINVAL   -EINVAL    -EINVAL

The middle column is the reason the second patch exists. A confirmation on
a real QCA8081 would still be worth more than an emulated register, if you
have one to hand.

v3: https://lore.kernel.org/netdev/20260915223138.321307-1-donggeunyoo.kernel@gmail.com/

Thanks for the review.