On Tue, Jun 15, 2021 at 2:28 AM Leif Lindholm <leif@...> wrote:
On Fri, Jun 11, 2021 at 21:21:57 +0530, Vikas Singh wrote:
Summary - 1.Configuration Manager(CM) is a common implementation and should not evaluate the SoC version using macro's However CM must directly consume SoC ver string from platfrom who is extending CM services for ACPI table generation.
This tells me nothing about what this patch does.
2.Platforms who extends CM services for themselves must notify their SoC details to CM.
Neither does this.
3.This patch will update the lx2160ardb platform header also with PLAT_SOC_NAME, this will be consumed by CM.
And this sound like it should be a separate patch.
*However* when I look at the code, this does look like a single change. And what is descibed as point 3 is the actual change in the patch. Moreover, this patch addresses a historic horror, in that SVR_LX2160A was defined both in the platform header and in the SoC header (which I'm not saying you had necessarily noticed, but I am suggesting it is added to the message).
If I wrote this commit message, I would start with a slightly tweaked subject line:
Platform/NXP: Make SoC version log in ConfigurationManager generic
(CM is not a recognised abbreviation, so should be printed expanded)
As for the body, I would say something like:
This patch replaces the logic in ConfigurationManager to print platform name based on platform ID with a simple #define. This also removes a duplication of the SVR_LX2160A definition between SoC and platform headers.
No comments on the code itself.
/ Leif
Leif, I will do the suggested changes in subject and body of the commit. Additionally, I will remove all the SVR_* duplicates from the platform headers. Will try to reuse the SVR_* definitions from the SoC headers itself.
