Hi Tien Hock, Given Mike's review, could you roll a v7 with all of the incorporated feedback from Mike based on this (instead of submitting the v6 updates as a separate patch, which I previously requested)?
Could you also roll in the following in that patch?: diff --git a/Maintainers.txt b/Maintainers.txt index 876ae5612ad8..47d58ffa0b2c 100644 --- a/Maintainers.txt +++ b/Maintainers.txt @@ -98,6 +98,11 @@ M: Shifei A Lu <shifei.a...@intel.com> M: Xiaohu Zhou <bowen.z...@intel.com> M: Isaac W Oram <isaac.w.o...@intel.com> +Platform/Intel/Stratix10SocPkg +M: Leif Lindholm <leif.lindh...@linaro.org> +M: Michael D Kinney <michael.d.kin...@intel.com> +R: Tien Hock Loh <tien.hock....@intel.com> + Platform/Intel/Tools M: Bob Feng <bob.c.f...@intel.com> M: Liming Gao <liming....@intel.com> Best Regards, Leif On Fri, Aug 09, 2019 at 02:17:42AM +0000, Loh, Tien Hock wrote: > > -----Original Message----- > > From: Kinney, Michael D > > Sent: Friday, August 9, 2019 3:50 AM > > To: Leif Lindholm <leif.lindh...@linaro.org>; Loh, Tien Hock > > <tien.hock....@intel.com>; Kinney, Michael D > > <michael.d.kin...@intel.com> > > Cc: devel@edk2.groups.io; thlo...@gmail.com; Ard Biesheuvel > > <ard.biesheu...@linaro.org> > > Subject: RE: [PATCH v6 1/1] Platform: Intel: Add Stratix 10 platform support > > > > Tien Hock, > > > > I have a few comments: > > > > 1) Recommend change name of directory > > > > Platform/Intel/Startix10 -> Platform/Intel/Startix10SocPkg. > OK will do that. > > > > > 2) S10ClockManager.c is missing file header with license and copyright > > 3) S10ClockManager.h is missing file header with license and copyright > Yeah, I'll submit a fix to that > > > 4) PlatformHookLib.inf uses '..' to access sources in a different directory. > > '..' should never be used in an INF. This INF also lists many > > PCDs that are not used by PlatformHookLib.c > OK I'll remove the dependencies. > > > 5) PlatformHookLib.c also uses '..' in an include that should not > > be used. > > 6) Can the following files be updated to a BSD+Patent license and > > use an SPDX identifier? > > > > IntelPlatformDxe.inf > > IntelPlatformDxe.c > > > > IntelPlatformLib.inf > > Stratix10PlatformLib.c > > Startix10Mmu.c > > ArmPlatformHelper.S > > > OK. Noted, I missed changing these license headers. > > > If S10ClockManager is only used by the PlatformHookLib, then I > > recommend you move the S10ClockManager sources into the > > PlatformHookLib > > directory or a subdirectory below PlatformHookLib. > > > The S10ClockManager is also being used by > Drivers/IntelPlatformDxe/IntelPlatformDxe.c, so I'm wondering what's the best > approach to this? > > > Thanks, > > > > Mike -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#45262): https://edk2.groups.io/g/devel/message/45262 Mute This Topic: https://groups.io/mt/32677389/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
