Hi Simon,

Thank you for reviewing the patch.

On 22/11/25 5:16 pm, Simon Horman wrote:

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On Tue, Nov 18, 2025 at 10:29:45AM +0530, Parthiban Veerasooran wrote:

...

quoted

@@ -1695,3 +1695,89 @@ int genphy_c45_oatc14_cable_test_start(struct phy_device *phydev)
                               OATC14_HDD_START_CONTROL);
  }
  EXPORT_SYMBOL(genphy_c45_oatc14_cable_test_start);
+
+/**
+ * genphy_c45_oatc14_get_sqi_max - Get maximum supported SQI or SQI+ level of
+ *                              OATC14 10Base-T1S PHY
+ * @phydev: pointer to the PHY device structure
+ *
+ * This function reads the advanced capability register to determine the maximum
+ * supported Signal Quality Indicator (SQI) or SQI+ level
+ *
+ * Return:
+ * * Maximum SQI/SQI+ level (≥0)
+ * * -EOPNOTSUPP if not supported
+ * * Negative errno on read failure
+ */
+int genphy_c45_oatc14_get_sqi_max(struct phy_device *phydev)
+{
+     u8 bits;
+     int ret;
+
+     ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MDIO_OATC14_ADFCAP);
+     if (ret < 0)
+             return ret;
+
+     /* Check for SQI+ capability
+      * 0 - SQI+ is not supported
+      * (3-8) bits for (8-256) SQI+ levels supported
+      */
+     bits = FIELD_GET(OATC14_ADFCAP_SQIPLUS_CAPABILITY, ret);
+     if (bits) {
+             phydev->oatc14_sqiplus_bits = bits;
+             /* Max sqi+ level supported: (2 ^ bits) - 1 */
+             return BIT(bits) - 1;
+     }
+
+     /* Check for SQI capability
+      * 0 - SQI is not supported
+      * 1 - SQI is supported (0-7 levels)
+      */
+     if (ret & OATC14_ADFCAP_SQI_CAPABILITY)
+             return OATC14_SQI_MAX_LEVEL;
+
+     return -EOPNOTSUPP;
+}
+EXPORT_SYMBOL(genphy_c45_oatc14_get_sqi_max);
+
+/**
+ * genphy_c45_oatc14_get_sqi - Get Signal Quality Indicator (SQI) from an OATC14
+ *                          10Base-T1S PHY
+ * @phydev: pointer to the PHY device structure
+ *
+ * This function reads the SQI+ or SQI value from an OATC14-compatible
+ * 10Base-T1S PHY. If SQI+ capability is supported, the function returns the
+ * extended SQI+ value; otherwise, it returns the basic SQI value.
+ *
+ * Return:
+ * * SQI/SQI+ value on success
+ * * Negative errno on read failure
+ */
+int genphy_c45_oatc14_get_sqi(struct phy_device *phydev)
+{
+     u8 shift;
+     int ret;
+
+     /* Calculate and return SQI+ value if supported */
+     if (phydev->oatc14_sqiplus_bits) {

Hi Parthiban,

AFAICT oatc14_sqiplus_bits will always be 0 until
genphy_c45_oatc14_get_sqi_max() is called, after which
it may be some other value.

But according to the flow of linkstate_prepare_data()
it seems that genphy_c45_oatc14_get_sqi_max()
will be called after genphy_c45_oatc14_get_sqi().

In which case the condition above will be false
(unless genphy_c45_oatc14_get_sqi_max was somehow already
called by some other means.)

This doesn't seem to be in line with the intention of this code.

Flagged by Claude Code with https://github.com/masoncl/review-prompts/

Good catch! oatc14_sqiplus_bits is not updated the first time 
genphy_c45_oatc14_get_sqi() is called, and is later updated by 
genphy_c45_oatc14_get_sqi_max(). Thank you for pointing it out; I will 
fix it in the next version.

Best regards,
Parthiban V

quoted

+             ret = phy_read_mmd(phydev, MDIO_MMD_VEND2,
+                                MDIO_OATC14_DCQ_SQIPLUS);
+             if (ret < 0)
+                     return ret;
+
+             /* SQI+ uses N MSBs out of 8 bits, left-aligned with padding 1's
+              * Calculate the right-shift needed to isolate the N bits.
+              */
+             shift = 8 - phydev->oatc14_sqiplus_bits;
+
+             return (ret & OATC14_DCQ_SQIPLUS_VALUE) >> shift;
+     }
+
+     /* Read and return SQI value if SQI+ capability is not supported */
+     ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MDIO_OATC14_DCQ_SQI);
+     if (ret < 0)
+             return ret;
+
+     return ret & OATC14_DCQ_SQI_VALUE;
+}
+EXPORT_SYMBOL(genphy_c45_oatc14_get_sqi);

...