[Intel-wired-lan] [next-queue 1/8] e1000: clean up the useless 'continue' statement

[Rustad, Mark D]

> On Sep 16, 2015, at 9:38 AM, Alexander Duyck <alexander.duyck at gmail.com> wrote:
> 
> I kind of like Mark's idea better, though I would make one change.  I would make it so that you pull the check for i outside the loop so you have something like:
> 		for (i = 0; i < 32; i++) {
> 			hw->phy_addr = i;
> 			e1000_read_phy_reg(hw, PHY_ID2, &tmp);
> 
> 			if (tmp != 0 && tmp != 0xFF)
> 				break;
> 		}
> 
> 		if (i >= 32)
> 			goto no_err;

Shouldn't that goto target be err_eeprom?

> This way you should only have to ever do a comparison of i once per loop, and hopefully the compiler is smart enough to realize that i >= 32 is the exit condition for the loop and will place the jump to that label accordingly.

Yes, obviously I prefer the above form, with the corrected goto target, since that is what I described. The only difference being >= vs = and thought shouldn't matter. With >= being the logical opposite of the loop end test, it is probably the better choice.

> As a side note I am curious if this code is even correct.  I see tmp is a u16, the declaration for which could be moved down into the if statement if I am not mistaken, and I am curious as to why we are comparing it to 0xFF instead of 0xFFFF.  I suspect that the 0xFF check might not be adding any value, but I could be wrong.

Yow! That is a really good question that I don't have an immediate answer to, having no experience with the internals of the e1000 driver and the devices it supports. Some research is required to answer that question.

--
Mark Rustad, Networking Division, Intel Corporation

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 841 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20150916/32366eb1/attachment-0001.asc>