Re: [PATCH net] net: phy: dp83867: restore the LED polarity after a soft reset

[Donggeun Yoo <donggeunyoo.kernel@gmail.com>]

On Tue, Sep 08, 2026 at 04:14:58PM +0200, Andrew Lunn wrote:
> What about the other bits in LEDCR2? dp83867_led_brightness_set()
> might of been used to turn the LED on/off in order to show the state
> of my caps lock key, etc.

You're right. LEDCR2 is cleared by SW_RESET -- v1 already showed the
polarity there is lost, and the DRV_EN/DRV_VAL on/off bits from
led_brightness_set() live in the same register, so a manually driven LED
loses its state too. Polarity-only was a partial fix.

For v2 I'll shadow what the driver programs into LEDCR2 (value plus a
written-bits mask, updated in the setters) and replay it from
config_init(), which runs right after the soft reset. That restores both
polarity and the software-driven on/off state. The runtime setters run
under phydev->lock but the resume-path phy_init_hw() does not, so v2 will
define how the shadow read is serialized against a concurrent setter
rather than leaving it racy.

The function nibble in LEDCR1 is lost the same way. The DP83867IR/CR
datasheet (Rev J) section 7.5.5.3 says the global software reset resets
all internal circuits including the IEEE-defined and extended registers
to defaults, and LEDCR1 (0x18) is an extended register, so SW_RESET
clears it too. The netdev trigger only re-issues the function on its next
set_baseline_state(), i.e. the next link or activity event, not on the
reset, so an offloaded LED whose link stays down after a resume would sit
at the reset-default function indefinitely -- the same failure this patch
fixes, one register up. So v2 shadows and replays LEDCR1 as well:
config_init() rewrites the last function the driver programmed, which is
the trigger's own last intent, and the trigger's next update writes the
identical value.

I did consider pushing this into phylib so every soft-resetting PHY would
benefit. The core could reapply the DT polarity, and that alone would let
aquantia drop its leds_active_low/high cache (61578f679378). But it can't
restore the software-driven on/off state -- the core has no way to tell a
software-driven LED from one offloaded to a hw trigger without reaching
into the trigger's private state -- so that half stays in the driver
regardless, and splitting the restore across two layers seemed worse than
keeping it in one. So I dropped the core route and kept it all in the
driver. I may well have missed a core mechanism that would change that; if
so, let me know and I'll rework it.

Thanks,
Donggeun