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;
> > + }
