[{"id":3682369,"web_url":"http://patchwork.ozlabs.org/comment/3682369/","msgid":"<8e72978d-f8ef-492d-8efb-508612ea702e@baylibre.com>","list_archive_url":null,"date":"2026-04-23T15:13:56","subject":"Re: [PATCH 6/9] net: phy: Add airoha AN8801 ethernet phy driver","submitter":{"id":87228,"url":"http://patchwork.ozlabs.org/api/people/87228/","name":"David Lechner","email":"dlechner@baylibre.com"},"content":"On 4/23/26 8:25 AM, Julien Stephan wrote:\n> Add Airoha AN8801 Ethernet PHY driver (air_an8801.c).\n> Implement CL22/CL45 MDIO access, LED control, and RGMII delay\n> configuration. Provide probe, initialization, LED setup, and status\n> handling. Expose DTS properties for clock delays. Register driver with\n> PHY framework and trigger on startup. Improve robustness and error\n> reporting across platforms now.\n\nI don't think this \"Improve robustness and error reporting across\nplatforms now.\" since this is all new code. I would remove this\nsentence.\n\n> \n> Signed-off-by: Yanqing Wang \n> Signed-off-by: Julien Stephan \n> ---\n> MAINTAINERS | 1 +\n> drivers/net/phy/airoha/Kconfig | 7 +\n> drivers/net/phy/airoha/Makefile | 1 +\n> drivers/net/phy/airoha/air_an8801.c | 614 ++++++++++++++++++++++++++++++++++++\n> 4 files changed, 623 insertions(+)\n> \n> diff --git a/MAINTAINERS b/MAINTAINERS\n> index 4bda6b9e8c4..a59342de3e7 100644\n> --- a/MAINTAINERS\n> +++ b/MAINTAINERS\n> @@ -60,6 +60,7 @@ F:\tlib/acpi/\n> \n> AIROHA PHY\n> M:\tTommy Shih \n> +M:\tKevin-KW Huang \n> S:\tMaintained\n> F:\tdrivers/net/phy/airoha/\n> \n> diff --git a/drivers/net/phy/airoha/Kconfig b/drivers/net/phy/airoha/Kconfig\n> index 2a2f4f05bb2..cbceb0cfdb4 100644\n> --- a/drivers/net/phy/airoha/Kconfig\n> +++ b/drivers/net/phy/airoha/Kconfig\n> @@ -2,6 +2,13 @@\n> menuconfig PHY_AIROHA\n> \tbool \"Airoha Ethernet PHYs support\"\n> \n> +config PHY_AIROHA_AN8801\n> +\tbool \"Airoha Ethernet AN8801 support\"\n> +\tdepends on PHY_AIROHA\n> +\tselect PHY_AIRONA_PHYLIB\n> +\thelp\n> +\t Currently support AIROHA AN8801 1G PHY.\n> +\n> config PHY_AIROHA_EN8811\n> \tbool \"Airoha Ethernet EN8811H support\"\n> \tdepends on PHY_AIROHA\n> diff --git a/drivers/net/phy/airoha/Makefile b/drivers/net/phy/airoha/Makefile\n> index cbf123986f2..3144f1dc54f 100644\n> --- a/drivers/net/phy/airoha/Makefile\n> +++ b/drivers/net/phy/airoha/Makefile\n> @@ -1,4 +1,5 @@\n> # SPDX-License-Identifier: GPL-2.0\n> \n> +obj-$(CONFIG_PHY_AIROHA_AN8801) += air_an8801.o\n> obj-$(CONFIG_PHY_AIROHA_EN8811) += air_en8811.o\n> obj-$(CONFIG_PHY_AIRONA_PHYLIB) += air_phy_lib.o\n> diff --git a/drivers/net/phy/airoha/air_an8801.c b/drivers/net/phy/airoha/air_an8801.c\n> new file mode 100644\n> index 00000000000..53657babf57\n> --- /dev/null\n> +++ b/drivers/net/phy/airoha/air_an8801.c\n> @@ -0,0 +1,614 @@\n> +// SPDX-License-Identifier: GPL-2.0\n\nPrefer explicit GPL-2.0-only or GPL-2.0-or-later.\n\n> +/*\n> + * air_an8801.c - PHY driver for Airoha AN8801.\n> + * Copyright (c) 2026 Airoha Technology Corp.\n> + * Copyright (C) 2026 BayLibre, SAS.\n> + * Author: Kevin-KW Huang \n> + * Sita Huang \n> + * Julien Stephan \n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"air_phy_lib.h\"\n> +\n> +#define AN8801R_PHY_ID1\t\t\t0xc0ff\n> +#define AN8801R_PHY_ID2\t\t\t0x0421\n> +#define AN8801R_PHY_ID\t\t\t((u32)((AN8801R_PHY_ID1 << 16) | AN8801R_PHY_ID2))\n> +\n> +#define LINK_UP\t\t\t\t1\n> +#define LINK_DOWN\t\t\t0\n\nSome of these macro names are quite generic. Would be better to always\nuse AN8801R_ prefix to avoid problems later.\n\n> +\n> +#define MAX_LED_SIZE\t\t\t3\n> +\n> +#define MAX_RETRY\t\t\t5\n> +\n> +#define LED_ENABLE\t\t\t1\n> +#define LED_DISABLE\t\t\t0\n> +\n> +/* MII Registers - Airoha Page 4 */\n> +#define AN8801_PBUS_ACCESS\t\tBIT(28)\n> +\n> +/* BPBUS Registers */\n> +#define AN8801_BPBUS_REG_LED_GPIO\t0x54\n> +#define AN8801_BPBUS_REG_LED_ID_SEL\t0x58\n> +#define LED_ID_GPIO_SEL(led, gpio)\t((led) << ((gpio) * 3))\n> +\n> +#define AN8801_BPBUS_REG_GPIO_MODE\t0x70\n> +\n> +#define AN8801_BPBUS_REG_LINK_MODE\t0x5054\n> +#define AN8801_BPBUS_LINK_MODE_1000\tBIT(0)\n> +\n> +#define AN8801_BPBUS_REG_BYPASS_PTP\t0x21c004\n> +#define AN8801_BYP_PTP_RGMII_TO_GPHY\tBIT(0)\n> +\n> +#define AN8801_BPBUS_REG_TXDLY_STEP\t0x21c024\n> +#define RGMII_DELAY_STEP_MASK\t\tGENMASK(2, 0)\n> +#define AIR_RGMII_DELAY_NOSTEP\t\t0\n> +#define AIR_RGMII_DELAY_STEP_1\t\t1\n> +#define AIR_RGMII_DELAY_STEP_2\t\t2\n> +#define AIR_RGMII_DELAY_STEP_3\t\t3\n> +#define AIR_RGMII_DELAY_STEP_4\t\t4\n> +#define AIR_RGMII_DELAY_STEP_5\t\t5\n> +#define AIR_RGMII_DELAY_STEP_6\t\t6\n> +#define AIR_RGMII_DELAY_STEP_7\t\t7\n\nnit: Macros that map a number to the same number don't add much.\n\n> +#define RGMII_TXDELAY_FORCE_MODE\tBIT(24)\n> +\n> +#define AN8801_BPBUS_REG_RXDLY_STEP\t0x21c02c\n> +#define RGMII_RXDELAY_ALIGN\t\tBIT(4)\n> +#define RGMII_RXDELAY_FORCE_MODE\tBIT(24)\n> +\n> +#define AN8801_BPBUS_REG_EFIFO_CTL(x)\t(0x270004 + (0x100 * (x))) /* 0..2 */\n> +#define AN8801_EFIFO_ALL_EN\t\tGENMASK(7, 0)\n> +#define AN8801_EFIFO_RX_EN\t\tBIT(0)\n> +#define AN8801_EFIFO_TX_EN\t\tBIT(1)\n> +#define AN8801_EFIFO_RX_CLK_EN\tBIT(2)\n> +#define AN8801_EFIFO_TX_CLK_EN\tBIT(3)\n> +#define AN8801_EFIFO_RX_EEE_EN\tBIT(4)\n> +#define AN8801_EFIFO_TX_EEE_EN\tBIT(5)\n> +#define AN8801_EFIFO_RX_ODD_NIBBLE_EN\tBIT(6)\n> +#define AN8801_EFIFO_TX_ODD_NIBBLE_EN\tBIT(7)\n> +\n> +#define AN8801_BPBUS_REG_HWRST_DE_GLITCH\t0xc8\n> +#define AN8801_DE_GLITCH_EN\t\t\tBIT(2)\n> +#define AN8801_11_CYCLE_XTAL_PERIOD_DE_GLITCH\tGENMASK(1, 0)\n> +\n> +#define LED_BCR\t\t\t\t0x21\n> +#define LED_BCR_MODE_MASK\t\tGENMASK(1, 0)\n> +#define LED_BCR_TIME_TEST\t\tBIT(2)\n> +#define LED_BCR_CLK_EN\t\t\tBIT(3)\n> +#define LED_BCR_EVT_ALL\t\tBIT(4)\n> +#define LED_BCR_EXT_CTRL\t\tBIT(15)\n> +#define LED_BCR_MODE_DISABLE\t\t0\n> +#define LED_BCR_MODE_2LED\t\t1\n> +#define LED_BCR_MODE_3LED_1\t\t2\n> +#define LED_BCR_MODE_3LED_2\t\t3\n> +\n> +#define LED_ON_DUR\t\t\t0x22\n> +#define LED_ON_DUR_MASK\t\t\tGENMASK(15, 0)\n> +\n> +#define LED_BLINK_DUR\t\t\t0x23\n> +#define LED_BLINK_DUR_MASK\t\tGENMASK(15, 0)\n> +\n> +#define LED_ON_CTRL(i)\t\t\t(0x024 + ((i) * 2))\n> +#define LED_ON_EVT_MASK\t\tGENMASK(6, 0)\n> +#define LED_ON_EVT_LINK_1000M\t\tBIT(0)\n> +#define LED_ON_EVT_LINK_100M\t\tBIT(1)\n> +#define LED_ON_EVT_LINK_10M\t\tBIT(2)\n> +#define LED_ON_EVT_LINK_DN\t\tBIT(3)\n> +#define LED_ON_EVT_FDX\t\t\tBIT(4)\n> +#define LED_ON_EVT_HDX\t\t\tBIT(5)\n> +#define LED_ON_EVT_FORCE\t\tBIT(6)\n> +#define LED_ON_POL\t\t\tBIT(14)\n> +#define LED_ON_EN\t\t\tBIT(15)\n> +\n> +#define LED_BLINK_CTRL(i)\t\t(0x025 + ((i) * 2))\n> +#define LED_BLINK_EVT_MASK\t\tGENMASK(9, 0)\n> +#define LED_BLINK_EVT_1000M_TX\t\tBIT(0)\n> +#define LED_BLINK_EVT_1000M_RX\t\tBIT(1)\n> +#define LED_BLINK_EVT_100M_TX\t\tBIT(2)\n> +#define LED_BLINK_EVT_100M_RX\t\tBIT(3)\n> +#define LED_BLINK_EVT_10M_TX\t\tBIT(4)\n> +#define LED_BLINK_EVT_10M_RX\t\tBIT(5)\n> +#define LED_BLINK_EVT_FORCE\t\tBIT(9)\n> +\n> +#define UNIT_LED_BLINK_DURATION\t\t780\n> +#define LED_BLINK_DURATION(f)\t\t(UNIT_LED_BLINK_DURATION << (f))\n> +\n> +/* Link on(1G/100M/10M), no activity */\n> +#define AIR_LED0_ON \\\n> +\t(LED_ON_EVT_LINK_1000M | LED_ON_EVT_LINK_100M | LED_ON_EVT_LINK_10M)\n> +#define AIR_LED0_BLINK\t\t\t0x0\n> +/* No link on, activity(1G/100M/10M TX/RX) */\n> +#define AIR_LED1_ON\t\t\t0x0\n> +#define AIR_LED1_BLINK \\\n> +\t(LED_BLINK_EVT_1000M_TX | LED_BLINK_EVT_1000M_RX | \\\n> +\tLED_BLINK_EVT_100M_TX | LED_BLINK_EVT_100M_RX | \\\n> +\tLED_BLINK_EVT_10M_TX | LED_BLINK_EVT_10M_RX)\n> +/* Link on(100M/10M), activity(100M/10M TX/RX) */\n> +#define AIR_LED2_ON \\\n> +\t(LED_ON_EVT_LINK_100M | LED_ON_EVT_LINK_10M)\n> +#define AIR_LED2_BLINK \\\n> +\t(LED_BLINK_EVT_100M_TX | LED_BLINK_EVT_100M_RX | \\\n> +\tLED_BLINK_EVT_10M_TX | LED_BLINK_EVT_10M_RX)\n> +\n> +/* Invalid data */\n> +#define INVALID_DATA\t\t\t0xffffffff\n\nGENMASK(31, 0)?\n\n> +\n> +#define AN8801_REG_PHY_INTERNAL0\t0x600\n> +#define AN8801_REG_PHY_INTERNAL1\t0x601\n> +\n> +enum AIR_LED_GPIO_PIN_T {\n\nCode style is lower_case_identifer without _t suffix.\n\n> +\tAIR_LED_GPIO1 = 1,\n> +\tAIR_LED_GPIO2,\n> +\tAIR_LED_GPIO3\n> +};\n> +\n> +enum AIR_LED_T {\n> +\tAIR_LED0 = 0,\n> +\tAIR_LED1,\n> +\tAIR_LED2,\n> +\tAIR_LED3\n> +};\n\nThese two above are more \"obvious\" enums that map a number to\nthe same number.\n\n> +\n> +enum AIR_LED_BLINK_DUT_T {\n> +\tAIR_LED_BLINK_DUR_32M = 0,\n> +\tAIR_LED_BLINK_DUR_64M,\n> +\tAIR_LED_BLINK_DUR_128M,\n> +\tAIR_LED_BLINK_DUR_256M,\n> +\tAIR_LED_BLINK_DUR_512M,\n> +\tAIR_LED_BLINK_DUR_1024M,\n> +\tAIR_LED_BLINK_DUR_LAST\n> +};\n> +\n> +enum AIR_LED_POLARITY {\n> +\tAIR_ACTIVE_LOW = 0,\n> +\tAIR_ACTIVE_HIGH,\n> +};\n> +\n> +enum AIR_LED_MODE_T {\n> +\tAIR_LED_MODE_DISABLE = 0,\n> +\tAIR_LED_MODE_USER_DEFINE,\n> +\tAIR_LED_MODE_LAST\n> +};\n> +\n> +struct air_led_cfg_t {\n\nWe don't usually have a _t suffix on type names.\n\n> +\tu16 en;\n> +\tu16 gpio;\n> +\tu16 pol;\n> +\tu16 on_cfg;\n> +\tu16 blk_cfg;\n> +};\n> +\n> +struct an8801r_priv {\n> +\tstruct air_led_cfg_t\tled_cfg[MAX_LED_SIZE];\n> +\tu32\t\t\tled_blink_cfg;\n> +\tu8\t\t\trxdelay_force;\n> +\tu8\t\t\ttxdelay_force;\n> +\tu16\t\t\trxdelay_step;\n> +\tu8\t\t\trxdelay_align;\n> +\tu16\t\t\ttxdelay_step;\n> +};\n> +\n> +#define phydev_cfg(phy)\t\t((struct an8801r_priv *)(phy)->priv)\n> +\n> +/*\n> + * For reference only\n\nNot sure what this comment means.\n\n> + *\tGPIO1 <-> LED0,\n> + *\tGPIO2 <-> LED1,\n> + *\tGPIO3 <-> LED2,\n> + */\n> +/* User-defined.B */\n\nWhat is User-defined.B?\n\n> +static const struct air_led_cfg_t led_cfg_dlt[MAX_LED_SIZE] = {\n> +\t/* LED Enable, GPIO, LED Polarity, LED ON, LED Blink */\n> +\t/* LED0 */\n\nComments here are all reduandant. We can deduce it from the code.\n\n> +\t{LED_ENABLE, AIR_LED_GPIO1, AIR_ACTIVE_LOW, AIR_LED0_ON, AIR_LED0_BLINK},\n> +\t/* LED1 */\n> +\t{LED_ENABLE, AIR_LED_GPIO2, AIR_ACTIVE_HIGH, AIR_LED1_ON, AIR_LED1_BLINK},\n> +\t/* LED2 */\n> +\t{LED_ENABLE, AIR_LED_GPIO3, AIR_ACTIVE_HIGH, AIR_LED2_ON, AIR_LED2_BLINK},\n> +};\n> +\n> +static const u16 led_blink_cfg_dlt = AIR_LED_BLINK_DUR_64M;\n> +/* RGMII delay */\n> +static const u8 rxdelay_force;\n> +static const u8 txdelay_force;\n> +static const u16 rxdelay_step = AIR_RGMII_DELAY_NOSTEP;\n> +static const u8 rxdelay_align;\n> +static const u16 txdelay_step = AIR_RGMII_DELAY_NOSTEP;\n\nI guess these were module parameters in Linux? Can we just drop them\nand use the default values directly when initilzing during probe?\n\nOr make a static const struct an8801r_priv an8801r_priv_defaults = {...};\ninstead and use it like `*priv = *an8801r_priv_defaults` in the probe\nfunction.\n\n> +\n> +static int an8801_buckpbus_reg_rmw(struct phy_device *phydev,\n> +\t\t\t\t u32 addr, u32 mask, u32 set)\n> +{\n> +\treturn air_phy_buckpbus_reg_modify(phydev,\n> +\t\t\t\t\t addr | AN8801_PBUS_ACCESS,\n> +\t\t\t\t\t mask, set);\n> +}\n> +\n> +static int an8801_buckpbus_reg_set_bits(struct phy_device *phydev,\n> +\t\t\t\t\tu32 addr, u32 mask)\n> +{\n> +\treturn air_phy_buckpbus_reg_modify(phydev,\n> +\t\t\t\t\t addr | AN8801_PBUS_ACCESS,\n> +\t\t\t\t\t mask, mask);\n> +}\n> +\n> +static int an8801_buckpbus_reg_clear_bits(struct phy_device *phydev,\n> +\t\t\t\t\t u32 addr, u32 mask)\n> +{\n> +\treturn air_phy_buckpbus_reg_modify(phydev,\n> +\t\t\t\t\t addr | AN8801_PBUS_ACCESS,\n> +\t\t\t\t\t mask, 0);\n> +}\n> +\n> +static int an8801_buckpbus_reg_write(struct phy_device *phydev, u32 addr, u32 data)\n> +{\n> +\treturn air_phy_buckpbus_reg_write(phydev, addr | AN8801_PBUS_ACCESS, data);\n> +}\n> +\n> +static int an8801r_led_set_usr_def(struct phy_device *phydev, u8 entity,\n> +\t\t\t\t u16 polar, u16 on_evt, u16 blk_evt)\n> +{\n> +\tint ret;\n> +\n> +\tif (polar == AIR_ACTIVE_HIGH)\n> +\t\ton_evt |= LED_ON_POL;\n> +\telse\n> +\t\ton_evt &= ~LED_ON_POL;\n> +\n> +\ton_evt |= LED_ON_EN;\n> +\n> +\tret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LED_ON_CTRL(entity), on_evt);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\treturn phy_write_mmd(phydev, MDIO_MMD_VEND2, LED_BLINK_CTRL(entity), blk_evt);\n> +}\n> +\n> +static int an8801r_led_set_blink(struct phy_device *phydev, u16 blink)\n> +{\n> +\tint ret;\n> +\n> +\tret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LED_BLINK_DUR,\n> +\t\t\t LED_BLINK_DURATION(blink));\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\treturn phy_write_mmd(phydev, MDIO_MMD_VEND2, LED_ON_DUR,\n> +\t\t\t LED_BLINK_DURATION(blink) >> 1);\n\n`>> 1` would be better written as `/ 2` (compiler will optimize it). I\nassume this is some sort of 50% duty cycle?\n\n> +}\n> +\n> +static int an8801r_led_set_mode(struct phy_device *phydev, u8 mode)\n> +{\n> +\tint ret;\n> +\n> +\tret = phy_read_mmd(phydev, MDIO_MMD_VEND2, LED_BCR);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tswitch (mode) {\n> +\tcase AIR_LED_MODE_DISABLE:\n> +\t\tret &= ~LED_BCR_EXT_CTRL;\n> +\t\tret &= ~LED_BCR_MODE_MASK;\n> +\t\tret |= LED_BCR_MODE_DISABLE;\n> +\t\tbreak;\n> +\tcase AIR_LED_MODE_USER_DEFINE:\n> +\t\tret |= LED_BCR_EXT_CTRL | LED_BCR_CLK_EN;\n> +\t\tbreak;\n> +\t}\n> +\treturn phy_write_mmd(phydev, MDIO_MMD_VEND2, LED_BCR, ret);\n> +}\n> +\n> +static int an8801r_led_set_state(struct phy_device *phydev, u8 entity, u8 state)\n> +{\n> +\tint ret;\n> +\n> +\tret = phy_read_mmd(phydev, MDIO_MMD_VEND2, LED_ON_CTRL(entity));\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tif (state)\n> +\t\tret |= LED_ON_EN;\n> +\telse\n> +\t\tret &= ~LED_ON_EN;\n> +\n> +\treturn phy_write_mmd(phydev, MDIO_MMD_VEND2, LED_ON_CTRL(entity), ret);\n> +}\n> +\n> +static int an8801r_led_init(struct phy_device *phydev)\n> +{\n> +\tstruct an8801r_priv *priv = phydev_cfg(phydev);\n> +\tstruct air_led_cfg_t *led_cfg = priv->led_cfg;\n> +\tu16 led_blink_cfg = priv->led_blink_cfg;\n> +\tint ret, led_id;\n> +\n> +\tret = an8801r_led_set_blink(phydev, led_blink_cfg);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = an8801r_led_set_mode(phydev, AIR_LED_MODE_USER_DEFINE);\n> +\tif (ret) {\n> +\t\tdev_err(phydev->dev, \"AN8801R: Fail to set LED mode, ret %d!\\n\", ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tfor (led_id = AIR_LED0; led_id < MAX_LED_SIZE; led_id++) {\n> +\t\tret = an8801r_led_set_state(phydev, led_id, led_cfg[led_id].en);\n> +\t\tif (ret) {\n> +\t\t\tdev_err(phydev->dev, \"AN8801R: Fail to set LED%d state, ret %d!\\n\",\n> +\t\t\t\tled_id, ret);\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\tif (led_cfg[led_id].en != LED_ENABLE)\n> +\t\t\tcontinue;\n> +\n> +\t\tret = an8801_buckpbus_reg_set_bits(phydev, AN8801_BPBUS_REG_LED_GPIO,\n> +\t\t\t\t\t\t BIT(led_cfg[led_id].gpio));\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\tret = an8801_buckpbus_reg_set_bits(phydev, AN8801_BPBUS_REG_LED_ID_SEL,\n> +\t\t\t\t\t\t LED_ID_GPIO_SEL(led_id,\n> +\t\t\t\t\t\t\t\t led_cfg[led_id].gpio));\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\tret = an8801_buckpbus_reg_clear_bits(phydev, AN8801_BPBUS_REG_GPIO_MODE,\n> +\t\t\t\t\t\t BIT(led_cfg[led_id].gpio));\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\tret = an8801r_led_set_usr_def(phydev, led_id,\n> +\t\t\t\t\t led_cfg[led_id].pol,\n> +\t\t\t\t\t led_cfg[led_id].on_cfg,\n> +\t\t\t\t\t led_cfg[led_id].blk_cfg);\n> +\t\tif (ret) {\n> +\t\t\tdev_err(phydev->dev, \"AN8801R: Fail to set LED%d, ret %d!\\n\",\n> +\t\t\t\tled_id, ret);\n> +\t\t\treturn ret;\n> +\t\t}\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> +static int an8801r_of_init(struct phy_device *phydev)\n> +{\n> +\tstruct an8801r_priv *priv = phydev_cfg(phydev);\n> +\tofnode node = phy_get_ofnode(phydev);\n> +\tu32 val = 0;\n> +\n> +\tif (!ofnode_valid(node))\n> +\t\treturn -EINVAL;\n> +\n> +\tif (ofnode_get_property(node, \"airoha,rxclk-delay\", NULL)) {\n\nofnode_has_property()?\n\n> +\t\tif (ofnode_read_u32(node, \"airoha,rxclk-delay\", &val)) {\n> +\t\t\tdev_err(phydev->dev, \"airoha,rxclk-delay value is invalid.\\n\");\n> +\t\t\treturn -EINVAL;\n\nWhy not pass actual return value?\n\n> +\t\t}\n> +\t\tif (val > AIR_RGMII_DELAY_STEP_7) {\n> +\t\t\tdev_err(phydev->dev, \"airoha,rxclk-delay value %u out of range.\\n\", val);\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t\tpriv->rxdelay_force = true;\n> +\t\tpriv->rxdelay_step = val;\n> +\t\tpriv->rxdelay_align = ofnode_read_bool(node,\n> +\t\t\t\t\t\t \"airoha,rxclk-delay-align\");\n> +\t}\n> +\n\nSame two comments apply below.\n\n> +\tif (ofnode_get_property(node, \"airoha,txclk-delay\", NULL)) {\n> +\t\tif (ofnode_read_u32(node, \"airoha,txclk-delay\", &val)) {\n> +\t\t\tdev_err(phydev->dev, \"airoha,txclk-delay value is invalid.\\n\");\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t\tif (val > AIR_RGMII_DELAY_STEP_7) {\n> +\t\t\tdev_err(phydev->dev, \"airoha,txclk-delay value %u out of range.\\n\", val);\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\t\tpriv->txdelay_force = true;\n> +\t\tpriv->txdelay_step = val;\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> +static int an8801r_rgmii_rxdelay(struct phy_device *phydev, u16 delay, u8 align)\n> +{\n> +\tu32 reg_val = delay & RGMII_DELAY_STEP_MASK;\n> +\tint ret;\n> +\n> +\tif (align) {\n> +\t\treg_val |= RGMII_RXDELAY_ALIGN;\n> +\t\tdebug(\"AN8801R: Rxdelay align\\n\");\n> +\t}\n> +\treg_val |= RGMII_RXDELAY_FORCE_MODE;\n> +\tret = an8801_buckpbus_reg_write(phydev, AN8801_BPBUS_REG_RXDLY_STEP, reg_val);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tdebug(\"AN8801R: Force rxdelay = %d(0x%x)\\n\", delay, reg_val);\n> +\treturn 0;\n> +}\n> +\n> +static int an8801r_rgmii_txdelay(struct phy_device *phydev, u16 delay)\n> +{\n> +\tu32 reg_val = delay & RGMII_DELAY_STEP_MASK;\n> +\tint ret;\n> +\n> +\treg_val |= RGMII_TXDELAY_FORCE_MODE;\n> +\tret = an8801_buckpbus_reg_write(phydev, AN8801_BPBUS_REG_TXDLY_STEP, reg_val);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tdebug(\"AN8801R: Force txdelay = %d(0x%x)\\n\", delay, reg_val);\n> +\treturn 0;\n> +}\n> +\n> +static int an8801r_rgmii_delay_config(struct phy_device *phydev)\n> +{\n> +\tstruct an8801r_priv *priv = phydev_cfg(phydev);\n> +\tint ret = 0;\n> +\n> +\tswitch (phydev->interface) {\n> +\tcase PHY_INTERFACE_MODE_RGMII_TXID:\n> +\t\tret = an8801r_rgmii_txdelay(phydev, AIR_RGMII_DELAY_STEP_4);\n> +\t\tbreak;\n\nCan return early here and in other cases.\n\n> +\tcase PHY_INTERFACE_MODE_RGMII_RXID:\n> +\t\tret = an8801r_rgmii_rxdelay(phydev, AIR_RGMII_DELAY_NOSTEP, true);\n> +\t\tbreak;\n> +\tcase PHY_INTERFACE_MODE_RGMII_ID:\n> +\t\tret = an8801r_rgmii_txdelay(phydev, AIR_RGMII_DELAY_STEP_4);\n> +\t\tif (ret)\n> +\t\t\tbreak;\n> +\t\tret = an8801r_rgmii_rxdelay(phydev, AIR_RGMII_DELAY_NOSTEP, true);\n> +\t\tbreak;\n> +\tcase PHY_INTERFACE_MODE_RGMII:\n> +\tdefault:\n> +\t\tif (priv->rxdelay_force) {\n> +\t\t\tret = an8801r_rgmii_rxdelay(phydev, priv->rxdelay_step,\n> +\t\t\t\t\t\t priv->rxdelay_align);\n> +\t\t\tif (ret)\n> +\t\t\t\tbreak;\n> +\t\t}\n> +\t\tif (priv->txdelay_force)\n> +\t\t\tret = an8801r_rgmii_txdelay(phydev, priv->txdelay_step);\n> +\t\tbreak;\n> +\t}\n> +\treturn ret;\n> +}\n> +\n> +static int an8801r_config_init(struct phy_device *phydev)\n> +{\n> +\tint ret;\n> +\n> +\tret = an8801r_of_init(phydev);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tret = an8801_buckpbus_reg_write(phydev, AN8801_BPBUS_REG_HWRST_DE_GLITCH,\n> +\t\t\t\t\tAN8801_DE_GLITCH_EN |\n> +\t\t\t\t\tAN8801_11_CYCLE_XTAL_PERIOD_DE_GLITCH);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = phy_write_mmd(phydev, MDIO_MMD_VEND2, AN8801_REG_PHY_INTERNAL0, 0x1e);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = phy_write_mmd(phydev, MDIO_MMD_VEND2, AN8801_REG_PHY_INTERNAL1, 0x02);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, 0x0);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = an8801_buckpbus_reg_write(phydev, AN8801_BPBUS_REG_BYPASS_PTP,\n> +\t\t\t\t\tAN8801_BYP_PTP_RGMII_TO_GPHY);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = an8801_buckpbus_reg_write(phydev, AN8801_BPBUS_REG_EFIFO_CTL(0),\n> +\t\t\t\t\tAN8801_EFIFO_RX_EN |\n> +\t\t\t\t\tAN8801_EFIFO_TX_EN |\n> +\t\t\t\t\tAN8801_EFIFO_RX_CLK_EN |\n> +\t\t\t\t\tAN8801_EFIFO_TX_CLK_EN |\n> +\t\t\t\t\tAN8801_EFIFO_RX_EEE_EN |\n> +\t\t\t\t\tAN8801_EFIFO_TX_EEE_EN);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = an8801_buckpbus_reg_write(phydev, AN8801_BPBUS_REG_EFIFO_CTL(1),\n> +\t\t\t\t\tAN8801_EFIFO_ALL_EN);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = an8801_buckpbus_reg_write(phydev, AN8801_BPBUS_REG_EFIFO_CTL(2),\n> +\t\t\t\t\tAN8801_EFIFO_ALL_EN);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = an8801r_rgmii_delay_config(phydev);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = an8801r_led_init(phydev);\n> +\tif (ret) {\n> +\t\tdev_err(phydev->dev, \"AN8801R: LED initialize fail, ret %d!\\n\", ret);\n> +\t\treturn ret;\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> +static int an8801r_phy_probe(struct phy_device *phydev)\n> +{\n> +\tstruct an8801r_priv *priv;\n> +\tu32 phy_id, led_id;\n> +\tint ret;\n> +\n> +\tret = get_phy_id(phydev->bus, phydev->addr, MDIO_DEVAD_NONE, &phy_id);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tif (phy_id != AN8801R_PHY_ID) {\n> +\t\tdev_err(phydev->dev,\n> +\t\t\t\"AN8801R can't be detected (id=0x%08x).\\n\", phy_id);\n> +\t\treturn -ENODEV;\n> +\t}\n> +\n> +\tpriv = calloc(1, sizeof(struct an8801r_priv));\n> +\tif (!priv)\n> +\t\treturn -ENOMEM;\n> +\n\n> +\tfor (led_id = AIR_LED0; led_id < MAX_LED_SIZE; led_id++)\n> +\t\tpriv->led_cfg[led_id] = led_cfg_dlt[led_id];\n> +\n> +\tpriv->led_blink_cfg = led_blink_cfg_dlt;\n> +\tpriv->rxdelay_force = rxdelay_force;\n> +\tpriv->txdelay_force = txdelay_force;\n> +\tpriv->rxdelay_step = rxdelay_step;\n> +\tpriv->rxdelay_align = rxdelay_align;\n> +\tpriv->txdelay_step = txdelay_step;\n\nAs in a previous comment, all of this could be:\n\n\t*priv = *an8801r_priv_default;\n\n> +\n> +\tphydev->priv = priv;\n> +\treturn 0;\n> +}\n> +\n> +static int an8801r_read_status(struct phy_device *phydev)\n> +{\n> +\tu32 data;\n> +\n> +\tif (phydev->link != LINK_UP)\n> +\t\treturn 0;\n> +\n> +\tdebug(\"AN8801R: SPEED %d\\n\", phydev->speed);\n> +\tdata = phydev->speed == SPEED_1000 ? AN8801_BPBUS_LINK_MODE_1000 : 0;\n> +\n> +\treturn an8801_buckpbus_reg_rmw(phydev, AN8801_BPBUS_REG_LINK_MODE,\n> +\t\t\t\t AN8801_BPBUS_LINK_MODE_1000, data);\n> +}\n> +\n> +static int an8801r_startup(struct phy_device *phydev)\n> +{\n> +\tint ret;\n> +\n> +\tret = genphy_startup(phydev);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\treturn an8801r_read_status(phydev);\n> +}\n> +\n> +U_BOOT_PHY_DRIVER(an8801r) = {\n> +\t.name = \"Airoha AN8801R\",\n> +\t.uid = AN8801R_PHY_ID,\n> +\t.mask = 0x0ffffff0,\n> +\t.features = PHY_GBIT_FEATURES,\n> +\t.probe = &an8801r_phy_probe,\n> +\t.config = &an8801r_config_init,\n> +\t.read_page = &air_phy_read_page,\n> +\t.write_page = &air_phy_write_page,\n> +\t.startup = &an8801r_startup,\n> +\t.shutdown = &genphy_shutdown,\n> +};\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=baylibre-com.20251104.gappssmtp.com\n header.i=@baylibre-com.20251104.gappssmtp.com header.a=rsa-sha256\n header.s=20251104 header.b=uEOXBwjE;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=lists.denx.de\n (client-ip=85.214.62.61; helo=phobos.denx.de;\n envelope-from=u-boot-bounces@lists.denx.de; receiver=patchwork.ozlabs.org)","phobos.denx.de;\n dmarc=none (p=none dis=none) header.from=baylibre.com","phobos.denx.de;\n spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de","phobos.denx.de;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=baylibre-com.20251104.gappssmtp.com\n header.i=@baylibre-com.20251104.gappssmtp.com header.b=\"uEOXBwjE\";\n\tdkim-atps=neutral","phobos.denx.de;\n dmarc=none (p=none dis=none) header.from=baylibre.com","phobos.denx.de;\n spf=pass smtp.mailfrom=dlechner@baylibre.com"],"Received":["from phobos.denx.de (phobos.denx.de [85.214.62.61])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange x25519 server-signature ECDSA (secp384r1) server-digest SHA384)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4g34QF1l4Nz1yHv\n\tfor ; Sun, 26 Apr 2026 08:34:45 +1000 (AEST)","from h2850616.stratoserver.net (localhost [IPv6:::1])\n\tby phobos.denx.de (Postfix) with ESMTP id B65498426C;\n\tSun, 26 Apr 2026 00:34:32 +0200 (CEST)","by phobos.denx.de (Postfix, from userid 109)\n id 5A1CF8437C; Sat, 25 Apr 2026 23:27:05 +0200 (CEST)","from mail-dl1-x122e.google.com (mail-dl1-x122e.google.com\n [IPv6:2607:f8b0:4864:20::122e])\n (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits))\n (No client certificate requested)\n by phobos.denx.de (Postfix) with ESMTPS id B58AC84376\n for ; Sat, 25 Apr 2026 23:27:01 +0200 (CEST)","by mail-dl1-x122e.google.com with SMTP id\n a92af1059eb24-12c565dd3a7so2242671c88.1\n for ; Sat, 25 Apr 2026 14:27:01 -0700 (PDT)","from ?IPV6:2600:8803:e7e4:500:3cc7:689f:be44:d58a?\n ([2600:8803:e7e4:500:3cc7:689f:be44:d58a])\n by smtp.gmail.com with ESMTPSA id\n 5614622812f47-4799fc19273sm13372874b6e.0.2026.04.23.08.13.56\n (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n Thu, 23 Apr 2026 08:13:58 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.2 (2018-09-13) on phobos.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,\n DKIM_VALID,RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_PASS autolearn=ham\n autolearn_force=no version=3.4.2","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=baylibre-com.20251104.gappssmtp.com; s=20251104; t=1777152420;\n x=1777757220;\n darn=lists.denx.de;\n h=content-transfer-encoding:in-reply-to:from:content-language\n :references:cc:to:subject:user-agent:mime-version:date:message-id\n :from:to:cc:subject:date:message-id:reply-to;\n bh=Qdpo6CwOygkvdsF6FuUpmzKB4NES3gnoiqOkFNI53lI=;\n b=uEOXBwjEg3R9b/Dxnj85bxepiVpg28hDK3+vVRvrUEncbcDO0L/TKN28Rwnq6XYDEt\n QTW/lHpBwe5ET66z2Qv55jtxYgATc1gYRISn7YFdmC/eZBLBGM1egDs/vUiS4rJW7lZS\n d7zXhsziJr0e0BL6OrnMQm90bN7C1ZLpf13YdNbKIPjDZhrEmdXTqolZvgTGLwW7GSg+\n wCmfE1cbMduLdSE3n/sNrq1OQNwaKvZZHFjdikK6Nr3SoZslZkJ9BHvvL8eXkyDCn5Sy\n tZpXHvFdhpvPIs0w5z13vRqAJmx9nlyP/P3Ot752VSdqg1te6s4tjgfNxla2ZouWo83B\n TBdQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=1e100.net; s=20251104; t=1777152420; x=1777757220;\n h=content-transfer-encoding:in-reply-to:from:content-language\n :references:cc:to:subject:user-agent:mime-version:date:message-id\n :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id\n :reply-to;\n bh=Qdpo6CwOygkvdsF6FuUpmzKB4NES3gnoiqOkFNI53lI=;\n b=H40OrN+PsHDVQe0Q7tE4dYDQzdmrTK5PV2zu8O69wePamLtTnrfYNHm29vRwgmed2f\n /cgXKX8VNkjPlrtZlPJinGZQHRUjiti9wL2UWjWOinXVCsS3a8CuX0+qzrL2lVUq1+wX\n ismR1MVv3sXTsUMNqy0H+ahQRYd9TFuHGmvZHygWhpAv1N3JtEpWeeAklMw8sqdaY6VE\n H6wRJCURJvj6wOBCM60Qlh6D3OTOCRRfKUj+7h9hM83tDVLENP512Mtl3FWM9Y3Rebr5\n JwFgIZVbQ63Jew8mNRFZkwofS58A6UNHPiPQ0kHfSM1uYWuN/PyxrifqKbdO/rmXSi/a\n Gmcg==","X-Forwarded-Encrypted":"i=1;\n AFNElJ9BDWScRtnNMlcKmQ+SmbOFfo0+Is94fu8BfGU0z6Y4RZ1BSIDhkBFHRDfT2QxLhXE+oVtwV2Y=@lists.denx.de","X-Gm-Message-State":"AOJu0Yw+86UvJ2uM47pa2tzttlh40zfeHLTk1yF7nMiWVsFcyMKFGE+t\n 89i5IjUwAV5Sn++8dGcyTLKUhF9uYNqH57hwPb9XiM/KPQUMwsfV4tlPNQ8HPQeU5FpKqQHRtXd\n iKa8A","X-Gm-Gg":"AeBDievE+Jf8CaGpmvF1es+JI2evQreqUnn+AXYI5xJo2eLLvbTGOBjJR6edNEqmF5o\n y66LblcCkJj78DyIHNoFOep+nOteZox3eUn5TpciW16kLi4utwFlXBCD/NBphvfiUQWpx16zk6Z\n Sp3lv4hWCsBwoi/DY5KtnHrh9qUTtxNdKd1Bq4yZ9XRSlcSCh6BrpZd+2+7UecF2otTtTBMccND\n qoyMzM3m4w6vWqnOFLnCsB9GGCn1l7uVYePehatKG5mKsCFrBx/NzA+j6E3Xko8I13lJlBGwNJe\n I3q/TZnw7dvPNDbk0p5Dwie7qMcMrHSgCqySqpbN4Hpn0pfjWeLYxwvCRXum5L1vPbwRypbmI0e\n n+14zBdUih5GmBL2Chy/O3ubYMpE+3oUm9dZv5R+QRqfulG+/T2slXuSwH3n4FQ2lY7eqWg7+mZ\n OJoPg+4maGRAb62FlaXjjnuFyEvAQweOsIh1UJWBrMh4b05DMQ+Nu8QAyjXjdyEHjponig8BKrJ\n 2BBVVu/2MJno1P+Ne3bROU=","X-Received":"by 2002:a05:6808:d4c:b0:479:72dc:bb6 with SMTP id\n 5614622812f47-4799ca7ed84mr15086021b6e.44.1776957238782;\n Thu, 23 Apr 2026 08:13:58 -0700 (PDT)","Message-ID":"<8e72978d-f8ef-492d-8efb-508612ea702e@baylibre.com>","Date":"Thu, 23 Apr 2026 10:13:56 -0500","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 6/9] net: phy: Add airoha AN8801 ethernet phy driver","To":"Julien Stephan , u-boot@lists.denx.de","Cc":"GSS_MTK_Uboot_upstream ,\n Jerome Forissier , Tom Rini ,\n Christian Marangi ,\n Robert Marko , Simon Glass ,\n Yao Zi , Quentin Schulz ,\n Mikhail Kshevetskiy ,\n Marek Vasut ,\n \"Lucien.Jheng\" , Weijie Gao\n , Romain Gantois ,\n Siddharth Vadapalli ,\n Yanqing Wang , Beiyan Yun ,\n Ryder Lee , Chunfeng Yun\n ,\n Igor Belwon ,\n Neil Armstrong ,\n Kory Maincent ,\n Ilias Apalodimas ,\n Kuan-Wei Chiu , Raymond Mao\n , Peng Fan ,\n Stefan Roese ,\n Philip Molloy ,\n fanyi zhang , Jonas Karlman ,\n Kever Yang , Marek Vasut\n , Patrick Delaunay ,\n Heiko Stuebner , Samuel Holland\n ,\n Christophe Roullier ,\n Patrice Chotard ,\n Chris-QJ Chen ,\n Macpaul Lin ,\n Sam Protsenko ,\n Michael Trimarchi ,\n Sky Huang ,\n Leo Yu-Chi Liang , Tommy Shih\n , Kevin-KW Huang ","References":"\n <20260423-add-ethernet-support-for-genio-520-720-v1-0-47ca918e0017@baylibre.com>\n <20260423-add-ethernet-support-for-genio-520-720-v1-6-47ca918e0017@baylibre.com>","Content-Language":"en-US","From":"David Lechner ","In-Reply-To":"\n <20260423-add-ethernet-support-for-genio-520-720-v1-6-47ca918e0017@baylibre.com>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","X-Mailman-Approved-At":"Sun, 26 Apr 2026 00:34:30 +0200","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.39","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n ","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n ","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" ","X-Virus-Scanned":"clamav-milter 0.103.8 at phobos.denx.de","X-Virus-Status":"Clean"}}]