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@xxxxxxxxx/

Thanks for the review.