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
