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

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 17 Jun 2024 13:38:41 +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.

Some nit picks below, but please don't repost until next week.
Sorry for the delay but it's vacation season, I think some of
the key folks are currently AFK :(

> + * bcm_config_lre_advert - sanitize and advertise Long-Distance Signaling
> + *  auto-negotiation parameters
> + * @phydev: target phy_device struct
> + * @return: 0 if the PHY's advertisement hasn't changed, < 0 on error,
> + *          > 0 if it has changed

 * Return: 0 if the PHY

no @ and after the description

> + *
> + * Description: Writes MII_BCM54XX_LREANAA with the appropriate values,

Please don't prefix the description with the word "description"..

> + *   after sanitizing the values to make sure we only advertise
> + *   what is supported.
> + */
> +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);
> +
> +	adv = bcm_linkmode_adv_to_lre_adv_t(phydev->advertising);
> +
> +	/* Setup BroadR-Reach mode advertisement */
> +	err = phy_modify_changed(phydev, MII_BCM54XX_LREANAA,
> +				 LRE_ADVERTISE_ALL | LREANAA_PAUSE |
> +				 LREANAA_PAUSE_ASYM, adv);
> +
> +	if (err < 0)
> +		return err;
> +
> +	return err;

You can return phy_modify_changed(... directly, no need for err

> +}
> +EXPORT_SYMBOL_GPL(bcm_config_lre_advert);
> +
>  MODULE_DESCRIPTION("Broadcom PHY Library");
>  MODULE_LICENSE("GPL v2");
>  MODULE_AUTHOR("Broadcom Corporation");
> diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
> index b52189e45a84..fecdd66ad736 100644
> --- a/drivers/net/phy/bcm-phy-lib.h
> +++ b/drivers/net/phy/bcm-phy-lib.h
> @@ -121,4 +121,8 @@ irqreturn_t bcm_phy_wol_isr(int irq, void *dev_id);
>  int bcm_phy_led_brightness_set(struct phy_device *phydev,
>  			       u8 index, enum led_brightness value);
>  
> +int bcm_setup_master_slave(struct phy_device *phydev);
> +int bcm_config_lre_aneg(struct phy_device *phydev, bool changed);
> +int bcm_config_lre_advert(struct phy_device *phydev);
> +
>  #endif /* _LINUX_BCM_PHY_LIB_H */
> 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.
> + *
> + *

double new line

>   *	Copyright (c) 2006  Maciej W. Rozycki
>   *
>   *	Inspired by code written by Amy Fong.
> @@ -553,18 +556,97 @@ static int bcm54810_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
>  	return -EOPNOTSUPP;
>  }
>  
> -static int bcm54811_config_init(struct phy_device *phydev)
> +static int bcm5481x_get_brrmode(struct phy_device *phydev, u8 *data)
>  {
> -	int err, reg;
> +	int reg;
>  
> -	/* Disable BroadR-Reach function. */
>  	reg = bcm_phy_read_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL);
> -	reg &= ~BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN;
> -	err = bcm_phy_write_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL,
> -				reg);
> -	if (err < 0)
> +
> +	*data = (reg & BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN) ? 1 : 0;
> +
> +	return 0;
> +}
> +
> +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 };

This is more normal formatting for the kernel:

	static const int modes_array[] = {
		ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
		ETHTOOL_LINK_MODE_10baseT1BRR_Full_BIT,

please try to avoid going over 80 characters

> +static int bcm54811_config_init(struct phy_device *phydev)
> +{
> +	int err, reg;
> +	bool brr = false;
> +	struct device_node *np = phydev->mdio.dev.of_node;

order variable declaration lines longest to shortest, 
AKA reverse xmas tree