Re: [PATCH v3 3/3] ArmPkg: add Universal/Smbios as a generic SMBIOS library

["Rebecca Cran" <rebecca@nuviainc.com>]

On 11/19/20 5:58 AM, Leif Lindholm wrote:


I hope I've addressed these and your other feedback in the new patch 
series I'll post soon.

> Please use --stat=1000 --stat-graph-width=20 when generating
> patches, to avoid above ellipses.
I'll try and remember in future.
> What version? FirmwareVersion? Would make sense to be explicit in the
> name.
I've removed the driver following Ard's feedback.

> Where is this function defined?
It's defined in the platform-specific OemMiscLib implementation.
> Changes to this file in general seem to be removing unused(?) existing
> cache-related macros and adding (architectural) special
> purposeregister descriptions as replacements. This feels like two
> separate changes (and hence patches).

Agreed. I'll split it.

> Eew.
> This is quite possibly very useful, but that '&' made me swear out
> loud. Add a comment describing how this is used further down?
I hope I've addressed this sufficiently in my new revision.
>> +
>> +  MISC_SMBIOS_TABLE_EXTERNS (SMBIOS_TABLE_TYPE0,  MiscBiosVendor, MiscBiosVendor)
> Why the indentation?
I have no idea. I've fixed it.
>> +#define MAX_HANDLE_COUNT  0x10
> Why?
> Does this belong in a generic header?
It's an arbitrary value, since the SMBIOS protocol doesn't have a way of 
letting you know how many records of a certain type are available 
without iterating through them. It might be better to just iterate twice.


-- 
Rebecca Cran