Messages in this thread |  | | From | <> | | Subject | Re: [PATCH net-next v2 1/2] net: phy: phy-c45: add SQI and SQI+ support for OATC14 10Base-T1S PHYs | | Date | Tue, 25 Nov 2025 13:10:07 +0000 |
| |
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: > > ... > >> @@ -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 > >> + 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); > > ...
|  |