Patch Set 10:
That doesn't mean we shouldn't review the Tiger Lake addition. And
I think we all agreed already that a 8k LOC patch isn't reviewable.
And of course, if we had more common code already, it would be much
easier... and we would have a better code base if 3ee54bb hadn't
slipped in. Let's not repeat that error.Sure, i believe Arthur has done great work of taking pain to review this CL. we have address majority of his feedback and few more is there. will continue to address also will see if we can submit this CL into 4 part
1. Bootblock (soc/intel/tigerlake/bootblock/)
2. romstage (soc/intel/tigerlake/romstage/)
3. ramstage (soc/intel/tigerlake/)
4. acpi (soc/intel/tigerlake/acpi/)I still don't think "this CL" is a good idea, even if split up. Any
commit history that first copies stale code and then adapts it in
later commits (instead of adding the adapted code step by step) will
lose lots of information.For instance, I assume some developers have already looked at the
copied code and identified chunks that can stay the same. A copy
first, adapt later commit history will not contain this information.
It would contain details about what was changed, but not about what
wasn't. OTOH, if the commit that adds (pieces of) unchanged code
targets a single topic, and the commit message states that this was
confirmed to work on the new platform, nobody will have to doubt
if that was already checked. And the information will be in the Git
history and can help future maintenance.i understand you are suggesting for incremental patches to create TGL code in upstream. but the philosophy i'm referring is that, we have already reviewed ICL code many time and should't we allow ICL code as is when we are renaming as TGL (this is copy patch logic) and then add only 8 CL to make TGL code complete. Rather we have many CL (definitely more than 8) to even reach at any stage in incremental approach. I agree that incremental approach is easy to review entire code but do we really like to review ICL code again in TGL development?
Well, I don't think ICL code was much reviewed either. Every line got
a +2 at some point, yes. But that doesn't mean half of it wasn't rubber
stamped. And you seem to completely ignore that reviewing includes to
check available documentation (for the newer silicon and FSP).
Yes, you are right in terms of ICL code because of not active chrome product out. But ICL is kind of base for next generation platform hence targeting ICL code.
Thanks to all reviewers help to make ICL code in shape, really appreciate all help here.
If you want to catch subtle difference between ICL and TGL silicon
and FSP interface early, the incremental approach seems so much easier.
If you copy first, please mention in the commit message which parts
were identified to behave exactly the same and which will be adapted
in future commits. Then, reviewers can confirm that. However, if that
requires to look ahead in the patch queue for every other line of code,
you'll likely scare the best reviewers away.
yes, will capture those increment changes expectation in commit msg so we know what we are expecting to get at each CL when we split this one in multiple smaller CL
To view, visit change 36087. To unsubscribe, or for help writing mail filters, visit settings.