From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web08.307.1617917183391710415 for ; Thu, 08 Apr 2021 14:26:23 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=q/3XAdkg; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [10.124.238.202] (unknown [167.220.2.74]) by linux.microsoft.com (Postfix) with ESMTPSA id E3F7120B5680; Thu, 8 Apr 2021 14:26:22 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com E3F7120B5680 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1617917183; bh=EVToDgQAwQVHZF0xoJayFyX4VfgHctE/4GDuCvPaKvM=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=q/3XAdkgoPYK/CjU1FQWT27UTuwh1CsSPyJtBX6XY/IhD9e0SgQNr4+VhoLiSTw2d DA6pZBznpTRd4WroLUu6DxbELYMeKOR0RrvdPhRJSw5KJzSnKszWMgd0uI5Po8VtOb kc+kk4hoDhGQ59naEQkKOgCJ0hcKxf6BrzemII6I= Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 2/2] MinPlatformPkg/BoardAcpiTableLibNull: Improve maintainability To: devel@edk2.groups.io, nathaniel.l.desimone@intel.com Cc: "Chiu, Chasel" , Liming Gao , "Dong, Eric" References: <20210407183324.1659-1-mikuback@linux.microsoft.com> <20210407183324.1659-3-mikuback@linux.microsoft.com> From: "Michael Kubacki" Message-ID: <4e8d0db2-87bd-fa27-a5e7-ce4a0d2c5916@linux.microsoft.com> Date: Thu, 8 Apr 2021 14:26:22 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Thanks for the feedback. I removed Base.h in a v2 series just sent. On 4/7/2021 5:29 PM, Nate DeSimone wrote: > Hi Michael, > > Feedback is line. > > Thanks, > Nate > >> -----Original Message----- >> From: mikuback@linux.microsoft.com >> Sent: Wednesday, April 7, 2021 11:33 AM >> To: devel@edk2.groups.io >> Cc: Chiu, Chasel ; Desimone, Nathaniel L >> ; Liming Gao >> ; Dong, Eric >> Subject: [edk2-platforms][PATCH v1 2/2] >> MinPlatformPkg/BoardAcpiTableLibNull: Improve maintainability >> >> From: Michael Kubacki >> >> The NULL instance of BoardAcpiTableLib in MinPlatformPkg currently has a >> few organization issues that make it more difficult to find and use than a >> typical NULL library instance. >> >> 1. It shares a directory with another unrelated library instance. >> 2. The directory name "BoardAcpiLibNull" is not directly related to >> either library instance name in the directory. >> 3. The library instance has unnecessary dependencies. >> 4. The BASE_NAME does not indicate the library instance is the NULL >> instance. >> 5. The C source file name does not match the INF file name making >> finding the C source by search more cumbersome than needed. >> >> This change resolves the above issues to improve use and maintainability. >> >> Cc: Chasel Chiu >> Cc: Nate DeSimone >> Cc: Liming Gao >> Cc: Eric Dong >> Signed-off-by: Michael Kubacki >> --- >> >> Platform/Intel/MinPlatformPkg/Acpi/Library/{BoardAcpiLibNull/BoardAcpiTa >> bleLib.c => BoardAcpiTableLibNull/BoardAcpiTableLibNull.c} | 4 +--- >> Platform/Intel/MinPlatformPkg/Acpi/Library/{BoardAcpiLibNull => >> BoardAcpiTableLibNull}/BoardAcpiTableLibNull.inf | 12 ++++-------- >> Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc >> | 4 ++-- >> 3 files changed, 7 insertions(+), 13 deletions(-) >> >> diff --git >> a/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiT >> ableLib.c >> b/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/Board >> AcpiTableLibNull.c >> similarity index 73% >> rename from >> Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiTab >> leLib.c >> rename to >> Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/BoardAc >> piTableLibNull.c >> index e49e6ad44162..0f871b6f07ef 100644 >> --- >> a/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiT >> ableLib.c >> +++ b/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/B >> +++ oardAcpiTableLibNull.c >> @@ -5,10 +5,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent >> >> **/ >> >> +#include > > Any reason for choosing Base.h? From what I see this file uses types that require Uefi/UefiBaseType.h like EFI_STATUS for example. It will still compile fine since BoardAcpiEnableLib.h includes Uefi.h (which includes Uefi/UefiBaseType.h). So I wonder what the value of including only Base.h is here since the file will still have implicit dependencies. > >> #include >> -#include -#include - >> #include >> >> EFI_STATUS >> EFIAPI >> diff --git >> a/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiT >> ableLibNull.inf >> b/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/Board >> AcpiTableLibNull.inf >> similarity index 67% >> rename from >> Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiTab >> leLibNull.inf >> rename to >> Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/BoardAc >> piTableLibNull.inf >> index 04f55b49d5a1..6102897ab67b 100644 >> --- >> a/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiT >> ableLibNull.inf >> +++ b/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/B >> +++ oardAcpiTableLibNull.inf >> @@ -1,7 +1,8 @@ >> ## @file >> -# Component information file for Board Acpi Library >> +# Component information file for NULL instance of the Board ACPI Enable >> +library >> # >> # Copyright (c) 2017, Intel Corporation. All rights reserved.
>> +# Copyright (c) Microsoft Corporation.
>> # >> # SPDX-License-Identifier: BSD-2-Clause-Patent # @@ -9,20 +10,15 @@ >> >> [Defines] >> INF_VERSION = 0x00010005 >> - BASE_NAME = BoardAcpiTableLib >> + BASE_NAME = BoardAcpiTableLibNull >> FILE_GUID = F220FAB7-F8E4-4E7A-A599-D47E2D547956 >> MODULE_TYPE = BASE >> VERSION_STRING = 1.0 >> LIBRARY_CLASS = BoardAcpiTableLib >> >> -[LibraryClasses] >> - BaseLib >> - PcdLib >> - DebugLib >> - >> [Packages] >> MinPlatformPkg/MinPlatformPkg.dec >> MdePkg/MdePkg.dec >> >> [Sources] >> - BoardAcpiTableLib.c >> + BoardAcpiTableLibNull.c >> diff --git a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc >> b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc >> index da27aa1c4227..cf3ff13e7b29 100644 >> --- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc >> +++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc >> @@ -78,7 +78,7 @@ [LibraryClasses.common] >> >> FspWrapperPlatformLib|MinPlatformPkg/FspWrapper/Library/PeiFspWrapp >> erPlatformLib/PeiFspWrapperPlatformLib.inf >> >> >> BoardInitLib|MinPlatformPkg/PlatformInit/Library/BoardInitLibNull/BoardInit >> LibNull.inf >> - >> BoardAcpiTableLib|MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAc >> piTableLibNull.inf >> + >> + >> BoardAcpiTableLib|MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/Bo >> + ardAcpiTableLibNull.inf >> >> BoardAcpiEnableLib|MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/B >> oardAcpiEnableLibNull.inf >> >> SiliconPolicyInitLib|MinPlatformPkg/PlatformInit/Library/SiliconPolicyInitLibN >> ull/SiliconPolicyInitLibNull.inf >> >> SiliconPolicyUpdateLib|MinPlatformPkg/PlatformInit/Library/SiliconPolicyUp >> dateLibNull/SiliconPolicyUpdateLibNull.inf >> @@ -151,7 +151,7 @@ [Components] >> MinPlatformPkg/Acpi/AcpiSmm/AcpiStandaloneMm.inf >> MinPlatformPkg/Acpi/Library/DxeAslUpdateLib/DxeAslUpdateLib.inf >> >> MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/BoardAcpiEnableLibN >> ull.inf >> - MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiTableLibNull.inf >> + >> + MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/BoardAcpiTableLibNul >> + l.inf >> >> MinPlatformPkg/Acpi/Library/MultiBoardAcpiSupportLib/DxeMultiBoardAcpi >> SupportLib.inf >> >> MinPlatformPkg/Acpi/Library/MultiBoardAcpiSupportLib/SmmMultiBoardAc >> piSupportLib.inf >> >> -- >> 2.28.0.windows.1 > > > > >