Hi Philippe,
Thanks for your suggestions. I incorporated some of the suggestions into the V2
series.
Thanks,
Michael
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Philippe
> Mathieu-Daudé
> Sent: Wednesday, November 27, 2019 3:49 AM
> To: devel@edk2.groups.io; Kubacki, Michael A
> <michael.a.kuba...@intel.com>
> Cc: Bi, Dandan <dandan...@intel.com>; Gao, Liming <liming....@intel.com>;
> Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>
> Subject: Re: [edk2-devel] [PATCH V1 2/2] MdeModulePkg PeiCore: Improve
> comment semantics
>
> On 11/27/19 5:06 AM, Kubacki, Michael A via Groups.Io wrote:
> > Clarifies wording in several PeiCore comments to improve
>
> "Clarify"?
>
I believe an implied subject noun led to some confusion so I just explicitly
included that in the V2 message.
> > reading comprehension.
> >
> > Cc: Dandan Bi <dandan...@intel.com>
> > Cc: Liming Gao <liming....@intel.com>
> > Cc: Jian J Wang <jian.j.w...@intel.com>
> > Cc: Hao A Wu <hao.a...@intel.com>
> > Signed-off-by: Michael Kubacki <michael.a.kuba...@intel.com>
> > ---
> > MdeModulePkg/Core/Pei/FwVol/FwVol.h | 4 ++--
> > MdeModulePkg/Core/Pei/PeiMain.h | 11 +++++-----
> > MdeModulePkg/Core/Pei/Dependency/Dependency.c | 4 ++--
> > MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 4 ++--
> > MdeModulePkg/Core/Pei/FwVol/FwVol.c | 23 ++++++++++----------
> > MdeModulePkg/Core/Pei/Memory/MemoryServices.c | 4 ++--
> > 6 files changed, 26 insertions(+), 24 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Pei/FwVol/FwVol.h
> > b/MdeModulePkg/Core/Pei/FwVol/FwVol.h
> > index 263f0d7a56..8aaf84870b 100644
> > --- a/MdeModulePkg/Core/Pei/FwVol/FwVol.h
> > +++ b/MdeModulePkg/Core/Pei/FwVol/FwVol.h
> > @@ -303,9 +303,9 @@ FindFileEx (
> > );
> >
> > /**
> > - Report the information for a new discovered FV in unknown format.
> > + Report the information for a newly discovered FV in an unknown format.
> >
> > - If the EFI_PEI_FIRMWARE_VOLUME_PPI has not been installed for
> > specific FV format, but
> > + If the EFI_PEI_FIRMWARE_VOLUME_PPI has not been installed for a
> > + third-party FV format, but
> > the FV in this FV format has been discovered, then the information of
> this FV
> > will be cached into PEI_CORE_INSTANCE's UnknownFvInfo array.
> > Also a notification would be installed for unknown FV format GUID,
> > if EFI_PEI_FIRMWARE_VOLUME_PPI diff --git
> > a/MdeModulePkg/Core/Pei/PeiMain.h
> b/MdeModulePkg/Core/Pei/PeiMain.h
> > index 3f61247a0f..96d6df0485 100644
> > --- a/MdeModulePkg/Core/Pei/PeiMain.h
> > +++ b/MdeModulePkg/Core/Pei/PeiMain.h
> > @@ -1217,8 +1217,8 @@ PeiFfsGetVolumeInfo (
> > );
> >
> > /**
> > - This routine enable a PEIM to register itself to shadow when PEI
> > Foundation
> > - discovery permanent memory.
> > + This routine enables a PEIM to register itself for shadow when the
> > + PEI Foundation discovers permanent memory.
> >
> > @param FileHandle File handle of a PEIM.
> >
> > @@ -1314,12 +1314,13 @@ ProcessFvFile (
> > );
> >
> > /**
> > - Get instance of PEI_CORE_FV_HANDLE for next volume according to
> given index.
> > + Gets a PEI_CORE_FV_HANDLE instance for the next volume according to
> the given index.
>
> "Get"?
>
I left as-is in V2. You can see similar usage in the majority of function
descriptions in BaseLib.h for example.
> >
> > - This routine also will install FvInfo PPI for FV HOB in PI ways.
> > + This routine also will install an instance of the FvInfo PPI for
> > + the FV HOB as defined in the PI specification.
> >
> > @param Private Pointer of PEI_CORE_INSTANCE
> > - @param Instance The index of FV want to be searched.
> > + @param Instance The index of the FV to search.
>
> Maybe without "The" and trailing dot?
>
Done in V2.
> >
> > @return Instance of PEI_CORE_FV_HANDLE.
> > **/
> > diff --git a/MdeModulePkg/Core/Pei/Dependency/Dependency.c
> > b/MdeModulePkg/Core/Pei/Dependency/Dependency.c
> > index 9a8353aef2..b53e5f2686 100644
> > --- a/MdeModulePkg/Core/Pei/Dependency/Dependency.c
> > +++ b/MdeModulePkg/Core/Pei/Dependency/Dependency.c
> > @@ -2,8 +2,8 @@
> > PEI Dispatcher Dependency Evaluator
> >
> > This routine evaluates a dependency expression
> > (DEPENDENCY_EXPRESSION) to determine
> > - if a driver can be scheduled for execution. The criteria for
> > - schedulability is that the dependency expression is satisfied.
> > + if a driver can be scheduled for execution. The criteria to be
> > + scheduled is that the dependency expression is satisfied.
> >
> > Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> > SPDX-License-Identifier: BSD-2-Clause-Patent diff --git
> > a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > index c9f2a91264..a18ac47f61 100644
> > --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> > @@ -1347,8 +1347,8 @@ DepexSatisfied (
> > }
> >
> > /**
> > - This routine enable a PEIM to register itself to shadow when PEI
> > Foundation
> > - discovery permanent memory.
> > + This routine enables a PEIM to register itself for shadow when the
> > + PEI Foundation discovers permanent memory.
> >
> > @param FileHandle File handle of a PEIM.
> >
> > diff --git a/MdeModulePkg/Core/Pei/FwVol/FwVol.c
> > b/MdeModulePkg/Core/Pei/FwVol/FwVol.c
> > index c21eb9c039..a9fa476846 100644
> > --- a/MdeModulePkg/Core/Pei/FwVol/FwVol.c
> > +++ b/MdeModulePkg/Core/Pei/FwVol/FwVol.c
> > @@ -593,7 +593,7 @@ FirmwareVolumeInfoPpiNotifyCallback (
> > }
> >
> > //
> > - // Locate the corresponding FV_PPI according to founded FV's format
> > GUID
> > + // Locate the corresponding FV_PPI according to the format GUID of
> > + the FV found
> > //
> > Status = PeiServicesLocatePpi (
> > &FvInfo2Ppi.FvFormat,
> > @@ -1533,7 +1533,7 @@ ProcessFvFile (
> > );
> >
> > //
> > - // Inform the extracted FvImage to FV HOB consumer phase, i.e. DXE
> phase
> > + // Expose the extracted FvImage to the FV HOB consumer phase,
> > + i.e. DXE phase
> > //
> > BuildFvHob (
> > (EFI_PHYSICAL_ADDRESS) (UINTN) FvHeader, @@ -2087,12 +2087,13
> > @@ FvHandleToCoreHandle (
> > }
> >
> > /**
> > - Get instance of PEI_CORE_FV_HANDLE for next volume according to
> given index.
> > + Gets a PEI_CORE_FV_HANDLE instance for the next volume according to
> the given index.
>
> "Get"?
>
As in FwVol.h, I left as-is in V2.
> >
> > - This routine also will install FvInfo PPI for FV HOB in PI ways.
> > + This routine also will install an instance of the FvInfo PPI for
> > + the FV HOB as defined in the PI specification.
> >
> > @param Private Pointer of PEI_CORE_INSTANCE
> > - @param Instance The index of FV want to be searched.
> > + @param Instance The index of the FV to search.
>
> Without "The"/trailing dot?
>
As in FwVol.h, this is done in V2.
> >
> > @return Instance of PEI_CORE_FV_HANDLE.
> > **/
> > @@ -2185,13 +2186,13 @@ PeiReinitializeFv (
> > }
> >
> > /**
> > - Report the information for a new discovered FV in unknown third-party
> format.
> > + Report the information for a newly discovered FV in an unknown format.
> >
> > - If the EFI_PEI_FIRMWARE_VOLUME_PPI has not been installed for
> > third-party FV format, but
> > - the FV in this format has been discovered, then this FV's
> > information will be cached into
> > - PEI_CORE_INSTANCE's UnknownFvInfo array.
> > - Also a notification would be installed for unknown third-party FV
> > format guid, if EFI_PEI_FIRMWARE_VOLUME_PPI
> > - is installed later by platform's PEIM, the original unknown
> > third-party FV will be processed by
> > + If the EFI_PEI_FIRMWARE_VOLUME_PPI has not been installed for a
> > + third-party FV format, but the FV in this FV format has been
> > + discovered, then the information of this FV
>
> Maybe "in this FV format" is redundant?
>
Removed "in this FV format" in V2.
> > + will be cached into PEI_CORE_INSTANCE's UnknownFvInfo array.
> > + Also a notification would be installed for unknown FV format GUID,
> > + if EFI_PEI_FIRMWARE_VOLUME_PPI is installed later by platform's
> > + PEIM, the original unknown FV will be processed by
> > using new installed EFI_PEI_FIRMWARE_VOLUME_PPI.
> >
> > @param PrivateData Point to instance of PEI_CORE_INSTANCE diff
> > --git a/MdeModulePkg/Core/Pei/Memory/MemoryServices.c
> > b/MdeModulePkg/Core/Pei/Memory/MemoryServices.c
> > index 838a003baa..e713e6811a 100644
> > --- a/MdeModulePkg/Core/Pei/Memory/MemoryServices.c
> > +++ b/MdeModulePkg/Core/Pei/Memory/MemoryServices.c
> > @@ -759,7 +759,7 @@ PeiFreePages (
> > /**
> >
> > Pool allocation service. Before permanent memory is discovered,
> > the pool will
> > - be allocated the heap in the temporary memory. Generally, the size
> > of heap in temporary
> > + be allocated in the heap in temporary memory. Generally, the size
> > + of the heap in temporary
> > memory does not exceed to 64K, so the biggest pool size could be
> allocated is
> > 64K.
> >
> > @@ -789,7 +789,7 @@ PeiAllocatePool (
> > //
> >
> > //
> > - // Generally, the size of heap in temporary memory does not exceed
> > to 64K,
> > + // Generally, the size of heap in temporary memory does not exceed
> > + 64K,
> > // HobLength is multiples of 8 bytes, so the maximum size of pool is
> 0xFFF8 - sizeof (EFI_HOB_MEMORY_POOL)
> > //
> > if (Size > (0xFFF8 - sizeof (EFI_HOB_MEMORY_POOL))) {
> >
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51376): https://edk2.groups.io/g/devel/message/51376
Mute This Topic: https://groups.io/mt/62227959/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
