Re: [PATCH v9 4/4] net: phy: bcm-phy-lib: Implement BroadR-Reach link modes

[Andrew Lunn <andrew@lunn.ch>]

On Fri, Jun 21, 2024 at 01:26:33PM +0200, Kamil Horák (2N) wrote:
> Implement single-pair BroadR-Reach modes on bcm5481x PHY by Broadcom.
> Create set of functions alternative to IEEE 802.3 to handle
> configuration of these modes on compatible Broadcom PHYs.

What i've not seen anywhere is a link between BroadR-Reach and LRE.
Maybe you could explain the relationship here in the commit message?
And maybe also how LDS fits in.

> +int bcm_setup_master_slave(struct phy_device *phydev)

This is missing the lre in the name.

> +static int bcm54811_read_abilities(struct phy_device *phydev)
> +{
> +	int i, val, err;
> +	u8 brr_mode;
> +
> +	for (i = 0; i < ARRAY_SIZE(bcm54811_linkmodes); i++)
> +		linkmode_clear_bit(bcm54811_linkmodes[i], phydev->supported);

I think that needs a comment since it is not clear what is going on
here. What set these bits in supported?

> +
> +	err = bcm5481x_get_brrmode(phydev, &brr_mode);
> +	if (err)
> +		return err;
> +
> +	if (brr_mode) {

I would expect the DT property to be here somewhere. If the DT
property is present, set phydev->supported to only the BRR modes,
otherwise set it to the standard baseT modes. That should then allow
the core to do most of the validation. This is based on my
understanding the coupling hardware makes the two modes mutually
exclusive?

> +	/* With BCM54811, BroadR-Reach implies no autoneg */
> +	if (brr)
> +		phydev->autoneg = 0;

So long as phydev->supported does not indicate autoneg, this should
not happen.

	Andrew