[PATCH net 1/2] net: phy: aquantia: fix setting active_low bit

[PATCH net 1/2] net: phy: aquantia: fix setting active_low bit

[Daniel Golle]

phy_modify_mmd was used wrongly in aqr_phy_led_active_low_set() resulting
in a no-op instead of setting the VEND1_GLOBAL_LED_DRIVE_VDD bit.
Correctly set VEND1_GLOBAL_LED_DRIVE_VDD bit.

Fixes: 61578f679378 ("net: phy: aquantia: add support for PHY LEDs")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/phy/aquantia/aquantia_leds.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/aquantia/aquantia_leds.c b/drivers/net/phy/aquantia/aquantia_leds.c
index 0516ac02c3f81..201c8df93fad9 100644
--- a/drivers/net/phy/aquantia/aquantia_leds.c
+++ b/drivers/net/phy/aquantia/aquantia_leds.c
@@ -120,7 +120,8 @@ int aqr_phy_led_hw_control_set(struct phy_device *phydev, u8 index,
 int aqr_phy_led_active_low_set(struct phy_device *phydev, int index, bool enable)
 {
 	return phy_modify_mmd(phydev, MDIO_MMD_VEND1, AQR_LED_DRIVE(index),
-			      VEND1_GLOBAL_LED_DRIVE_VDD, enable);
+			      VEND1_GLOBAL_LED_DRIVE_VDD,
+			      enable ? VEND1_GLOBAL_LED_DRIVE_VDD : 0);
 }
 
 int aqr_phy_led_polarity_set(struct phy_device *phydev, int index, unsigned long modes)
-- 
2.46.1

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

[Daniel Golle]

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(-)

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;
-- 
2.46.1

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

[Russell King (Oracle)]

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!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Daniel Golle]

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

Re: [PATCH net 1/2] net: phy: aquantia: fix setting active_low bit

[Russell King (Oracle)]

On Tue, Sep 17, 2024 at 02:49:40PM +0100, Daniel Golle wrote:
> phy_modify_mmd was used wrongly in aqr_phy_led_active_low_set() resulting
> in a no-op instead of setting the VEND1_GLOBAL_LED_DRIVE_VDD bit.
> Correctly set VEND1_GLOBAL_LED_DRIVE_VDD bit.
> 
> Fixes: 61578f679378 ("net: phy: aquantia: add support for PHY LEDs")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net 1/2] net: phy: aquantia: fix setting active_low bit

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Tue, 17 Sep 2024 14:49:40 +0100 you wrote:
> phy_modify_mmd was used wrongly in aqr_phy_led_active_low_set() resulting
> in a no-op instead of setting the VEND1_GLOBAL_LED_DRIVE_VDD bit.
> Correctly set VEND1_GLOBAL_LED_DRIVE_VDD bit.
> 
> Fixes: 61578f679378 ("net: phy: aquantia: add support for PHY LEDs")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> 
> [...]

Here is the summary with links:
  - [net,1/2] net: phy: aquantia: fix setting active_low bit
    https://git.kernel.org/netdev/net/c/d2b366c43443
  - [net,2/2] net: phy: aquantia: fix applying active_low bit after reset
    https://git.kernel.org/netdev/net/c/6f9defaf9912

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html