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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, Jun 17, 2024 at 01:38:41PM +0200, Kamil Horák - 2N wrote:
> +int bcm_config_lre_advert(struct phy_device *phydev)
> +{
> +	int err;
> +	u32 adv;
> +
> +	/* Only allow advertising what this PHY supports */
> +	linkmode_and(phydev->advertising, phydev->advertising,
> +		     phydev->supported);

Isn't this already done by phy_ethtool_ksettings_set() ?

        linkmode_copy(advertising, cmd->link_modes.advertising);
...
        /* We make sure that we don't pass unsupported values in to the PHY */
        linkmode_and(advertising, advertising, phydev->supported);
...
        linkmode_copy(phydev->advertising, advertising);

> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> index 370e4ed45098..5e590c8f82c4 100644
> --- a/drivers/net/phy/broadcom.c
> +++ b/drivers/net/phy/broadcom.c
> @@ -5,6 +5,9 @@
>   *	Broadcom BCM5411, BCM5421 and BCM5461 Gigabit Ethernet
>   *	transceivers.
>   *
> + *	Broadcom BCM54810, BCM54811 BroadR-Reach transceivers.
> + *
> + *

Nit: why two blank lines?

> +static int bcm54811_read_abilities(struct phy_device *phydev)
> +{
> +	static const int modes_array[] = { ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
> +					   ETHTOOL_LINK_MODE_10baseT1BRR_Full_BIT,
> +					   ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> +					   ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> +					   ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
> +					   ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> +					   ETHTOOL_LINK_MODE_100baseT_Half_BIT,
> +					   ETHTOOL_LINK_MODE_10baseT_Full_BIT,
> +					   ETHTOOL_LINK_MODE_10baseT_Half_BIT };

Formatting...

	static const int modes_array[] = {
		ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
		...
		ETHTOOL_LINK_MODE_10baseT_Half_BIT
	};

please. This avoids wrapping beyond column 80, and is to kernel coding
standards.

> +	int i, val, err;
> +	u8 brr_mode;
> +
> +	for (i = 0; i < ARRAY_SIZE(modes_array); i++)
> +		linkmode_clear_bit(modes_array[i], phydev->supported);
> +
> +	err = bcm5481x_get_brrmode(phydev, &brr_mode);
> +	if (err)
> +		return err;
> +
> +	if (brr_mode) {
> +		linkmode_set_bit_array(phy_basic_ports_array,
> +				       ARRAY_SIZE(phy_basic_ports_array),
> +				       phydev->supported);
> +
> +		val = phy_read(phydev, MII_BCM54XX_LRESR);
> +		if (val < 0)
> +			return val;
> +
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> +				 phydev->supported, 1);

		linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
				 phydev->supported);

...
> +	/* Ensure LRE or IEEE register set is accessed according to the brr on/off,
> +	 *  thus set the override
> +	 */
> +	return bcm_phy_write_exp(phydev, BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL,
> +		BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_EN |
> +		(on ? 0 : BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_OVERRIDE_VAL));

Needless parens, wrong formatting. Consider a local variable:

	val = BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_EN;
	if (!on)
		val |= BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_OVERRIDE_VAL;

	return bcm_phy_write_exp(phydev,
				 BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL, val);

would be much nicer to read.

> +	if (phydev->autoneg != AUTONEG_ENABLE) {
> +		if (!phydev->autoneg_complete) {
> +			/* aneg not yet done, reset all relevant bits */
> +			static const int br_bits[] = { ETHTOOL_LINK_MODE_Autoneg_BIT,
> +						       ETHTOOL_LINK_MODE_Pause_BIT,
> +						       ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +						       ETHTOOL_LINK_MODE_10baseT1BRR_Full_BIT,
> +						       ETHTOOL_LINK_MODE_100baseT1_Full_BIT };

More formatting issues. Maybe consider moving these out of the function?

> +			for (i = 0; i < ARRAY_SIZE(br_bits); i++)
> +				linkmode_clear_bit(br_bits[i], phydev->lp_advertising);

Formatting issue...

...
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +				 phydev->lp_advertising, lrelpa & LRELPA_PAUSE_ASYM);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +				 phydev->lp_advertising, lrelpa & LRELPA_PAUSE);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
> +				 phydev->lp_advertising, lrelpa & LRELPA_100_1PAIR);
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_10baseT1BRR_Full_BIT,
> +				 phydev->lp_advertising, lrelpa & LRELPA_10_1PAIR);

More formatting issues.

> +static int bcm54811_read_status(struct phy_device *phydev)
> +{
> +	int err;
> +	u8 brr_mode;

Reverse Christmas tree please.

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