| Message ID | 9b1f0cd91f4cda54c8be56b4fe780480baf4aa0f.1726580902.git.daniel@makrotopia.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,1/2] net: phy: aquantia: fix setting active_low bit | expand |
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: 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). Thanks!
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
diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c index e982e9ce44a59..650df261eea8e 100644 --- a/drivers/net/phy/aquantia/aquantia_main.c +++ b/drivers/net/phy/aquantia/aquantia_main.c @@ -478,7 +478,7 @@ static int aqr107_config_init(struct phy_device *phydev) { struct aqr107_priv *priv = phydev->priv; u32 led_active_low; - int ret, index = 0; + int ret; /* Check that the PHY interface type is compatible */ if (phydev->interface != PHY_INTERFACE_MODE_SGMII && @@ -505,10 +505,9 @@ static int aqr107_config_init(struct phy_device *phydev) /* Restore LED polarity state after reset */ for_each_set_bit(led_active_low, &priv->leds_active_low, AQR_MAX_LEDS) { - ret = aqr_phy_led_active_low_set(phydev, index, led_active_low); + ret = aqr_phy_led_active_low_set(phydev, led_active_low, true); if (ret) return ret; - index++; } return 0;
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. Fixes: 61578f679378 ("net: phy: aquantia: add support for PHY LEDs") Signed-off-by: Daniel Golle <daniel@makrotopia.org> --- drivers/net/phy/aquantia/aquantia_main.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)