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

[Andrew Lunn <andrew@lunn.ch>]

On Fri, Jul 31, 2026 at 06:14:50PM +0200, Birger Koblitz wrote:
> 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?

We have two bitmaps.

phydev->supported lists all the modes the device supports. Generally,
that is filled out during probe, when genphy_read_abilities() or
genphy_c45_pma_read_abilities() is called, which looks at register
values which indicate what the PHY actually supports. If those
registers are wrong, because it breaks the standard, you can provide a
.get_features() callback in the PHY driver. That typically calls
genphy_read_abilities() and/or genphy_c45_pma_read_abilities(), and
them fixes up what they have discovered to fit what the device really
does.

phylink will also mask phydev->supported with what the MAC actually
supports. So the PHY might indicate it supported 1G Half, but the MAC
does not, and so it will be removed.

phydev->supported is then copied into phydev->advertised. The user can
modify phydev->advertised, but phydev->supported is fixed.

When the link is configured up, the .config_aneg() driver method is
called. It needs to push the bits in phydev->advertised into the
hardware, so it actually advertises those link modes.

If the hardware is broken and puts the bits in the wrong place, you
cannot use the helper. So the drive needs to do the interpretation and
write the correct bits in the registers. Depending on how it is
broken, you might be able to call genphy_config_aneg() to do
10/100/1G, and then do 2.5G manually in the driver.

The PHY driver should consider phydev->advertised read only.

	Andrew