Create reusable helpers in preparation for the AN8811HB support.
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
drivers/net/phy/air_en8811h.c | 123 +++++++++++++++++++++-------------
1 file changed, 77 insertions(+), 46 deletions(-)
diff --git a/drivers/net/phy/air_en8811h.c b/drivers/net/phy/air_en8811h.c
index e890bb2c0aa8..e617108fe469 100644
--- a/drivers/net/phy/air_en8811h.c
+++ b/drivers/net/phy/air_en8811h.c
@@ -448,6 +448,11 @@ static int en8811h_wait_mcu_ready(struct phy_device *phydev)
{
int ret, reg_value;
+ ret = air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
+ EN8811H_FW_CTRL_1_FINISH);
+ if (ret)
+ return ret;
+
/* Because of mdio-lock, may have to wait for multiple loads */
ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1,
EN8811H_PHY_FW_STATUS, reg_value,
@@ -461,9 +466,18 @@ static int en8811h_wait_mcu_ready(struct phy_device *phydev)
return 0;
}
-static int en8811h_load_firmware(struct phy_device *phydev)
+static void en8811h_print_fw_version(struct phy_device *phydev)
{
struct en8811h_priv *priv = phydev->priv;
+
+ air_buckpbus_reg_read(phydev, EN8811H_FW_VERSION,
+ &priv->firmware_version);
+ phydev_info(phydev, "MD32 firmware version: %08x\n",
+ priv->firmware_version);
+}
+
+static int en8811h_load_firmware(struct phy_device *phydev)
+{
struct device *dev = &phydev->mdio.dev;
const struct firmware *fw1, *fw2;
int ret;
@@ -500,17 +514,11 @@ static int en8811h_load_firmware(struct phy_device *phydev)
if (ret < 0)
goto en8811h_load_firmware_out;
- ret = air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
- EN8811H_FW_CTRL_1_FINISH);
+ ret = en8811h_wait_mcu_ready(phydev);
if (ret < 0)
goto en8811h_load_firmware_out;
- ret = en8811h_wait_mcu_ready(phydev);
-
- air_buckpbus_reg_read(phydev, EN8811H_FW_VERSION,
- &priv->firmware_version);
- phydev_info(phydev, "MD32 firmware version: %08x\n",
- priv->firmware_version);
+ en8811h_print_fw_version(phydev);
en8811h_load_firmware_out:
release_firmware(fw2);
@@ -533,11 +541,6 @@ static int en8811h_restart_mcu(struct phy_device *phydev)
if (ret < 0)
return ret;
- ret = air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
- EN8811H_FW_CTRL_1_FINISH);
- if (ret < 0)
- return ret;
-
return en8811h_wait_mcu_ready(phydev);
}
@@ -919,6 +922,23 @@ static int en8811h_clk_provider_setup(struct device *dev, struct clk_hw *hw)
return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
}
+static int en8811h_leds_setup(struct phy_device *phydev)
+{
+ struct en8811h_priv *priv = phydev->priv;
+ int ret;
+
+ priv->led[0].rules = AIR_DEFAULT_TRIGGER_LED0;
+ priv->led[1].rules = AIR_DEFAULT_TRIGGER_LED1;
+ priv->led[2].rules = AIR_DEFAULT_TRIGGER_LED2;
+
+ ret = air_leds_init(phydev, EN8811H_LED_COUNT, AIR_PHY_LED_DUR,
+ AIR_LED_MODE_DISABLE);
+ if (ret < 0)
+ phydev_err(phydev, "Failed to disable leds: %d\n", ret);
+
+ return ret;
+}
+
static int en8811h_probe(struct phy_device *phydev)
{
struct en8811h_priv *priv;
@@ -937,19 +957,12 @@ static int en8811h_probe(struct phy_device *phydev)
/* mcu has just restarted after firmware load */
priv->mcu_needs_restart = false;
- priv->led[0].rules = AIR_DEFAULT_TRIGGER_LED0;
- priv->led[1].rules = AIR_DEFAULT_TRIGGER_LED1;
- priv->led[2].rules = AIR_DEFAULT_TRIGGER_LED2;
-
/* MDIO_DEVS1/2 empty, so set mmds_present bits here */
phydev->c45_ids.mmds_present |= MDIO_DEVS_PMAPMD | MDIO_DEVS_AN;
- ret = air_leds_init(phydev, EN8811H_LED_COUNT, AIR_PHY_LED_DUR,
- AIR_LED_MODE_DISABLE);
- if (ret < 0) {
- phydev_err(phydev, "Failed to disable leds: %d\n", ret);
+ ret = en8811h_leds_setup(phydev);
+ if (ret < 0)
return ret;
- }
priv->phydev = phydev;
/* Co-Clock Output */
@@ -1090,11 +1103,9 @@ static int en8811h_config_aneg(struct phy_device *phydev)
return __genphy_config_aneg(phydev, changed);
}
-static int en8811h_read_status(struct phy_device *phydev)
+static int en8811h_get_lpa(struct phy_device *phydev)
{
- struct en8811h_priv *priv = phydev->priv;
- u32 pbus_value;
- int ret, val;
+ int ret;
ret = genphy_update_link(phydev);
if (ret)
@@ -1112,23 +1123,12 @@ static int en8811h_read_status(struct phy_device *phydev)
if (ret < 0)
return ret;
- ret = genphy_read_lpa(phydev);
- if (ret < 0)
- return ret;
-
- /* Get link partner 2.5GBASE-T ability from vendor register */
- ret = air_buckpbus_reg_read(phydev, EN8811H_2P5G_LPA, &pbus_value);
- if (ret < 0)
- return ret;
- linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
- phydev->lp_advertising,
- pbus_value & EN8811H_2P5G_LPA_2P5G);
-
- if (phydev->autoneg_complete)
- phy_resolve_aneg_pause(phydev);
+ return genphy_read_lpa(phydev);
+}
- if (!phydev->link)
- return 0;
+static int en8811h_get_speed(struct phy_device *phydev)
+{
+ int val;
/* Get real speed from vendor register */
val = phy_read(phydev, AIR_AUX_CTRL_STATUS);
@@ -1146,6 +1146,40 @@ static int en8811h_read_status(struct phy_device *phydev)
break;
}
+ /* Only supports full duplex */
+ phydev->duplex = DUPLEX_FULL;
+
+ return 0;
+}
+
+static int en8811h_read_status(struct phy_device *phydev)
+{
+ struct en8811h_priv *priv = phydev->priv;
+ u32 pbus_value;
+ int ret;
+
+ ret = en8811h_get_lpa(phydev);
+ if (ret < 0)
+ return ret;
+
+ /* Get link partner 2.5GBASE-T ability from vendor register */
+ ret = air_buckpbus_reg_read(phydev, EN8811H_2P5G_LPA, &pbus_value);
+ if (ret < 0)
+ return ret;
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
+ phydev->lp_advertising,
+ pbus_value & EN8811H_2P5G_LPA_2P5G);
+
+ if (phydev->autoneg_complete)
+ phy_resolve_aneg_pause(phydev);
+
+ if (!phydev->link)
+ return 0;
+
+ ret = en8811h_get_speed(phydev);
+ if (ret < 0)
+ return ret;
+
/* Firmware before version 24011202 has no vendor register 2P5G_LPA.
* Assume link partner advertised it if connected at 2500Mbps.
*/
@@ -1155,9 +1189,6 @@ static int en8811h_read_status(struct phy_device *phydev)
phydev->speed == SPEED_2500);
}
- /* Only supports full duplex */
- phydev->duplex = DUPLEX_FULL;
-
return 0;
}
--
2.47.3
> -static int en8811h_read_status(struct phy_device *phydev)
> +static int en8811h_get_lpa(struct phy_device *phydev)
> {
> - struct en8811h_priv *priv = phydev->priv;
> - u32 pbus_value;
> - int ret, val;
> + int ret;
>
> ret = genphy_update_link(phydev);
> if (ret)
This call to genphy_update_link() means this function is doing more
than en8811h_get_lpa() would imply.
It is hard to see from just the patch, but it also seems to set the
state in phydev back to unknown defaults, and read the master/slave
status?
So i think it needs a better name.
Andrew
Andrew Lunn <andrew@lunn.ch> writes:
>> -static int en8811h_read_status(struct phy_device *phydev)
>> +static int en8811h_get_lpa(struct phy_device *phydev)
>> {
>> - struct en8811h_priv *priv = phydev->priv;
>> - u32 pbus_value;
>> - int ret, val;
>> + int ret;
>>
>> ret = genphy_update_link(phydev);
>> if (ret)
>
> This call to genphy_update_link() means this function is doing more
> than en8811h_get_lpa() would imply.
>
> It is hard to see from just the patch, but it also seems to set the
> state in phydev back to unknown defaults, and read the master/slave
> status?
>
> So i think it needs a better name.
Yes, that was my feeling too. Just couldn't think of any. So I took the
lazy route. You wouldn't happen to have a suggestion?
OK, I know it's my job. I'll try to come up with something better.
Bjørn
On Sat, Jan 24, 2026 at 05:55:05PM +0100, Bjørn Mork wrote:
> Andrew Lunn <andrew@lunn.ch> writes:
>
> >> -static int en8811h_read_status(struct phy_device *phydev)
> >> +static int en8811h_get_lpa(struct phy_device *phydev)
> >> {
> >> - struct en8811h_priv *priv = phydev->priv;
> >> - u32 pbus_value;
> >> - int ret, val;
> >> + int ret;
> >>
> >> ret = genphy_update_link(phydev);
> >> if (ret)
> >
> > This call to genphy_update_link() means this function is doing more
> > than en8811h_get_lpa() would imply.
> >
> > It is hard to see from just the patch, but it also seems to set the
> > state in phydev back to unknown defaults, and read the master/slave
> > status?
> >
> > So i think it needs a better name.
>
> Yes, that was my feeling too. Just couldn't think of any. So I took the
> lazy route. You wouldn't happen to have a suggestion?
Maybe a different strategy. Most of read_status is generic, it appears
just reading lpa is specific to the device. So have a generic
read_status() function, and then use phy_id_compare() to call the
needed device specific function.
Andrew
Andrew Lunn <andrew@lunn.ch> writes: > Maybe a different strategy. Most of read_status is generic, it appears > just reading lpa is specific to the device. So have a generic > read_status() function, and then use phy_id_compare() to call the > needed device specific function. Yes, maybe that's better. My immediate feeling was that this contradicted your comments on the RFC, but after rereading it I see that it doesn't. I was reading too much into that I'll try and see if I can improve the readability of read_status() with a generic function. Bjørn
On Sun, Jan 25, 2026 at 11:51:56AM +0100, Bjørn Mork wrote: > Andrew Lunn <andrew@lunn.ch> writes: > > > Maybe a different strategy. Most of read_status is generic, it appears > > just reading lpa is specific to the device. So have a generic > > read_status() function, and then use phy_id_compare() to call the > > needed device specific function. > > Yes, maybe that's better. My immediate feeling was that this > contradicted your comments on the RFC Reviewers sometimes get things wrong. It is not always possible to see everything from just the patch, and then jump to the wrong conclusion, etc. The review process should be a conversation. We can talk about different solutions, which might contradict earlier suggestions, as we get a better understanding of the problem to be solved. Andrew
© 2016 - 2026 Red Hat, Inc.