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

[Andrew Lunn <andrew@lunn.ch>]

On Wed, Jun 05, 2024 at 11:56:46AM +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.
> 
> Change-Id: I592d261bc0d60aaa78fc1717a315b0b1c1449c81
> Signed-off-by: Kamil Horák - 2N <kamilh@axis.com>
> ---
>  drivers/net/phy/bcm-phy-lib.c | 123 ++++++++++++
>  drivers/net/phy/bcm-phy-lib.h |   4 +
>  drivers/net/phy/broadcom.c    | 368 ++++++++++++++++++++++++++++++++--
>  drivers/net/phy/phy-core.c    |   2 +-
>  include/linux/brcmphy.h       |   9 +
>  5 files changed, 488 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
> index 876f28fd8256..cc1b5e5a958c 100644
> --- a/drivers/net/phy/bcm-phy-lib.c
> +++ b/drivers/net/phy/bcm-phy-lib.c
> @@ -794,6 +794,47 @@ static int _bcm_phy_cable_test_get_status(struct phy_device *phydev,
>  	return ret;
>  }
>  
> +static int bcm_setup_forced(struct phy_device *phydev)
> +{
> +	u16 ctl = 0;
> +
> +	phydev->pause = 0;
> +	phydev->asym_pause = 0;
> +
> +	if (phydev->speed == SPEED_100)
> +		ctl |= LRECR_SPEED100;
> +
> +	if (phydev->duplex != DUPLEX_FULL)
> +		return -EOPNOTSUPP;
> +
> +	return phy_modify(phydev, MII_BCM54XX_LRECR, LRECR_SPEED100, ctl);

We should consider naming here. I assume this will not work for IEEE
forced mode? So maybe this should have _lre_ or _brr_ in the name to
make it clear what it actually does.

> +}



> +
> +/**
> + * bcm_linkmode_adv_to_mii_adv_t
> + * @advertising: the linkmode advertisement settings
> + * @return: LDS Auto-Negotiation Advertised Ability register value
> + *
> + * A small helper function that translates linkmode advertisement
> + * settings to phy autonegotiation advertisements for the
> + * MII_BCM54XX_LREANAA register of Broadcom PHYs capable of LDS
> + */
> +static u32 bcm_linkmode_adv_to_mii_adv_t(unsigned long *advertising)
> +{
> +	u32 result = 0;

Make here, and maybe lre_advertising?

Please go through all the functions and think about naming.

    Andrew

---
pw-bot: cr