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

[Oleksij Rempel <o.rempel@pengutronix.de>]

Hi Thangaraj,

Thanks for the review!

On Tue, May 06, 2025 at 05:31:30AM +0000, Thangaraj.S@microchip.com wrote:
> 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?

You're right to ask — but in this case, we don't need to clean up
non-fixed PHYs, since we only probe them using phy_find_first(), and do
not allocate, register, or attach them in lan78xx_get_phy(). So no extra
cleanup is needed in the error path for those cases.

If we ever call phy_connect_direct() earlier in the flow, we would need
to add a corresponding phy_disconnect(), but that's not the case here
yet.

Best regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |