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

[Andrew Lunn]

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

> +static int asix_ax88279_config_aneg(struct phy_device *phydev)
> +{
> +	bool changed = false;
> +	int ret;
> +	u32 adv;
> +
> +	if (phydev->autoneg == AUTONEG_DISABLE) {
> +		phydev_warn(phydev, "Disabling autoneg is not supported\n");
> +		return -EINVAL;
> +	}
> +
> +	adv = linkmode_adv_to_mii_10gbt_adv_t(phydev->advertising);
> +
> +	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?

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

> +	/* 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()

	Andrew