I have no concern about the logic. Just some comments about the coding style.
Regards, Ray >-----Original Message----- >From: edk2-devel [mailto:[email protected]] On Behalf Of >Shivamurthy Shastri >Sent: Wednesday, February 10, 2016 10:10 PM >To: [email protected] >Cc: [email protected] >Subject: [edk2] [PATCH] Ax88772b: support for multiple dongles and chips > >Driver code is modified to support multiple ethernet dongles, which uses >similar ASIX chips. Also, it can be used for multiple ASIX chips with >similar register map. > >Enabled support for Apple Ethernet Adapter > >Contributed-under: TianoCore Contribution Agreement 1.0 >Signed-off-by: Shivamurthy Shastri <[email protected]> >--- > .../Bus/Usb/UsbNetworking/Ax88772b/Ax88772.h | 13 ++++++++- > .../Bus/Usb/UsbNetworking/Ax88772b/DriverBinding.c | 33 +++++++++++++--------- > 2 files changed, 32 insertions(+), 14 deletions(-) > >diff --git a/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/Ax88772.h >b/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/Ax88772.h >index ab75ec2..816f2b2 100644 >--- a/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/Ax88772.h >+++ b/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/Ax88772.h >@@ -297,12 +297,23 @@ > #define AN_10_HDX 0x0020 ///< 1 = 10BASE-T support > #define AN_CSMA_CD 0x0001 ///< 1 = IEEE 802.3 CSMA/CD > support > >- >+/* asix_flags defines */ >+#define FLAG_NONE 0 >+#define FLAG_TYPE_AX88172 (1U << 0) >+#define FLAG_TYPE_AX88772 (1U << 1) >+#define FLAG_TYPE_AX88772B (1U << 2) >+#define FLAG_EEPROM_MAC (1U << 3) /* initial mac address in eeprom */ 1. Please use "//" to start the comments. Please use BIT0, BIT1, BIT2, BIT3 macro. > > //------------------------------------------------------------------------------ > // Data Types > //------------------------------------------------------------------------------ > >+struct ASIX_DONGLE { >+ UINT16 VendorId; >+ UINT16 ProductId; >+ INT32 Flags; >+}; 2. please use "typedef struct {...} ASIX_DONGLE" >+ > /** > Ethernet header layout > >diff --git a/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/DriverBinding.c >b/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/DriverBinding.c >index 3b73040..6a10cbf 100644 >--- a/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/DriverBinding.c >+++ b/OptionRomPkg/Bus/Usb/UsbNetworking/Ax88772b/DriverBinding.c >@@ -14,6 +14,13 @@ > > #include "Ax88772.h" > >+struct ASIX_DONGLE ASIX_DONGLES[] = { >+ { 0x05AC, 0x1402, FLAG_TYPE_AX88772 }, /* Apple USB Ethernet Adapter >*/ >+ /* ASIX 88772B */ >+ { 0x0B95, 0x772B, FLAG_TYPE_AX88772B | FLAG_EEPROM_MAC }, >+ { 0x0000, 0x0000, FLAG_NONE } /* END - Do not remove */ >+}; 3. When you follow my proposal #2, you can remove "struct" keyword in above. >+ > /** > Verify the controller type > >@@ -36,6 +43,8 @@ DriverSupported ( > EFI_USB_DEVICE_DESCRIPTOR Device; > EFI_USB_IO_PROTOCOL * pUsbIo; > EFI_STATUS Status; >+ UINT32 i; 4. Better to use Index rather than i. >+ > // > // Connect to the USB stack > // >@@ -60,19 +69,17 @@ DriverSupported ( > else { > // > // Validate the adapter >- // >- if ( VENDOR_ID == Device.IdVendor ) { >- >- if (PRODUCT_ID == Device.IdProduct) { >- DEBUG ((EFI_D_INFO, "Found the AX88772B\r\n")); >- } 5. Since you removed the above code, can you also remove VENDOR_ID and PRODUCT_ID macro in header file? >- else { >- Status = EFI_UNSUPPORTED; >- } >- } >- else { >- Status = EFI_UNSUPPORTED; >- } >+ // >+ for (i = 0; ASIX_DONGLES[i].VendorId != 0; i++) { >+ if (ASIX_DONGLES[i].VendorId == Device.IdVendor && >+ ASIX_DONGLES[i].ProductId == Device.IdProduct) { >+ DEBUG ((EFI_D_INFO, "Found the AX88772B\r\n")); >+ break; >+ } >+ } 6. Please do not use TAB and use 2 spaces to indent. >+ >+ if (ASIX_DONGLES[i].VendorId == 0) >+ Status = EFI_UNSUPPORTED; > } > > // >-- >1.9.1 > >_______________________________________________ >edk2-devel mailing list >[email protected] >https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
