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

[Simon Horman]

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

Hi Kamil,

Please don't include tags for external trackers in upstream commit messages.

> Signed-off-by: Kamil Horák - 2N <kamilh@xxxxxxxx>

...

> diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c

...

> +/**
> + * bcm_linkmode_adv_to_mii_adv_t

Please include a short description on the line above.

Flagged by kernel-doc -none -Wall

> + * @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)

...

> +/**
> + * bcm_config_advert - sanitize and advertise auto-negotiation parameters
> + * @phydev: target phy_device struct
> + *
> + * Description: Writes MII_BCM54XX_LREANAA with the appropriate values,
> + *   after sanitizing the values to make sure we only advertise
> + *   what is supported.  Returns < 0 on error, 0 if the PHY's advertisement
> + *   hasn't changed, and > 0 if it has changed.
> + */
> +int bcm_config_advert(struct phy_device *phydev)

Please consider including a Return: section in Kernel docs
for functions that return a value. Likewise for lre_update_link.

Also flagged by kernel-doc -none -Wall

...

> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c

...

> +static int bcm54811_read_abilities(struct phy_device *phydev)
> +{
> +	int val, err;
> +	int i;
> +	u8 brr_mode;
> +	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 };

Please consider arranging local variables in reverse xmas tree order -
longest like to shortest.

Edward Cree's tool can be useful here:
https://github.com/ecree-solarflare/xmastree

...