Received-SPF: pass (google.com: domain of linux-kernel+bounces-204016-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 6 Jun 2024 10:34:23 +0100
From: Simon Horman <horms@kernel.org>
To: Kamil =?utf-8?B?SG9yw6Fr?= - 2N <kamilh@axis.com>
Cc: florian.fainelli@broadcom.com, bcm-kernel-feedback-list@broadcom.com,
	andrew@lunn.ch, hkallweit1@gmail.com, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 3/3] net: phy: bcm-phy-lib: Implement BroadR-Reach
 link modes
Message-ID: <20240606093423.GA875456@kernel.org>
References: <20240605095646.3924454-1-kamilh@axis.com>
 <20240605095646.3924454-4-kamilh@axis.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240605095646.3924454-4-kamilh@axis.com>

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@axis.com>

...

> 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

...