[PATCH 0/2] net: phy: adin1100: Fix powerdown mode setting

[PATCH 0/2] net: phy: adin1100: Fix powerdown mode setting

[Alexander Dahl]

Hei hei,

while building a new device around the ADIN1100 I noticed some errors in
kernel log when calling `ifdown` on the ethernet device.  Series has a
straight forward fix and an obvious follow-up code simplification.

Greets
Alex

Alexander Dahl (2):
  net: phy: adin1100: Fix software power-down ready condition
  net: phy: adin1100: Simplify register value passing

 drivers/net/phy/adin1100.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)


base-commit: 6a23ae0a96a600d1d12557add110e0bb6e32730c
-- 
2.39.5

[PATCH 1/2] net: phy: adin1100: Fix software power-down ready condition

[Alexander Dahl]

Value CRSM_SFT_PD written to Software Power-Down Control Register
(CRSM_SFT_PD_CNTRL) is 0x01 and therefor different to value
CRSM_SFT_PD_RDY (0x02) read from System Status Register (CRSM_STAT) for
confirmation powerdown has been reached.

The condition could have only worked when disabling powerdown
(both 0x00), but never when enabling it (0x01 != 0x02).

Result is a timeout, like so:

    $ ifdown eth0
    macb f802c000.ethernet eth0: Link is Down
    ADIN1100 f802c000.ethernet-ffffffff:01: adin_set_powerdown_mode failed: -110
    ADIN1100 f802c000.ethernet-ffffffff:01: adin_set_powerdown_mode failed: -110

Fixes: 7eaf9132996a ("net: phy: adin1100: Add initial support for ADIN1100 industrial PHY")
Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 drivers/net/phy/adin1100.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/adin1100.c b/drivers/net/phy/adin1100.c
index bd7a47a903ac..10b796c2daee 100644
--- a/drivers/net/phy/adin1100.c
+++ b/drivers/net/phy/adin1100.c
@@ -201,7 +201,7 @@ static int adin_set_powerdown_mode(struct phy_device *phydev, bool en)
 		return ret;
 
 	return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1, ADIN_CRSM_STAT, ret,
-					 (ret & ADIN_CRSM_SFT_PD_RDY) == val,
+					 !!(ret & ADIN_CRSM_SFT_PD_RDY) == en,
 					 1000, 30000, true);
 }
 
-- 
2.39.5

[PATCH 2/2] net: phy: adin1100: Simplify register value passing

[Alexander Dahl]

The additional use case for that variable is gone,
the expression is simple enough to pass it inline now.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 drivers/net/phy/adin1100.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/adin1100.c b/drivers/net/phy/adin1100.c
index 10b796c2daee..8f9753d4318c 100644
--- a/drivers/net/phy/adin1100.c
+++ b/drivers/net/phy/adin1100.c
@@ -192,11 +192,10 @@ static irqreturn_t adin_phy_handle_interrupt(struct phy_device *phydev)
 static int adin_set_powerdown_mode(struct phy_device *phydev, bool en)
 {
 	int ret;
-	int val;
 
-	val = en ? ADIN_CRSM_SFT_PD_CNTRL_EN : 0;
 	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
-			    ADIN_CRSM_SFT_PD_CNTRL, val);
+			    ADIN_CRSM_SFT_PD_CNTRL,
+			    en ? ADIN_CRSM_SFT_PD_CNTRL_EN : 0);
 	if (ret < 0)
 		return ret;
 
-- 
2.39.5

Re: [PATCH 2/2] net: phy: adin1100: Simplify register value passing

[Russell King (Oracle)]

On Wed, Nov 19, 2025 at 01:47:37PM +0100, Alexander Dahl wrote:
> The additional use case for that variable is gone,
> the expression is simple enough to pass it inline now.

Looking at net-next, this patch is wrong. You remove "val" but the code
after the context in this patch is:

        return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1, ADIN_CRSM_STAT, ret,
                                         (ret & ADIN_CRSM_SFT_PD_RDY) == val,
                                         1000, 30000, true);

