Re: [PATCH v1 1/1] ShellPkg/PrintInfo.c: Add decoding of SMBIOS record type 43

["Zeng, Star" <star.zeng@intel.com>]

Linson,

Add some comments below.

On 2017/1/9 17:14, Linson Augustine wrote:
> Added decoding of the new SMBIOS Type 43 record.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Augustine Linson P <linson.augustine@hpe.com>
> ---
>  ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c
> index 7e17b69d5a..6eb4e5aada 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/PrintInfo.c
> @@ -3,7 +3,7 @@
>
>    Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.<BR>
>    (C) Copyright 2014 Hewlett-Packard Development Company, L.P.<BR>
> -  (C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>
> +  (C) Copyright 2015-2017 Hewlett Packard Enterprise Development LP<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD License
>    which accompanies this distribution.  The full text of the license may be found at
> @@ -1083,6 +1083,20 @@ SmbiosPrintStructure (
>      break;
>
>    //
> +  // TPM Device (Type 43)
> +  //
> +  case 43:
> +    PRINT_SMBIOS_BIT_FIELD (Struct, &(Struct->Type43->VendorID), VendorID, 4);

1. I prefer to use PRINT_BIT_FIELD (Struct, Type43, VendorID, 4);

> +    PRINT_STRUCT_VALUE_H (Struct, Type43, MajorSpecVersion);
> +    PRINT_STRUCT_VALUE_H (Struct, Type43, MinorSpecVersion);
> +    PRINT_STRUCT_VALUE_H (Struct, Type43, FirmwareVersion1);
> +    PRINT_STRUCT_VALUE_H (Struct, Type43, FirmwareVersion2);
> +    PRINT_SMBIOS_STRING (Struct, Struct->Type43->Description, Description);

2. Similar to 1, I prefer to use PRINT_PENDING_STRING (Struct, Type43, 
Description);

> +    PRINT_SMBIOS_BIT_FIELD (Struct, &(Struct->Type43->Characteristics), Characteristics, 8);

3. Could you create a function similar with DisplayBiosCharacteristics() 
for the Characteristics?

Thanks,
Star

> +    PRINT_STRUCT_VALUE_H (Struct, Type43, OemDefined);
> +    break;
> +
> +  //
>    // Inactive (Type 126)
>    //
>    case 126:
>