+static int an8801r_did_interrupt(struct phy_device *phydev)
+{
+	u32 irq_en, irq_status;
+	int ret;
+
+	ret = air_buckpbus_reg_read(phydev, AN8801_BPBUS_REG_WAKE_IRQ_EN,
+				    &irq_en);
+	if (ret)
+		return ret;
+
+	ret = air_buckpbus_reg_read(phydev, AN8801_BPBUS_REG_WAKE_IRQ_STS,
+				    &irq_status);
+	if (ret)
+		return ret;
+
+	if (irq_status & AN8801_IRQ_WAKE_MAGICPKT)
+		return 0;

With a name like an8801r_did_interrupt() you would expect the return
value to be some value of True, if there was an interrupt. I would
suggest either a different name, or return 1. Maybe also add a
kerneldoc header indicating the return values, since it is probably
not going to be standard.

+static void an8801r_get_wol(struct phy_device *phydev,
+			    struct ethtool_wolinfo *wol)
+{
+	u32 reg_val;
+
+	air_buckpbus_reg_read(phydev, AN8801_BPBUS_REG_WAKEUP_CTL1, &reg_val);
+
+	wol->supported = WAKE_MAGIC;

How does WoL work on this device. Only via interrupts? If so, maybe
you should only return WAKE_MAGIC as supported if there is a valid
interrupt?

+static int an8801r_rgmii_delay_config(struct phy_device *phydev)
+{
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		return an8801r_rgmii_txdelay(phydev, 4);
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		return an8801r_rgmii_rxdelay(phydev, 0);
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		return an8801r_rgmii_txdelay(phydev, 4);
+		return an8801r_rgmii_rxdelay(phydev, 0);

The parameters look very odd here. 4 means 2ns, but 0 also means 0ns?
Can this API be improved?

Also, PHY_INTERFACE_MODE_RGMII_TXID means 2ns delay for TX, but it
also means 0ns delay for RX. The code appears to be missing this
second part.

+	case PHY_INTERFACE_MODE_RGMII:

And here you should be disabling all delays. We have seen boards where
the strapping is wrong, the PHY boots in RGMII_ID, but RGMII is
required, and so the driver must fully implement
PHY_INTERFACE_MODE_RGMII disabling the delays.

+static int an8801r_config_init(struct phy_device *phydev)
+{
+	u8 led_default_function[AN8801R_NUM_LEDS] = { 0 };
+	int prev_page, ret;
+
+	ret = an8801r_of_init_leds(phydev, led_default_function);
+	if (ret)
+		return ret;
+
+	/* Disable Low Power Mode (LPM) */
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, AN8801_REG_PHY_INTERNAL0,
+			    FIELD_PREP(AN8801_PHY_INTFUNC_MASK, 0x1e));
+	if (ret)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, AN8801_REG_PHY_INTERNAL1,
+			    FIELD_PREP(AN8801_PHY_INTFUNC_MASK, 0x2));
+	if (ret)
+		return ret;
+
+	/* Disable EEE by default */
+	ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, 0);
+	if (ret)
+		return ret;

Why? If EEE is broken, this is not sufficient to stop a user
re-enabling it.

+static int an8801r_read_status(struct phy_device *phydev)
+{
+	int prev_speed, ret;
+	u32 val;
+
+	prev_speed = phydev->speed;
+
+	ret = genphy_read_status(phydev);
+	if (ret)
+		return ret;

You configure the PHY to support downshift. If it has performed a
downshift, does it report the actual speed in the usual registers read
by genphy_read_status(), or is it necessary to read a vendor register?

+static struct phy_driver airoha_driver[] = {
+{
+	PHY_ID_MATCH_MODEL(AN8801R_PHY_ID),
+	.name			= "Airoha AN8801R",
+	.features		= PHY_GBIT_FEATURES,

Should not be needed, if the PHY enumerates its capabilities
correctly.

    Andrew

---
pw-bot: cr