On Fri, Oct 14, 2016 at 11:12:32AM +0800, Heyi Guo wrote:
- soctype = PcdGet32(Pcdsoctype); for (HostBridgeNum = 0; HostBridgeNum < PCIE_HOST_BRIDGE_NUM; HostBridgeNum++) { for (Port = 0; Port < PCIE_MAX_PORT_NUM; Port++) {
if (!(((( PcieRootBridgeMask >> (4 * HostBridgeNum))) >> Port) & 0x1))
if (0x1610 == soctype)Revers the comparison, please.
Is there any special reason to reverse the comparison?
It confuses the reader by stating the comparison backwards compared to the operation actually being performed. While it is not explicitly banned by the EDK2 coding standard, all examples in that document do it the other way around.
Actually, our internal code conduct requires us to put the constant at the left, so that we won't forget the 2nd "=". Even it seems outdated now, I think it does no harm to do that.
Both gcc and clang will generate warnings for that mistake. gcc will generate it with -Wall and clang always. And since we always build with -Wall, doing the comparisons backwards adds no protection.
@@ -169,6 +218,14 @@ EFI_STATUS PcieEnableItssm(UINT32 soctype, UINT32 HostBridgeNum, UINT32 Port) (VOID)PcieRxValidCtrl(soctype, HostBridgeNum, Port, 1);
The return value of this function is VOID - why is it being cast to VOID? I noticed this in other locations too - please address throughout.
We add (VOID) cast to some function call which we don't care about the return value, so that static check tools like pc-lint will not complain.
Which is absolutely the correct thing to do, and makes the code more clear.
But if the prototype of the function is already VOID, then the (VOID) should have been added by mistake.
I guessed asmuch :)
For other comments, we will try to improve the code accordingly.
Thanks!
/ Leif