Re: [PATCH net-next V4] net: phy: microchip_t1: SQI support for LAN887x

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

On Tue, Sep 17, 2024 at 05:26:57PM +0530, Tarun Alle wrote:
> From: Tarun Alle <Tarun.Alle@microchip.com>
> 
> Add support for measuring Signal Quality Index for LAN887x T1 PHY.
> Signal Quality Index (SQI) is measure of Link Channel Quality from
> 0 to 7, with 7 as the best. By default, a link loss event shall
> indicate an SQI of 0.
> 
> Signed-off-by: Tarun Alle <Tarun.Alle@microchip.com>

Please note that the merge window is open, which means that net-next is
currently closed. Thus, patches should be submitted as RFC.

> ---
> v3 -> v4
> - Added check to handle invalid samples.
> - Added macro for ARRAY_SIZE(rawtable).
> 
> v2 -> v3
> - Replaced hard-coded values with ARRAY_SIZE(rawtable).
> 
> v1 -> v2
> - Replaced hard-coded 200 with ARRAY_SIZE(rawtable).

Hmm. We've been through several iterations trying to clean this up
into something more easily readable, but I fear there'll be another
iteration.

Maybe the following would be nicer:

enum {
	SQI_SAMPLES = 200,
	/* Look at samples of the middle 60% */
	SQI_INLIERS_NUM = SQI_SAMPLES * 60 / 100,
	SQI_INLIERS_START = (SQI_SAMPLES - SQI_INLIERS_NUM) / 2,
	SQI_INLIERS_END = SQI_INLIERS_START + SQI_INLIERS_NUM,
};

> +static int lan887x_get_sqi_100M(struct phy_device *phydev)
> +{
> +	u16 rawtable[200];

	u16 rawtable[SQI_SAMPLES];

> +	u32 sqiavg = 0;
> +	u8 sqinum = 0;
> +	int rc;

Since you use "i" multiple times, declare it at the beginning of the
function rather than in each for loop.

	int i;

> +
> +	/* Configuration of SQI 100M */
> +	rc = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> +			   LAN887X_COEFF_PWR_DN_CONFIG_100,
> +			   LAN887X_COEFF_PWR_DN_CONFIG_100_V);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = phy_write_mmd(phydev, MDIO_MMD_VEND1, LAN887X_SQI_CONFIG_100,
> +			   LAN887X_SQI_CONFIG_100_V);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = phy_read_mmd(phydev, MDIO_MMD_VEND1, LAN887X_SQI_CONFIG_100);
> +	if (rc != LAN887X_SQI_CONFIG_100_V)
> +		return -EINVAL;
> +
> +	rc = phy_modify_mmd(phydev, MDIO_MMD_VEND1, LAN887X_POKE_PEEK_100,
> +			    LAN887X_POKE_PEEK_100_EN,
> +			    LAN887X_POKE_PEEK_100_EN);
> +	if (rc < 0)
> +		return rc;
> +
> +	/* Required before reading register
> +	 * otherwise it will return high value
> +	 */
> +	msleep(50);
> +
> +	/* Link check before raw readings */
> +	rc = genphy_c45_read_link(phydev);
> +	if (rc < 0)
> +		return rc;
> +
> +	if (!phydev->link)
> +		return -ENETDOWN;
> +
> +	/* Get 200 SQI raw readings */
> +	for (int i = 0; i < ARRAY_SIZE(rawtable); i++) {

	for (i = 0; i < SQI_SAMPLES; i++) {

> +		rc = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> +				   LAN887X_POKE_PEEK_100,
> +				   LAN887X_POKE_PEEK_100_EN);
> +		if (rc < 0)
> +			return rc;
> +
> +		rc = phy_read_mmd(phydev, MDIO_MMD_VEND1,
> +				  LAN887X_SQI_MSE_100);
> +		if (rc < 0)
> +			return rc;
> +
> +		rawtable[i] = (u16)rc;
> +	}
> +
> +	/* Link check after raw readings */
> +	rc = genphy_c45_read_link(phydev);
> +	if (rc < 0)
> +		return rc;
> +
> +	if (!phydev->link)
> +		return -ENETDOWN;
> +
> +	/* Sort SQI raw readings in ascending order */
> +	sort(rawtable, ARRAY_SIZE(rawtable), sizeof(u16), data_compare, NULL);

	sort(rawtable, SQI_SAMPLES, sizeof(u16), data_compare, NULL);

Although renaming data_compare to sqi_compare would be even more
descriptive of what it's doing.

> +
> +	/* Keep inliers and discard outliers */
> +	for (int i = SQI100M_SAMPLE_INIT(5, rawtable);
> +	     i < SQI100M_SAMPLE_INIT(5, rawtable) * 4; i++)

	for (i = SQI_INLIERS_START; i < SQI_INLIERS_END; i++)

> +		sqiavg += rawtable[i];
> +
> +	/* Handle invalid samples */
> +	if (sqiavg != 0) {
> +		/* Get SQI average */
> +		sqiavg /= SQI100M_SAMPLE_INIT(5, rawtable) * 4 -
> +				SQI100M_SAMPLE_INIT(5, rawtable);

		sqiavg /= SQI_INLIERS_NUM;

Overall, I think this is better rather than the SQI100M_SAMPLE_INIT()
macro... for which I'm not sure what the _INIT() bit actually means.

I think my suggestion has the advantage that it makes it clear what
these various calculations are doing, because the result of the
calculations is described in the enum name.

Thanks.

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