Re: [PATCH net-next 1/2] net: phy: phy-c45: add SQI and SQI+ support for OATC14 10Base-T1S PHYs

[<Parthiban.Veerasooran@microchip.com>]

Hi Andrew,

On 13/11/25 9:45 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +/**
>> + * genphy_c45_oatc14_get_sqi - Get Signal Quality Indicator (SQI) from an OATC14
>> +                            10Base-T1S PHY
>> + * @phydev: pointer to the PHY device structure
>> + *
>> + * 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:
>> + * * Positive SQI/SQI+ value on success
>> + * * 0 if SQI update is not available
> 
> I thought 0 represented a very bad link? How is the call supposed to
> know the difference between a bad link, and no new value, try again
> later?
Yes, you are right. A value of 0 represents very poor SQI. Thank you for 
pointing out the need to distinguish between bad SQI and ‘no update’ 
values. Anyway, as per your comment below, this code will be removed 
since there is no need to check the update status.
> 
> I had a very quick look at the standard. All that update seems to
> indicate is that value has been updated since the last poll of that
> bit. There is no indication you cannot read the 'old' SQI value if
> there has not been an update. So i think you should always return an
> SQI value, if it is if 'old'.
Yes, I agree. I will update the code so that the reported SQI value is 
returned regardless of the update status in the next version.
> 
>> +     /* Read SQI capability */
>> +     ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MDIO_OATC14_ADFCAP);
>> +     if (ret < 0)
>> +             return ret;
> 
> I wounder if this should be cached somewhere. You don't expect it to
> change.
If I understand correctly, do you mean to store the capability details 
in the phydev structure when genphy_c45_oatc14_get_sqi_max() is called, 
and then use them in the genphy_c45_oatc14_get_sqi() function?

In that case, I may need to introduce new parameters in the phydev 
structure. Do you think introducing new parameters in the phydev 
structure is still necessary for this?

Best regards,
Parthiban V
> 
>          Andrew