[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0573C9D4B793EF43BF95221F2F4CC85153E4BD@CHN-SV-EXMX06.mchp-main.com>
Date: Thu, 26 Apr 2018 06:27:57 +0000
From: <RaghuramChary.Jallipalli@...rochip.com>
To: <f.fainelli@...il.com>, <davem@...emloft.net>
CC: <netdev@...r.kernel.org>, <UNGLinuxDriver@...rochip.com>,
<Woojung.Huh@...rochip.com>
Subject: RE: [PATCH v2 net-next] lan78xx: Lan7801 Support for Fixed PHY
Hi Florian,
> > v0->v1:
> > * Remove driver version #define
>
> This should be a separate patch of its own, more comment below.
>
OK. Patch should be for net, correct?
> > -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.
>
Done.
> > 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.
OK.
> > + 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.
>
OK. Will handle this too in separate patch/commit for net.
Thanks,
Raghu
Powered by blists - more mailing lists