Re: [PATCH net-next] net: phy: microchip_t1: Adds support for LAN887x phy

[Russell King (Oracle)]

On Thu, Aug 08, 2024 at 08:29:16PM +0530, Divya Koppera wrote:
> +static int lan887x_config_init(struct phy_device *phydev)
> +{
> +	/* Disable pause frames */
> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->supported);
> +	/* Disable asym pause */
> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->supported);

Why is this here? Pause frames are just like normal ethernet frames,
they only have meaning to the MAC, not to the PHY.

In any case, by the time the config_init() method has been called,
the higher levels have already looked at phydev->supported and made
decisions on what's there.

> +static int lan887x_config_aneg(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* First patch only supports 100Mbps and 1000Mbps force-mode.
> +	 * T1 Auto-Negotiation (Clause 98 of IEEE 802.3) will be added later.
> +	 */
> +	if (phydev->autoneg != AUTONEG_DISABLE) {
> +		/* PHY state is inconsistent due to ANEG Enable set
> +		 * so we need to assign ANEG Disable for consistent behavior
> +		 */
> +		phydev->autoneg = AUTONEG_DISABLE;

If you clear phydev->supported's autoneg bit, then phylib ought to
enforce this for you. Please check this rather than adding code to
drivers.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!