Re: [PATCH net-next v3 13/13] ax88796b: Add support for AX88772D, AX88179A and AX88279

[Birger Koblitz]

On 24/07/2026 19:18, Andrew Lunn wrote:
> > +static int asix_ax88279_read_status(struct phy_device *phydev)
> > +{
>
> > +	val = phy_read(phydev, MII_ADVERTISE);
> > +	if (val < 0)
> > +		return val;
> > +
> > +	linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> > +			 phydev->advertising, val & AX_ADVERTISE_2500);
>
> This looks wrong. A PHY driver should configure the hardware to
> advertise what is in phydev->advertising. The PHY driver itself should
> never change this value, especially in _read_status(). It is up to
> user space to decide on what is advertised, and the phylib core will
> sanitise the value to ensure it is within the PHYs capabilities.
This looks wrong, because the PHY uses the wrong bit for configuring
advertising 2500_FD. Since phylink does not know about this behaviour
the PHY driver needs to decipher this additional bit and then set it
in advertising based on the value in the PHY register. But maybe
I miss something?

> > +
> > +	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
> > +				     MDIO_AN_10GBT_CTRL_ADV2_5G, adv);
>
> Can genphy_c45_an_config_aneg() be used here?
I tried and it did not work. 2500MBit was always advertised. The
same code is also used in en8811h_config_aneg(), which has some
of the same quirks as the AX88279-PHY.

> > +static int asix_ax88279_get_features(struct phy_device *phydev)
> > +{
> > +	int ret;
> > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = {};
> > +
> > +	/* MDIO_DEVS1/2 empty, so set mmds_present bits to allow reading abilities */
> > +	phydev->c45_ids.mmds_present |= MDIO_DEVS_PMAPMD | MDIO_DEVS_AN;
>
> I would do this in probe. If you do this early, phylib might be able
> to call genphy_c45_pma_read_abilities() on its own. Maybe, i don't
> know.
 left it there. Putting it into probe did not make phylib do anything
magically on its own. When left by itself in probe() there is
an additional function needed and here it is where one understands what
it is needed for.

> > +	/* AX88279 does not support reported 100baseT-half duplex mode */
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, mask);
> > +	linkmode_andnot(phydev->supported, phydev->supported, mask);
>
> If you are only changing one bit, it is simpler to use
> linkmode_set_bit()/linkmode_clear_bit()
Fixed in v4.

Birger