Re: [edk2-devel] [PATCH v1 06/10] DynamicTablesPkg: Add Configuration Manager Object parser

["Joey Gouly" <joey.gouly@arm.com>]

Hi,

This looks good to me!

[...]

> +
> +/** A parser for EArmObjFixedFeatureFlags.
> +*/
> +STATIC CONST CM_OBJ_PARSER CmArmFixedFeatureFlagsParser[] = {
> +  {"Flags", 4, "0x%x", NULL}
> +};
> +
> +/** A parser for EArmObjItsGroup.
> +*/
> +STATIC CONST CM_OBJ_PARSER CmArmItsGroupNodeParser[] = {
> +  {"GTBlockTimerFrameToken", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL},

This should just be Token, not GTBlockTimerFrameToken.

> +  {"ItsIdCount", 4, "0x%x", NULL},
> +  {"ItsIdToken", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL}
> +};
> +

[...]

> diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.h b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.h
> new file mode 100644
> index 000000000000..e229df7095d9
> --- /dev/null
> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.h

[...]

> +/**
> +  The CM_OBJ_PARSER structure describes the fields of an CmObject and
> +  provides means for the parser to interpret and trace appropriately.
> +
> +  ParseAcpi() uses the format string specified by 'Format' for tracing
> +  the field data.
> +*/
> +typedef struct CmObjParser CM_OBJ_PARSER;
> +struct CmObjParser {
> +
> +  /// String describing the Cm Object
> +  CONST CHAR8*            NameStr;
> +
> +  /// The length of the field.
> +  UINT32                  Length;
> +
> +  /// Optional Print() style format string for tracing the data. If not
> +  /// used this must be set to NULL.
> +  CONST CHAR8*            Format;
> +
> +  /// Optional pointer to a print formatter function which
> +  /// is typically used to trace complex field information.
> +  /// If not used this must be set to NULL.
> +  /// The Format string is passed to the PrintFormatter function
> +  /// but may be ignored by the implementation code.
> +  FNPTR_PRINT_FORMATTER   PrintFormatter;
> +
> +  /// Optional pointer to print the fields of another CM_OBJ_PARSER
> +  /// structure. This is useful to print sub-structures.
> +  CONST CM_OBJ_PARSER     *SubObjParser;
> +
> +  /// Count of items in the SubObj.
> +  UINTN                   SubObjItemCount;

The SubObjParser doesn't actually seem to be used by any of the objects? (Unless I misread when reading the list of them..)

> +
> +  /// Count of items
> +  UINTN                 ItemCount;
> +} CM_OBJ_PARSER_ARRAY;
> +
> +#endif // CONFIGURATION_MANAGER_OBJECT_PARSER_H_
> diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/TableHelperLib.inf b/DynamicTablesPkg/Library/Common/TableHelperLib/TableHelperLib.inf
> index 5435f74aa0b8..abbf4bc38cab 100644
> --- a/DynamicTablesPkg/Library/Common/TableHelperLib/TableHelperLib.inf
> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/TableHelperLib.inf
> @@ -15,6 +15,8 @@ [Defines]
>    LIBRARY_CLASS  = TableHelperLib
>
>  [Sources]
> +  ConfigurationManagerObjectParser.c
> +  ConfigurationManagerObjectParser.h
>    TableHelper.c
>
>  [Packages]

Need to update the copyright year.

Otherwise:
  Reviewed-by: Joey Gouly <joey.gouly@arm.com>

Thanks,
Joey
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.