[edk2-devel] [edk2-platforms][PATCH v1 1/2] MinPlatformPkg/BoardAcpiEnableLibNull: Improve maintainability
Nate DeSimone
nathaniel.l.desimone at intel.com
Thu Apr 8 00:29:06 UTC 2021
Hi Michael,
Feedback is line.
Thanks,
Nate
> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Michael
> Kubacki
> Sent: Wednesday, April 7, 2021 11:33 AM
> To: devel at edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu at intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone at intel.com>; Liming Gao
> <gaoliming at byosoft.com.cn>; Dong, Eric <eric.dong at intel.com>
> Subject: [edk2-devel] [edk2-platforms][PATCH v1 1/2]
> MinPlatformPkg/BoardAcpiEnableLibNull: Improve maintainability
>
> From: Michael Kubacki <michael.kubacki at microsoft.com>
>
> The NULL instance of BoardAcpiEnableLib 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 maintainability.
>
> Cc: Chasel Chiu <chasel.chiu at intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone at intel.com>
> Cc: Liming Gao <gaoliming at byosoft.com.cn>
> Cc: Eric Dong <eric.dong at intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki at microsoft.com>
> ---
>
> Platform/Intel/MinPlatformPkg/Acpi/Library/{BoardAcpiLibNull/BoardAcpiEn
> ableLib.c => BoardAcpiEnableLibNull/BoardAcpiEnableLibNull.c} | 4 +---
> Platform/Intel/MinPlatformPkg/Acpi/Library/{BoardAcpiLibNull =>
> BoardAcpiEnableLibNull}/BoardAcpiEnableLibNull.inf | 12 ++++------
> --
> Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> | 4 ++--
> 3 files changed, 7 insertions(+), 13 deletions(-)
>
> diff --git
> a/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiE
> nableLib.c
> b/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/Boar
> dAcpiEnableLibNull.c
> similarity index 73%
> rename from
> Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiEna
> bleLib.c
> rename to
> Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/Board
> AcpiEnableLibNull.c
> index 334cb21714ac..494389ae5dc8 100644
> ---
> a/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiE
> nableLib.c
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/
> +++ BoardAcpiEnableLibNull.c
> @@ -5,10 +5,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
>
> -#include <Library/BoardAcpiTableLib.h>
> +#include <Base.h>
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 <Library/BoardAcpiEnableLib.h> -#include <Library/PcdLib.h> -
> #include <Library/DebugLib.h>
>
> EFI_STATUS
> EFIAPI
> diff --git
> a/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiE
> nableLibNull.inf
> b/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/Boar
> dAcpiEnableLibNull.inf
> similarity index 67%
> rename from
> Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiEna
> bleLibNull.inf
> rename to
> Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/Board
> AcpiEnableLibNull.inf
> index 54985ee50d5f..90bed3760e2a 100644
> ---
> a/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiE
> nableLibNull.inf
> +++ b/Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/
> +++ BoardAcpiEnableLibNull.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.<BR>
> +# Copyright (c) Microsoft Corporation.<BR>
> #
> # SPDX-License-Identifier: BSD-2-Clause-Patent # @@ -9,20 +10,15 @@
>
> [Defines]
> INF_VERSION = 0x00010005
> - BASE_NAME = BoardAcpiEnableLib
> + BASE_NAME = BoardAcpiEnableLibNull
> FILE_GUID = EAF8AA6D-549A-40CC-8FB2-3A1FFEC41601
> MODULE_TYPE = BASE
> VERSION_STRING = 1.0
> LIBRARY_CLASS = BoardAcpiEnableLib
>
> -[LibraryClasses]
> - BaseLib
> - PcdLib
> - DebugLib
> -
> [Packages]
> MinPlatformPkg/MinPlatformPkg.dec
> MdePkg/MdePkg.dec
>
> [Sources]
> - BoardAcpiEnableLib.c
> + BoardAcpiEnableLibNull.c
> diff --git a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> index 998ee7909568..da27aa1c4227 100644
> --- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> +++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> @@ -79,7 +79,7 @@ [LibraryClasses.common]
>
>
> BoardInitLib|MinPlatformPkg/PlatformInit/Library/BoardInitLibNull/BoardInit
> LibNull.inf
>
> BoardAcpiTableLib|MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAc
> piTableLibNull.inf
> -
> BoardAcpiEnableLib|MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardA
> cpiEnableLibNull.inf
> +
> +
> BoardAcpiEnableLib|MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/
> + BoardAcpiEnableLibNull.inf
>
> SiliconPolicyInitLib|MinPlatformPkg/PlatformInit/Library/SiliconPolicyInitLibN
> ull/SiliconPolicyInitLibNull.inf
>
> SiliconPolicyUpdateLib|MinPlatformPkg/PlatformInit/Library/SiliconPolicyUp
> dateLibNull/SiliconPolicyUpdateLibNull.inf
>
> @@ -150,7 +150,7 @@ [Components]
> MinPlatformPkg/Acpi/AcpiSmm/AcpiSmm.inf
> MinPlatformPkg/Acpi/AcpiSmm/AcpiStandaloneMm.inf
> MinPlatformPkg/Acpi/Library/DxeAslUpdateLib/DxeAslUpdateLib.inf
> - MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiEnableLibNull.inf
> +
> +
> MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/BoardAcpiEnableLibN
> + ull.inf
> MinPlatformPkg/Acpi/Library/BoardAcpiLibNull/BoardAcpiTableLibNull.inf
>
> MinPlatformPkg/Acpi/Library/MultiBoardAcpiSupportLib/DxeMultiBoardAcpi
> SupportLib.inf
>
> MinPlatformPkg/Acpi/Library/MultiBoardAcpiSupportLib/SmmMultiBoardAc
> piSupportLib.inf
> --
> 2.28.0.windows.1
>
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#73791): https://edk2.groups.io/g/devel/message/73791
> Mute This Topic: https://groups.io/mt/81923537/1767664
> Group Owner: devel+owner at edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [nathaniel.l.desimone at intel.com]
> -=-=-=-=-=-=
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73805): https://edk2.groups.io/g/devel/message/73805
Mute This Topic: https://groups.io/mt/81923537/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list