On Fri, Jan 04, 2019 at 03:43:31PM +0100, Ard Biesheuvel wrote: > In preparation of creating a SMM version of the FIP006 NOR flash > driver, refactor the existing pieces into a core driver, the FVB > methods and the DXE instantiation code. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
I only have one nitpicky question on this patch: > --- > Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/Fip006Dxe.inf > | 6 +- > Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlash.c > | 1006 +++++++++++++++++ > Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/{NorFlashDxe.h => NorFlash.h} > | 52 +- > Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlashDxe.c > | 1150 +++----------------- > Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/{NorFlashFvbDxe.c => > NorFlashFvb.c} | 161 +-- > 5 files changed, 1194 insertions(+), 1181 deletions(-) > > diff --git a/Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlashDxe.h > b/Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlash.h > similarity index 88% > rename from Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlashDxe.h > rename to Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlash.h > index 20e74b0320ce..61b8e6a08fa0 100644 > --- a/Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlashDxe.h > +++ b/Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlash.h > @@ -27,11 +27,9 @@ > #include <Protocol/FirmwareVolumeBlock.h> > > #include <Library/DebugLib.h> > -#include <Library/DxeServicesTableLib.h> > #include <Library/IoLib.h> > +#include <Library/MemoryAllocationLib.h> Why add this include? I'm not going to ask to move out the existing headers not actually used by this file, but could we avoid adding new ones? https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/v/release/2.20/5_source_files/53_include_files.html#534-include-files-may-include-only-those-headers-that-it-directly-depends-upon is actually a rule I agree with. But also, aren't all of the users of this file already manually including this one? > #include <Library/NorFlashPlatformLib.h> > -#include <Library/UefiLib.h> > -#include <Library/UefiRuntimeLib.h> > > #include "Fip006Reg.h" > > diff --git a/Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlashDxe.c > b/Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlashDxe.c > index e52ab52d8cf7..6c07799b22d8 100644 > --- a/Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlashDxe.c > +++ b/Silicon/Socionext/SynQuacer/Drivers/Fip006Dxe/NorFlashDxe.c > @@ -15,15 +15,16 @@ > **/ > > #include <Library/BaseMemoryLib.h> > +#include <Library/DxeServicesTableLib.h> > +#include <Library/HobLib.h> > #include <Library/MemoryAllocationLib.h> At least this one does. > #include <Library/NorFlashInfoLib.h> > #include <Library/PcdLib.h> > #include <Library/UefiBootServicesTableLib.h> > #include <Library/UefiLib.h> > +#include <Library/UefiRuntimeLib.h> > > -#include "NorFlashDxe.h" > - > -STATIC EFI_EVENT mNorFlashVirtualAddrChangeEvent; > +#include "NorFlash.h" _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel