Re: [PATCH net-next v3 1/7] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management

[Oleksij Rempel]

On Mon, Mar 17, 2025 at 03:36:11PM +0000, Simon Horman wrote:
> On Mon, Mar 10, 2025 at 12:57:31PM +0100, Oleksij Rempel wrote:
> 
> ...
> 
> > +static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
> > +{
> >  	struct phy_device *phydev;
> > +	int ret;
> 
> nit: not strictly related to this patch as the problem already existed,
>      but ret is set but otherwise unused in this function. Perhaps
>      it can be removed at some point?
> 
>      Flagged by W=1 builds.

Ack. This is addresses in 3. patch

> > +	u32 buf;
> >  
> >  	phydev = phy_find_first(dev->mdiobus);
> >  	if (!phydev) {
> > -		netdev_dbg(dev->net, "PHY Not Found!! Registering Fixed PHY\n");
> > -		phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
> > -		if (IS_ERR(phydev)) {
> > -			netdev_err(dev->net, "No PHY/fixed_PHY found\n");
> > -			return NULL;
> > -		}
> > -		netdev_dbg(dev->net, "Registered FIXED PHY\n");
> > -		dev->interface = PHY_INTERFACE_MODE_RGMII;
> > +		netdev_dbg(dev->net, "PHY Not Found!! Forcing RGMII configuration\n");
> >  		ret = lan78xx_write_reg(dev, MAC_RGMII_ID,
> >  					MAC_RGMII_ID_TXC_DELAY_EN_);
> >  		ret = lan78xx_write_reg(dev, RGMII_TX_BYP_DLL, 0x3D00);
> 
> ...
> 
> > @@ -4256,13 +4281,13 @@ static void lan78xx_delayedwork(struct work_struct *work)
> >  		}
> >  	}
> >  
> > -	if (test_bit(EVENT_LINK_RESET, &dev->flags)) {
> > +	if (test_bit(EVENT_PHY_INT_ACK, &dev->flags)) {
> >  		int ret = 0;
> >  
> > -		clear_bit(EVENT_LINK_RESET, &dev->flags);
> > -		if (lan78xx_link_reset(dev) < 0) {
> > -			netdev_info(dev->net, "link reset failed (%d)\n",
> > -				    ret);
> > +		clear_bit(EVENT_PHY_INT_ACK, &dev->flags);
> > +		if (lan78xx_phy_int_ack(dev) < 0) {
> > +			netdev_info(dev->net, "PHY INT ack failed (%pe)\n",
> > +				    ERR_PTR(ret));
> 
> nit: ret is always 0 here. So I'm unsure both why it is useful to include
>      in the error message, and why ERR_PTR() is being used.
> 
>      Flagged by Smatch.

Thank you, i'll take a look on it.

-- 
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 |