Re: [PATCH net-next v1] lan78xx: lan7801 MAC support with lan8841

[Rengarajan.S]

Hi Jakub,

Apologies for the delay in reply. Thanks for reviewing the patch and
please find my comments inline.

On Wed, 2024-06-12 at 18:33 -0700, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Tue, 11 Jun 2024 15:12:33 +0530 Rengarajan S wrote:
> >  /* define external phy id */
> >  #define      PHY_LAN8835                     (0x0007C130)
> > +#define PHY_LAN8841                  (0x00221650)
> 
> For whatever reason the existing code uses a tab between define and
> its
> name, so let's stick to that?

Sure. Will address the change in the next patch revision.

> 
> >  #define      PHY_KSZ9031RNX                  (0x00221620)
> > 
> >  /* use ethtool to change the level for any given device */
> > @@ -2327,6 +2328,13 @@ static struct phy_device
> > *lan7801_phy_init(struct lan78xx_net *dev)
> >                       netdev_err(dev->net, "Failed to register
> > fixup for PHY_LAN8835\n");
> >                       return NULL;
> >               }
> > +             /* external PHY fixup for LAN8841 */
> > +             ret = phy_register_fixup_for_uid(PHY_LAN8841,
> > 0xfffffff0,
> > +                                              lan8835_fixup);
> > +             if (ret < 0) {
> > +                     netdev_err(dev->net, "Failed to register
> > fixup for PHY_LAN8841\n");
> 
> Don't you have to unregister the previous fixup on the error path
> here?
> In fact the existing error path for PHY_LAN8835 is missing an
> unregsiter
> for PHY_KSZ9031RNX.

There is a seperate register and unregister done for PHY_LAN8835 and
PHY_KSZ9031RNX. Also, if the ret < 0 the fixup is not registered and
there is no need to unregister it further. Can you please elaborate on
what is missing in case of unregistering PHY_KSZ9031RNX.

> 
> Could you please send a separate fix for that with a Fixes tag?
> 
> > +                     return NULL;
> > +             }