Re: [PATCH net-next v3 4/6] net: phy: microchip_t1s: fix reset complete status handling

[Parthiban.Veerasooran]

Hi Ramon,

On 25/05/23 11:56 pm, Ramón Nordin Rodriguez wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +     /* Read STS2 register and check for the Reset Complete status to do the
>> +      * init configuration. If the Reset Complete is not set, wait for 5us
>> +      * and then read STS2 register again and check for Reset Complete status.
>> +      * Still if it is failed then declare PHY reset error or else proceed
>> +      * for the PHY initial register configuration.
>> +      */
>> +     err = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_STS2);
>> +     if (err < 0)
>> +             return err;
>> +
>> +     if (!(err & LAN867x_RESET_COMPLETE_STS)) {
>> +             udelay(5);
>> +             err = phy_read_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_STS2);
>> +             if (err < 0)
>> +                     return err;
>> +             if (!(err & LAN867x_RESET_COMPLETE_STS)) {
>> +                     phydev_err(phydev, "PHY reset failed\n");
>> +                     return -ENODEV;
>> +             }
>> +     }
> 
> This comment explains exactly what the code does, which is also obvious
> from reading the code. A meaningful comment would be explaining why the
> state can change 5us later.
> 
As per design, LAN867x reset to be completed by 3us. Just for a safer 
side it is recommended to use 5us. With the assumption of more than 3us 
completion, the first read checks for the Reset Complete. If the 
config_init is more faster, then once again checks for it after 5us.

As you mentioned, can we remove the existing block comment as it 
explains the code and add the above comment to explain 5us delay.
What is your opinion on this proposal?

Best Regards,
Parthiban V