From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web12.5837.1634117433414186696 for ; Wed, 13 Oct 2021 02:30:33 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id D27701063; Wed, 13 Oct 2021 02:30:31 -0700 (PDT) Received: from [192.168.1.16] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 028103F70D; Wed, 13 Oct 2021 02:30:30 -0700 (PDT) Message-ID: Date: Wed, 13 Oct 2021 10:30:20 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 From: "PierreGondois" Subject: Re: [edk2-devel] [PATCH v2 1/7] Silicon/ARM/NeoverseN1Soc: Fix missing function documentation To: devel@edk2.groups.io, khasim.mohammed@arm.com References: <20211010182956.13526-1-khasim.mohammed@arm.com> <20211010182956.13526-2-khasim.mohammed@arm.com> Cc: Sami Mujawar In-Reply-To: <20211010182956.13526-2-khasim.mohammed@arm.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hi Khasim, I had some questions about this path: On 10/10/21 19:29, Khasim Mohammed via groups.io wrote: > This patch adds missing documentation for few of the functions > and fixes few formatting changes. > > Signed-off-by: Khasim Syed Mohammed > --- > .../PciHostBridgeLib/PciHostBridgeLib.c | 18 ++++---- > .../Library/PlatformLib/PlatformLib.c | 43 ++++++++++++++++--- > 2 files changed, 47 insertions(+), 14 deletions(-) > > diff --git a/Silicon/ARM/NeoverseN1Soc/Library/PciHostBridgeLib/PciHostBridgeLib.c b/Silicon/ARM/NeoverseN1Soc/Library/PciHostBridgeLib/PciHostBridgeLib.c > index 9332939f63..ac88415fd2 100644 > --- a/Silicon/ARM/NeoverseN1Soc/Library/PciHostBridgeLib/PciHostBridgeLib.c > +++ b/Silicon/ARM/NeoverseN1Soc/Library/PciHostBridgeLib/PciHostBridgeLib.c Are the modifications in this file required ? The indentation seems correct to me > @@ -1,10 +1,10 @@ > /** @file > -* PCI Host Bridge Library instance for ARM Neoverse N1 platform > -* > -* Copyright (c) 2019 - 2020, ARM Limited. All rights reserved. > -* > -* SPDX-License-Identifier: BSD-2-Clause-Patent > -* > + PCI Host Bridge Library instance for ARM Neoverse N1 platform > + > + Copyright (c) 2019 - 2021, ARM Limited. All rights reserved.
> + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > **/ > > #include > @@ -96,7 +96,7 @@ STATIC PCI_ROOT_BRIDGE mPciRootBridge[] = { > /** > Return all the root bridge instances in an array. > > - @param Count Return the count of root bridge instances. > + @param Count Return the count of root bridge instances. > > @return All the root bridge instances in an array. > The array should be passed into PciHostBridgeFreeRootBridges() > @@ -115,8 +115,8 @@ PciHostBridgeGetRootBridges ( > /** > Free the root bridge instances array returned from PciHostBridgeGetRootBridges(). > > - @param Bridges The root bridge instances array. > - @param Count The count of the array. > + @param Bridges The root bridge instances array. > + @param Count The count of the array. > **/ > VOID > EFIAPI > diff --git a/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLib.c b/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLib.c > index f722080e56..d5ec0ff30d 100644 > --- a/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLib.c > +++ b/Silicon/ARM/NeoverseN1Soc/Library/PlatformLib/PlatformLib.c > @@ -1,9 +1,9 @@ > /** @file > -* > -* Copyright (c) 2018-2020, ARM Limited. All rights reserved. > -* > -* SPDX-License-Identifier: BSD-2-Clause-Patent > -* > + > + Copyright (c) 2018-2021, ARM Limited. All rights reserved.
> + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > **/ > > #include > @@ -17,6 +17,12 @@ STATIC ARM_CORE_INFO mCoreInfoTable[] = { > { 0x1, 0x1 } // Cluster 1, Core 1 > }; > > +/** > + Return the current Boot Mode. > + > + This function returns the boot reason on the platform. There are the '@return' and '@retval' keywords to indicate what a function returns. I think here it should be: @return The boot reason on the platform. > + > +**/ > EFI_BOOT_MODE > ArmPlatformGetBootMode ( > VOID > @@ -25,6 +31,15 @@ ArmPlatformGetBootMode ( > return BOOT_WITH_FULL_CONFIGURATION; > } > > +/** > + Initialize controllers that must be setup in the normal world. > + > + This function is called by the ArmPlatformPkg/Pei or ArmPlatformPkg/Pei/PlatformPeim > + in the PEI phase. It seems to exceed the 80 chars. > + > + @param[in] MpId Processor ID Same here, can you add: @retval RETURN_SUCCESS > + > +**/ > RETURN_STATUS > ArmPlatformInitialize ( > IN UINTN MpId > @@ -33,6 +48,15 @@ ArmPlatformInitialize ( > return RETURN_SUCCESS; > } > > +/** > + Populate the Platform core information. > + > + This function populates the ARM_MP_CORE_INFO_PPI with information about the cores. It seems to exceed the 80 chars. > + > + @param[out] CoreCount Number of cores > + @param[out] ArmCoreTable Table containing information about the cores > + Same here, can you add: @retval RETURN_SUCCESS > +**/ > EFI_STATUS > PrePeiCoreGetMpCoreInfo ( > OUT UINTN *CoreCount, > @@ -56,6 +80,15 @@ EFI_PEI_PPI_DESCRIPTOR gPlatformPpiTable[] = { > } > }; > > +/** > + Return the Platform specific PPIs > + > + This function exposes the N1Sdp Specific PPIs. > + > + @param[out] PpiListSize Size in Bytes of the Platform PPI List > + @param[out] PpiList Platform PPI List > + > +**/ > VOID > ArmPlatformGetPlatformPpiList ( > OUT UINTN *PpiListSize,