Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Michael Walle <michael@walle.cc>
To:     divya.koppera@microchip.com
Cc:     UNGLinuxDriver@microchip.com, andrew@lunn.ch, davem@davemloft.net,
        edumazet@google.com, hkallweit1@gmail.com, kuba@kernel.org,
        linux-kernel@vger.kernel.org, linux@armlinux.org.uk,
        netdev@vger.kernel.org, pabeni@redhat.com,
        Michael Walle <michael@walle.cc>
Subject: Re: [PATCH net-next] net: phy: micrel: Adding SQI support for lan8814 phy
Date:   Fri, 26 Aug 2022 10:42:49 +0200
Message-Id: <20220826084249.1031557-1-michael@walle.cc>
In-Reply-To: <20220825080549.9444-1-Divya.Koppera@microchip.com>
References: <20220825080549.9444-1-Divya.Koppera@microchip.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

> Supports SQI(Signal Quality Index) for lan8814 phy, where
> it has SQI index of 0-7 values and this indicator can be used
> for cable integrity diagnostic and investigating other noise
> sources.
> 
> Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com>
> ---
>  drivers/net/phy/micrel.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index e78d0bf69bc3..3775da7afc64 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -1975,6 +1975,13 @@ static int ksz886x_cable_test_get_status(struct phy_device *phydev,
>  #define LAN8814_CLOCK_MANAGEMENT			0xd
>  #define LAN8814_LINK_QUALITY				0x8e
>  
> +#define LAN8814_DCQ_CTRL				0xe6
> +#define LAN8814_DCQ_CTRL_READ_CAPTURE_			BIT(15)

Why does it end with an underscore?

> +#define LAN8814_DCQ_CTRL_CHANNEL_MASK			GENMASK(1, 0)
> +#define LAN8814_DCQ_SQI					0xe4
> +#define LAN8814_DCQ_SQI_MAX				7
> +#define LAN8814_DCQ_SQI_VAL_MASK			GENMASK(3, 1)
> +
>  static int lanphy_read_page_reg(struct phy_device *phydev, int page, u32 addr)
>  {
>  	int data;
> @@ -2927,6 +2934,32 @@ static int lan8814_probe(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int lan8814_get_sqi(struct phy_device *phydev)
> +{
> +	int rc, val;
> +
> +	val = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_CTRL);
> +	if (val < 0)
> +		return val;
> +
> +	val &= ~LAN8814_DCQ_CTRL_CHANNEL_MASK;

I do have a datasheet for this PHY, but it doesn't mention 0xe6 on EP1.
So I can only guess that this "channel mask" is for the 4 rx/tx pairs
on GbE? And you only seem to evaluate one of them. Is that the correct
thing to do here?

-michael


> +	val |= LAN8814_DCQ_CTRL_READ_CAPTURE_;
> +	rc = lanphy_write_page_reg(phydev, 1, LAN8814_DCQ_CTRL, val);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_SQI);
> +	if (rc < 0)
> +		return rc;
> +
> +	return FIELD_GET(LAN8814_DCQ_SQI_VAL_MASK, rc);
> +}