On 04/25/2018 12:07 PM, Raghuram Chary J wrote:
> Adding Fixed PHY support to the lan78xx driver.
>
> Signed-off-by: Raghuram Chary J
> ---
> v0->v1:
>* Remove driver version #define
This should be a separate patch of its own, more comment below.
>* Modify netdev_info to netdev_dbg
>* Move lan7801 specific to new routine and add switch case
>* Minor cleanup
> -static int lan78xx_phy_init(struct lan78xx_net *dev)
> +static int lan7801_phy_init(struct phy_device **phydev,
> + struct lan78xx_net *dev)
Passing a **phydev looks really unnecessary here, why don't just return
a struct phy_device * as a return argument here? If you need to
communicate a specific return value, you can use ERR_PTR()/PTR_ERR() for
that purpose.
> {
> + u32 buf;
> int ret;
> - u32 mii_adv;
> - struct phy_device *phydev;
> -
> - phydev = phy_find_first(dev->mdiobus);
> - if (!phydev) {
> - netdev_err(dev->net, "no PHY found\n");
> - return -EIO;
> - }
> -
> - if ((dev->chipid == ID_REV_CHIP_ID_7800_) ||
> - (dev->chipid == ID_REV_CHIP_ID_7850_)) {
> - phydev->is_internal = true;
> - dev->interface = PHY_INTERFACE_MODE_GMII;
> -
> - } else if (dev->chipid == ID_REV_CHIP_ID_7801_) {
> - if (!phydev->drv) {
> + struct fixed_phy_status fphy_status = {
> + .link = 1,
> + .speed = SPEED_1000,
> + .duplex = DUPLEX_FULL,
> + };
> +
> + if (!*phydev) {
> + netdev_dbg(dev->net, "PHY Not Found!! Registering Fixed PHY\n");
> + *phydev = fixed_phy_register(PHY_POLL, &fphy_status, -1,
> + NULL);
> + if (IS_ERR(*phydev)) {
> + netdev_err(dev->net, "No PHY/fixed_PHY found\n");
> + return -ENODEV;
> + }
> + netdev_dbg(dev->net, "Registered FIXED PHY\n");
> + dev->interface = PHY_INTERFACE_MODE_RGMII;
> + ret = lan78xx_write_reg(dev, MAC_RGMII_ID,
> + MAC_RGMII_ID_TXC_DELAY_EN_);
> + ret = lan78xx_write_reg(dev, RGMII_TX_BYP_DLL, 0x3D00);
> + ret = lan78xx_read_reg(dev, HW_CFG, &buf);
> + buf |= HW_CFG_CLK125_EN_;
> + buf |= HW_CFG_REFCLK25_EN_;
> + ret = lan78xx_write_reg(dev, HW_CFG, buf);
> + } else {
> + if (!(*phydev)->drv) {
> netdev_err(dev->net, "no PHY driver found\n");
> return -EIO;
> }
> -
> dev->interface = PHY_INTERFACE_MODE_RGMII;
> -
> /* external PHY fixup for KSZ9031RNX */
> ret = phy_register_fixup_for_uid(PHY_KSZ9031RNX, 0xfff0,
>ksz9031rnx_fixup);
> if (ret < 0) {
> - netdev_err(dev->net, "fail to register fixup\n");
> + netdev_err(dev->net, "fail to register fixup
> PHY_KSZ9031RNX\n");
Unrelated message change, that should be a separate commit.
> return ret;
> }
> /* external PHY fixup for LAN8835 */
> ret = phy_register_fixup_for_uid(PHY_LAN8835, 0xfff0,
>lan8835_fixup);
> if (ret < 0) {
> - netdev_err(dev->net, "fail to register fixup\n");
> + netdev_err(dev->net, "fail to register fixup for
> PHY_LAN8835\n");
Same here, can be grouped in the same commit as the one right above.
> return ret;
> }
> /* add more external PHY fixup here if needed */
>
> - phydev->is_internal = false;
> - } else {
> - netdev_err(dev->net, "unknown ID found\n");
> - ret = -EIO;
> - goto error;
> + (*phydev)->is_internal = false;
> + }
> + return 0;
> +}
> +
> +static int lan78xx_phy_init(struct lan78xx_net *dev)
> +{
> + int ret;
> + u32 mii_adv;
> + struct phy_device *phydev;
> +
> + phydev = phy_find_first(dev->mdiobus);
> + switch (dev->chipid) {
> + case ID_REV_CHIP_ID_7801_:
> + ret = lan7801_phy_init(&phydev, dev);
> + if (ret < 0) {
> + netdev_err(dev->net, "lan7801: PHY Init Failed");
> + return ret;
> + }
> + break;
> +
> + case ID_REV_CHIP_ID_7800_:
> + case ID_REV_CHIP_ID_7850_:
> + if (!phydev) {
> + netdev_err(dev->net, "no PHY found\n");
> + return -EIO;
> + }
> + phydev->is_internal = true;
Uggh you are not really supposed to set that, the matching PHY driver
should have PHY_IS_INTERNAL in the .flags member to indicate that to the
core PHY library.
> + dev->interf
