Nuvoton: netsocket correction of judgment errors

[sword-huang]

Description

Fixing reset_phy for Nuvoton - there are two checks that work with invalid values, should check for -1.

Impact of changes

Migration actions required

None

---

Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

---

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[X] Tests / results supplied as part of this PR

---

Reviewers

---

[ciarmcom]

@sword-huang, thank you for your changes.
@ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

[0xc0170]

@sword-huang Thanks for the fix. Please describe the fix in few words in description, helps introducing the pull request. I added there a text for now

Although as I don't have the entire context, it would help as well to provide a reason why -1 should be in there instead 0 (I'll read the code above and check return types to find out but should be provided in the commit message itself).

[0xc0170]

Wouldn't this be better than relying on wrapping around ?

    while(delayCnt > 0) {
        delayCnt--;
        if((mdio_read(CONFIG_PHY_ADDR, MII_BMSR) & (BMSR_ANEGCOMPLETE | BMSR_LSTATUS))
                == (BMSR_ANEGCOMPLETE | BMSR_LSTATUS))
            break;
    }

Then the checks for delayCnt == 0 are correct.

[sword-huang]

Wouldn't this be better than relying on wrapping around ?

    while(delayCnt > 0) {
        delayCnt--;
        if((mdio_read(CONFIG_PHY_ADDR, MII_BMSR) & (BMSR_ANEGCOMPLETE | BMSR_LSTATUS))
                == (BMSR_ANEGCOMPLETE | BMSR_LSTATUS))
            break;
    }

Then the checks for delayCnt == 0 are correct.

Yes! Your code is more readable. I have recommitted.

[0xc0170]

CI restarted

[mbed-ci]

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 1
Build artifacts

[0xc0170]

@ARMmbed/team-nuvoton please review

[michalpasztamobica]

I guess the main point was for the code to be able to trigger the delayCnt == 0 condition, so this change indeed accomplished its goal. I also understand that this is just a brute-force delay.
However this change actually causes one loop run less: the decrement operator comes first before the comparison, so before the changes the loop ended after 1999 decrements, now it ends after 2000 decrements. It would be good if @ARMmbed/team-nuvoton confirmed that this is also OK. If not - we could do a do-while instead of while-do.
But honestly - I don't think this single loop run makes any difference, so I just approve (ipcore is mentioned as reviewers) ;-)

[0xc0170]

please align this (4 spaces to left as other code)

[cyliangtw]

This loop is to avoid endless waiting as while PHY broken.
Thanks of your modification.

Pull request has been modified.

[0xc0170]

I pushed the change via github (there were tabs, was tricky on github), will start CI later today

Pull request has been modified.

[0xc0170]

CI started

[mbed-ci]

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 2
Build artifacts