On 20/06/2023 17:30, Jani Nikula wrote:
On Thu, 15 Jun 2023, Dnyaneshwar Bhadane <dnyaneshwar.bhadane@xxxxxxxxx> wrote:
Replace all occurences of ADL with ALDERLAKE, TGL with TIGERLAKE,
MTL with METEORLAKE, RKL with ROCKETLAKE, JSL with JASPERLAKE,
KBL with KABYLAKE and SKL with SKYLAKE in platform and subplatform
defines. This way there is a consistent pattern to how platforms
are referred. While the change is minor and could be combined to
have lesser patches, splitting to per subpaltform for easier
cherrypicks, if needed.
First of all, I'll note that changes like these need maintainer acks
before merging. Simple review for correctness is not enough!
While discussing this, there was perhaps a slight preference for moving
towards acronyms for brevity instead of expanding all of them to full
names. It can get a bit unwieldy.
For background, the reasons for having IS_<TLA>_DISPLAY_STEP() are
two-fold: the steppings used to be platform specific, so it made sense
to tie platform and stepping together, and IS_<LONG_NAME>_DISPLAY_STEP()
was considered too long combined.
Nowadays, we've abstracted steppings in code to be independent of
platforms, so we could use IS_<LONG_NAME>() && IS_DISPLAY_STEP(), and
throw out all the IS_<TLA>_DISPLAY_STEP() macros. They're orthogonal
things, and it actually bugs me to have so many platform specific
wrappers for the combos.
If in addition we moved to acronyms, we could have IS_<TLA>() &&
IS_DISPLAY_STEP(), and it would be pretty short and nice.
That said, all of these changes are a lot of churn, so I'd rather not
make them lightly.
I don't have a strong opinion on whether to churn or not. Or for the TLA vs LONG_NAME. As long as we do not mix the two. I think this translates to "If you do something, make it consistent, make it readable and make it tidy.". :)
Historically we were not avoiding churn if we could sense a real gain. In this case we could drop some combo macros, as Jani points out, gain some consistency, what else?
As minimum compact the numerous pointlessly expanded(*) MTL checks into a single platform check. But that is even orthogonal to the renames. Like:
if (IS_MTL_GRAPHICS_STEP(i915, M, STEP_B0, STEP_FOREVER) ||
IS_MTL_GRAPHICS_STEP(i915, P, STEP_B0, STEP_FOREVER))
Into possibly:
if (IS_MTL(i915) && IS_GRAPHICS_STEP(i915, STEP_B0, STEP_FOREVER))
But that could also be:
if (IS_MTL_GRAPHICS_STEP(i915, STEP_B0, STEP_FOREVER))
Regards,
Tvrtko
