On Mon, 16 Feb 2026 18:22:03 +0530 Ashok Kumar Natarajan <[email protected]> wrote:
> Introduce helper functions to perform external PHY register read and > write operations. These helpers currently support only IEEE Clause 22 > PHY access, providing a simple and consistent API for accessing > standard 16‑bit MII registers on external PHY devices. > > This patch does not implement Clause 45 transactions; support for C45 > access can be added in future updates as needed. > > No functional changes are introduced for existing drivers. The new > helpers will be used by subsequent patches that require external PHY > management. > > Signed-off-by: Ashok Kumar Natarajan <[email protected]> > --- The result of AI review feedback reports some issues that need resolution. All AI feedback can be wrong, so use this is inspiration. Correctness issues (must fix): 1. axgbe_get_phy_id() uses the return value of read() directly in bit-shift arithmetic without checking for errors. If either MDIO read fails (returns negative), the resulting PHY ID is garbage and could accidentally match M88E1512_E_PHY_ID, triggering the wrong init sequence. Please add error checks after each read. 2. axgbe_phy_an_config() silently drops the return value of axgbe_m88e1512_config_aneg(). That function can fail at multiple points but the caller always returns 0. Should be: return axgbe_m88e1512_config_aneg(pdata); 3. axgbe_m88e1512_init() switches PHY pages multiple times but never restores page 0 on error paths. If a write fails while on page 0xFF or 0x12, all subsequent MDIO accesses from other code paths will hit the wrong registers. Add an error cleanup label that restores page 0. 4. The read/write helpers (patch 1) don't check the return value of the underlying read_ext_mii_regs_c22/write_ext_mii_regs_c22 calls. Errors from the hardware layer pass through silently. Other items: 5. Function name typo: axgbe_m88e5112_set_page should be axgbe_m88e1512_set_page to match the actual PHY model. 6. All three subject lines use uppercase "Add" after the colon — should be lowercase per DPDK convention. 7. Missing space: if(ret) should be if (ret) in the init caller in patch 2. 8. The init function blocks for 2 seconds total (two rte_delay_ms(1000) calls). Is polling for reset completion feasible instead? 9. Missing release notes for external PHY support and 100M speed — these are user-visible features. 10. Various magic numbers (0x8001, 0x1177, 0xC00D) written to registers without defines or comments referencing the datasheet. Please fix at least items 1-4 and resubmit.
