diff mbox series

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

Message ID 9b1f0cd91f4cda54c8be56b4fe780480baf4aa0f.1726580902.git.daniel@makrotopia.org (mailing list archive)
State Accepted
Commit 6f9defaf99122d1af9c2562181c77bc99be0672d
Delegated to: Netdev Maintainers
Headers show
Series [net,1/2] net: phy: aquantia: fix setting active_low bit | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang success Errors and warnings before: 16 this patch: 16
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 16 this patch: 16
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 19 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-09-17--15-00 (tests: 764)

Commit Message

Daniel Golle Sept. 17, 2024, 1:49 p.m. UTC
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(-)

Comments

Russell King (Oracle) Sept. 17, 2024, 2:41 p.m. UTC | #1
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!
Daniel Golle Sept. 17, 2024, 3:23 p.m. UTC | #2
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 mbox series

Patch

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;