Tom, Yes the intent is to allow overwrite the functionality elsewhere. I agree that the name is too generic and should be more unique to avoid name space conflicts.
Abbie, Please Review and update per Tom’s request. > On Sep 29, 2020, at 10:57 AM, Tom Rini <tr...@konsulko.com> wrote: > > On Wed, Sep 23, 2020 at 05:59:13PM -0700, Alex Nemirovsky wrote: >> From: Abbie Chang <abbie.ch...@cortina-access.com> >> >> Add phy driver support for MACs embedded inside Cortina Access SoCs >> >> Signed-off-by: Abbie Chang <abbie.ch...@cortina-access.com> >> Signed-off-by: Alex Nemirovsky <alex.nemirov...@cortina-access.com> >> >> CC: Joe Hershberger <joe.hershber...@ni.com> >> CC: Tom Rini <tr...@konsulko.com> >> CC: Aaron Tseng <aaron.ts...@cortina-access.com> >> >> Moved out PHY specific code out of Cortina NI Ethernet driver >> and into a Cortina Access PHY interface driver > [snip] >> +__weak void __internal_phy_init(struct phy_device *phydev, int reset_phy) >> +{ >> + u8 phy_addr; >> + u16 data; >> + >> + /* should initialize 4 GPHYs at once */ >> + for (phy_addr = 4; phy_addr > 0; phy_addr--) { >> + phydev->addr = phy_addr; >> + phy_write(phydev, MDIO_DEVAD_NONE, 31, 0x0BC6); >> + phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x0053); >> + phy_write(phydev, MDIO_DEVAD_NONE, 18, 0x4003); >> + phy_write(phydev, MDIO_DEVAD_NONE, 22, 0x7e01); >> + phy_write(phydev, MDIO_DEVAD_NONE, 31, 0x0A42); >> + phy_write(phydev, MDIO_DEVAD_NONE, 31, 0x0A40); >> + phy_write(phydev, MDIO_DEVAD_NONE, 0, 0x1140); >> + } >> + >> + /* workaround to fix GPHY fail */ >> + for (phy_addr = 1; phy_addr < 5; phy_addr++) { >> + /* Clear clock fail interrupt */ >> + phydev->addr = phy_addr; >> + phy_write(phydev, MDIO_DEVAD_NONE, 31, 0xB90); >> + data = phy_read(phydev, MDIO_DEVAD_NONE, 19); >> + if (data == 0x10) { >> + phy_write(phydev, MDIO_DEVAD_NONE, 31, 0xB90); >> + data = phy_read(phydev, MDIO_DEVAD_NONE, 19); >> + printf("%s: read again.\n", __func__); >> + } >> + >> + printf("%s: phy_addr=%d, read register 19, value=0x%x\n", >> + __func__, phy_addr, data); >> + } >> +} >> + >> +__weak void __external_phy_init(struct phy_device *phydev, int reset_phy) >> +{ >> + u16 val; >> + >> + /* Disable response PHYAD=0 function of RTL8211 series PHY */ >> + /* REG31 write 0x0007, set to extension page */ >> + phy_write(phydev, MDIO_DEVAD_NONE, 31, 0x0007); >> + >> + /* REG30 write 0x002C, set to extension page 44 */ >> + phy_write(phydev, MDIO_DEVAD_NONE, 30, 0x002C); >> + >> + /* >> + * REG27 write bit[2] = 0 disable response PHYAD = 0 function. >> + * we should read REG27 and clear bit[2], and write back >> + */ >> + val = phy_read(phydev, MDIO_DEVAD_NONE, 27); >> + val &= ~(1 << 2); >> + phy_write(phydev, MDIO_DEVAD_NONE, 27, val); >> + >> + /* REG31 write 0X0000, back to page0 */ >> + phy_write(phydev, MDIO_DEVAD_NONE, 31, 0x0000); >> +} >> + >> +static int rtl8211_external_config(struct phy_device *phydev) >> +{ >> + __external_phy_init(phydev, 0); >> + printf("%s: initialize RTL8211 external done.\n", __func__); >> + return 0; >> +} >> + >> +static int rtl8211_internal_config(struct phy_device *phydev) >> +{ >> + struct phy_device phydev_init; >> + >> + memcpy(&phydev_init, phydev, sizeof(struct phy_device)); >> + /* should initialize 4 GPHYs at once */ >> + __internal_phy_init(&phydev_init, 0); >> + printf("%s: initialize RTL8211 internal done.\n", __func__); >> + return 0; >> +} > > Why do we have two weak functions (which have very generic names and > could get overwritten by accident) and not just inlining them to the > functions? > > -- > Tom