Re: [PATCH net 2/2] net: phy: aquantia: fix applying active_low bit after reset

[Daniel Golle <daniel@makrotopia.org>]

Hi Russell,

On Tue, Sep 17, 2024 at 03:41:11PM +0100, Russell King (Oracle) wrote:
> On Tue, Sep 17, 2024 at 02:49:55PM +0100, Daniel Golle wrote:
> > for_each_set_bit was used wrongly in aqr107_config_init() when iterating
> > over LEDs. Drop misleading 'index' variable and call
> > aqr_phy_led_active_low_set() for each set bit representing an LED which
> > is driven by VDD instead of GND pin.
> 
> Assuming that the intention is only to set LEDs active-low that were
> previously configured to be active-low, then:

Exactly. That was supposedly also the original intention.

> 
> Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> 
> It's good that we don't call aqr_phy_led_active_low_set() for every LED
> in the .config_init method because we don't know whether the LED
> outputs for this PHY are used on SFPs to drive e.g. the SFP LOS pin,
> and changing LED settings in such a case could cause incorrect
> signalling. If this ever changes, then this code needs to be
> conditional on !phy_on_sfp(phydev).

Ack. The post-reset default is active-high and the only case we need to
cover here are LEDs for which VDD is driven instead of GND, so
supposedly if any of those signals are used as LOS in a SFP module it
would be wired in a way which uses the post-reset default of the PHY
anyway which were aren't changing in this case.

Thank you for the quick review!


Daniel