On Thu, 17 Jan 2019 at 12:27, Ard Biesheuvel <ard.biesheu...@linaro.org> wrote: > > On Thu, 17 Jan 2019 at 11:10, Leif Lindholm <leif.lindh...@linaro.org> wrote: > > > > 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? > > > > Fair enough. I will drop the include here, and add it to whichever > source file requires it afterwards. >
That include turns out to be entirely redundant, so I will just drop it. > > > #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