Re: [PATCH 7/9] net: phy: use linkmode operation return values in phy_device.c

[Yury Norov]

On Tue, Sep 08, 2026 at 03:52:09PM +0200, Andrew Lunn wrote:
> > -	linkmode_and(sfp_support, port->supported, caps->link_modes);
> > -	if (linkmode_empty(sfp_support)) {
> > +	if (!linkmode_and(sfp_support, port->supported, caps->link_modes)) {
> >  		dev_err(&phydev->mdio.dev, "incompatible SFP module inserted, no common linkmode\n");
> 
> From a readability perspective, i like linkmode_empty(). It is more
> obvious than !linkmode_and().

OK, a function returning 2 values is not something obvious in C. What
about this?

        
	sfp_supported = linkmode_and(sfp_support, port->supported, caps->link_modes);
        if (!sfp_supported)
		dev_err(&phydev->mdio.dev, "incompatible SFP module inserted, no common linkmode\n");

> None of this code is in the hot path. So we should put readability
> above performance. 
> 
> >  	/* Some PHYs may advertise, by default, not support EEE modes. So,
> >  	 * we need to clean them. In addition remove all disabled EEE modes.
> >  	 */
> > -	linkmode_and(phydev->advertising_eee, phydev->supported_eee,
> > -		     phydev->advertising_eee);
> > -	linkmode_andnot(phydev->advertising_eee, phydev->advertising_eee,
> > -			phydev->eee_disabled_modes);
> > -
> >  	/* There is no "enabled" flag. If PHY is advertising, assume it is
> >  	 * kind of enabled.
> >  	 */
> > -	phydev->eee_cfg.eee_enabled = !linkmode_empty(phydev->advertising_eee);
> > +	phydev->eee_cfg.eee_enabled =
> > +		linkmode_and_andnot(phydev->advertising_eee,
> > +				    phydev->advertising_eee,
> > +				    phydev->supported_eee,
> > +				    phydev->eee_disabled_modes);
> 
> So, which is more readable, the original or this?

The new version is more readable to me. What about this:
        
        eee_advertised = linkmode_and_andnot(phydev->advertising_eee,
	                                     phydev->advertising_eee,
                                             phydev->supported_eee,
		                             phydev->eee_disabled_modes);

  	/* There is no "enabled" flag. If PHY is advertising, assume it is
  	 * kind of enabled.
  	 */
	phydev->eee_cfg.eee_enabled = eee_advertised;

Thanks,
Yury