On 7/24/20 9:29 AM, bryan.whiteh...@microchip.com wrote:
> Hi Florian, see below.
>
>>> /* bus->mdio_lock should be locked when using this function */
>>> +/* Page should already be set to MSCC_PHY_PAGE_EXTENDED_GPIO */
>>> +static int vsc8574_wait_for_micro_complete(struct phy_device *phydev)
>>> +{
>>> + u16 timeout = 500;
>>> + u16 reg18g = 0;
>>> +
>>> + reg18g = phy_base_read(phydev, 18);
>>> + while (reg18g & 0x8000) {
>>> + timeout--;
>>> + if (timeout == 0)
>>> + return -1;
>>> + usleep_range(1000, 2000);
>>> + reg18g = phy_base_read(phydev, 18);
>>
>> Please consider using phy_read_poll_timeout() instead of open coding this
>> busy
>> waiting loop.
>
> There are a couple issues with the use of phy_read_poll_timeout
> 1) It requires the use of phy_read, which acquires bus->mdio_lock.
> But this function is run with the assumption that, that lock is already
> acquired.
> There for I presume it will deadlock> 2) The implementation of phy_base_read
> uses __phy_package_read which
uses a shared phy addr, rather than the addr associated with the phydev.
>
> These issues could be eliminated if I used read_poll_timeout directly.
> Does that seem reasonable to you?
Certainly, whatever makes the code more maintainable and makes use of
existing functions is better on all counts.
--
Florian
