Re: [PATCH net-next v8 3/7] net: usb: lan78xx: refactor PHY init to separate detection and MAC configuration

[<Thangaraj.S@microchip.com>]

Hi Oleksj.
Thanks for the patch.

On Mon, 2025-05-05 at 10:43 +0200, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Split out PHY detection into lan78xx_get_phy() and MAC-side setup
> into
> lan78xx_mac_prepare_for_phy(), making the main lan78xx_phy_init()
> cleaner
> and easier to follow.
> 
> This improves separation of concerns and prepares the code for a
> future
> transition to phylink. Fixed PHY registration and interface selection
> are now handled in lan78xx_get_phy(), while MAC-side delay
> configuration
> is done in lan78xx_mac_prepare_for_phy().
> 
> The fixed PHY fallback is preserved for setups like EVB-KSZ9897-1,
> where LAN7801 connects directly to a KSZ switch without a standard
> PHY
> or device tree support.
> 
> No functional changes intended.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> changes v6:
> - this patch is added in v6
> ---
>  drivers/net/usb/lan78xx.c | 174 ++++++++++++++++++++++++++++------
> ----
>  1 file changed, 128 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 9c0658227bde..7f1ecc415d53 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -2508,53 +2508,145 @@ static void lan78xx_remove_irq_domain(struct
> lan78xx_net *dev)
>         dev->domain_data.irqdomain = NULL;
>  }
> 
> 
>  static int lan78xx_phy_init(struct lan78xx_net *dev)
> @@ -2564,31 +2656,13 @@ static int lan78xx_phy_init(struct
> lan78xx_net *dev)
>         u32 mii_adv;
>         struct phy_device *phydev;
> 
> -       switch (dev->chipid) {
> -       case ID_REV_CHIP_ID_7801_:
> -               phydev = lan7801_phy_init(dev);
> -               if (IS_ERR(phydev)) {
> -                       netdev_err(dev->net, "lan7801: failed to init
> PHY: %pe\n",
> -                                  phydev);
> -                       return PTR_ERR(phydev);
> -               }
> -               break;
> -
> -       case ID_REV_CHIP_ID_7800_:
> -       case ID_REV_CHIP_ID_7850_:
> -               phydev = phy_find_first(dev->mdiobus);
> -               if (!phydev) {
> -                       netdev_err(dev->net, "no PHY found\n");
> -                       return -ENODEV;
> -               }
> -               phydev->is_internal = true;
> -               dev->interface = PHY_INTERFACE_MODE_GMII;
> -               break;
> +       phydev = lan78xx_get_phy(dev);
> +       if (IS_ERR(phydev))
> +               return PTR_ERR(phydev);
> 
> -       default:
> -               netdev_err(dev->net, "Unknown CHIP ID found\n");
> -               return -ENODEV;
> -       }
> +       ret = lan78xx_mac_prepare_for_phy(dev);
> +       if (ret < 0)
> +               goto free_phy;
> 
>         /* if phyirq is not set, use polling mode in phylib */
>         if (dev->domain_data.phyirq > 0)
> @@ -2662,6 +2736,14 @@ static int lan78xx_phy_init(struct lan78xx_net
> *dev)
>         dev->fc_autoneg = phydev->autoneg;
> 
>         return 0;
> +
> +free_phy:
> +       if (phy_is_pseudo_fixed_link(phydev)) {
> +               fixed_phy_unregister(phydev);
> +               phy_device_free(phydev);
> +       }
> +
> +       return ret;
>  }

Could see as per implementation, this case might hit on normal phy
other than fixed-phy too. Should we not add any cleanup for phydev
here?

Thanks,
Thangaraj Samynathan
> 
>  static int lan78xx_set_rx_max_frame_length(struct lan78xx_net *dev,
> int size)
> --
> 2.39.5
>