On 19 August 2016 at 12:40, Leif Lindholm leif.lindholm@linaro.org wrote:
On Thu, Aug 18, 2016 at 05:58:21PM +0200, Marcin Wojtas wrote:
Unless you're happy to do the latter, I would actually be quite keen to move this discussion to edk2-devel to ensure we don't encure unexpected extra effort at a future migration of OpenPlatformPkg code into an official Tianocore repository.
Do you think that following solution is acceptable - leave 'import patch' as is and add 'adjust to edk2 style' on top of it?
So how are we going to track upstream changes to this library?
Well, the question is - should/are we going to be tracking upstream changes to this library? If not, I'd rather not have added files that will keep triggering PatchCheck errors on every future modification. If we are, I would much prefer us to 'import' this library before build, like we do with OpenSSL. Yes, there is a huge difference in scale, but the situation is similar.
This conversion may be pretty painful and dull, but it's only a question decision, that we would have to obey:)
Well, the point isn't you doing whatever I tell you - the point is which decision is going to leave us with:
- A driver that will be acceptable into a future official edk2 tree.
- A driver that will be likely to benefit from future improvements and bugfixes.
In fact, if there is a public upstream that carries this code, I'd like the git commit hash of the version that we imported in the commit log of this patch.
+1
There is no place to take hash from, as the library exists in no public repository. Mainline linux version of previous controller revision (drivers/net/ethernet/marvell/mvpp2.c) is a merged and mixed version of it with all OS-stuff. Plans about upstreaming support for new generation of it are not yet known, nor the timeframe. Not sure if it's going to be used or not. If yes, I bet David Miller won't accept EDK2 style for sure. Anyway, all this are just hypothesis and endless 'ifs' - for now the first user will be edk2/opp, so we may force others to adjust. Please let me know your thoughts on this.
At which point I'm quite OK with the "importing", as long as it's wrapped into something that does not require components accessing it to have special knowledge of its internal typedefs and macros.
But I'd also be astonished if David Miller accepted the MV_UINT32 type defines.
If there isn't a common upstream for this driver between projects, then they will diverge and whichever ends up with more devs will be the source of patches and hence their code style will be de facto standard. Plus we won't be able to merge GPL licensed contributions anyway, so we can't really be a downstream of the Linux driver.
So in that case, yes, I would prefer EDK2-ising it. But I'd appreciate Ard's input. Either way, we should start by isolating the typedefs and macros into a separate header forming part of 1/3.
I think we should be pragmatic about importing existing code. I care very little about things like STATIC, VOID, and CONST, since they are unconditionally #defined to static, void and const, respectively, anyway.
Also, checkpatch scripts are useful for spotting changes that result in coding style violations in files that were formerly compliant. Taking existing code and then perform blanket search/replace on them for things like the above is a waste of effort imo, especially if there are other changes required that may actually affect code size or correctness in UEFI context, like alignment or struct assignment, things that are forbidden for a reason. If such changes are needed, I would like to see them in a separate patch that goes on top of the original introduction of the new code.