which also references "val".

Note that this code is buggy as things stand. "val" will be zero (when
en is false) or 1 (when en is true), whereas ADIN_CRSM_SFT_PD_RDY is 2.
Thus, if en is true, then the condition can never be true.

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

Re: [PATCH 1/2] net: phy: adin1100: Fix software power-down ready condition

[Russell King (Oracle)]

On Wed, Nov 19, 2025 at 01:47:36PM +0100, Alexander Dahl wrote:
> Value CRSM_SFT_PD written to Software Power-Down Control Register
> (CRSM_SFT_PD_CNTRL) is 0x01 and therefor different to value
> CRSM_SFT_PD_RDY (0x02) read from System Status Register (CRSM_STAT) for
> confirmation powerdown has been reached.
> 
> The condition could have only worked when disabling powerdown
> (both 0x00), but never when enabling it (0x01 != 0x02).
> 
> Result is a timeout, like so:
> 
>     $ ifdown eth0
>     macb f802c000.ethernet eth0: Link is Down
>     ADIN1100 f802c000.ethernet-ffffffff:01: adin_set_powerdown_mode failed: -110
>     ADIN1100 f802c000.ethernet-ffffffff:01: adin_set_powerdown_mode failed: -110
> 
> Fixes: 7eaf9132996a ("net: phy: adin1100: Add initial support for ADIN1100 industrial PHY")
> Signed-off-by: Alexander Dahl <ada@thorsis.com>

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 2/2] net: phy: adin1100: Simplify register value passing

[Russell King (Oracle)]

On Wed, Nov 19, 2025 at 01:20:51PM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 19, 2025 at 01:47:37PM +0100, Alexander Dahl wrote:
> > The additional use case for that variable is gone,
> > the expression is simple enough to pass it inline now.
> 
> Looking at net-next, this patch is wrong. You remove "val" but the code
> after the context in this patch is:
> 
>         return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1, ADIN_CRSM_STAT, ret,
>                                          (ret & ADIN_CRSM_SFT_PD_RDY) == val,
>                                          1000, 30000, true);
> 
> which also references "val".
> 
> Note that this code is buggy as things stand. "val" will be zero (when
> en is false) or 1 (when en is true), whereas ADIN_CRSM_SFT_PD_RDY is 2.
> Thus, if en is true, then the condition can never be true.

Sorry, missed the first patch (the two patches arrived in reverse
order.) Please ignore this comment.

For the patch:

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 0/2] net: phy: adin1100: Fix powerdown mode setting

[Nuno Sá]

On Wed, 2025-11-19 at 13:47 +0100, Alexander Dahl wrote:
> Hei hei,
> 
> while building a new device around the ADIN1100 I noticed some errors in
> kernel log when calling `ifdown` on the ethernet device.  Series has a
> straight forward fix and an obvious follow-up code simplification.
> 
> Greets
> Alex
> 
> Alexander Dahl (2):
>   net: phy: adin1100: Fix software power-down ready condition
>   net: phy: adin1100: Simplify register value passing
> 
>  drivers/net/phy/adin1100.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> 
> base-commit: 6a23ae0a96a600d1d12557add110e0bb6e32730c

Acked-by: Nuno Sá <nuno.sa@analog.com>

Re: [PATCH 0/2] net: phy: adin1100: Fix powerdown mode setting

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 19 Nov 2025 13:47:35 +0100 you wrote:
> Hei hei,
> 
> while building a new device around the ADIN1100 I noticed some errors in
> kernel log when calling `ifdown` on the ethernet device.  Series has a
> straight forward fix and an obvious follow-up code simplification.
> 
> Greets
> Alex
> 
> [...]

Here is the summary with links:
  - [1/2] net: phy: adin1100: Fix software power-down ready condition
    https://git.kernel.org/netdev/net-next/c/bccaf1fe08f2
  - [2/2] net: phy: adin1100: Simplify register value passing
    https://git.kernel.org/netdev/net-next/c/5894cab4e1b9

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