Attention is currently required from: Felix Singer, Lance Zhao, Tim Wawrzynczak. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60645 )
Change subject: arch/x86/acpi: Replace Add(a,b,c) with ASL 2.0 syntax ......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
Nico, do you have a pointer to somewhere where not mixing syntaxes was discussed? Perhaps I forgot or missed that one.
I'm afraid that was on Gerrit on random patches. It was when the very first patches to update the syntax hit the tree.
IMHO, there are a few ways to approach this:
- Wholesale, per-file changes, ensuring that the resulting AML does not change.
In the past, we had patches that covered all changes in a file that didn't change the AML and a separate one for things that did change AML. It's not always wrong when it changes, for instance there are multiple ways to store the result, Store() vs. last argument to an op. IIRC, with the old syntax the compiler translated things literally while with the new syntax it tries to optimize that. Just an example, the compiler might behave differently by now.
- medium Per-Method or per-Device changes
This sounds reasonable. I guess I wouldn't mind if the surrounding code would look consistent.
- Small, per-operand changes which are nearly trivial to review
Well, I don't doubt that there are trivial changes. But it doesn't seem trivial to decide which are trivial and which are not. The spec says the new ops act like their C equivalents but it already contradicts itself with the precedence rules. And I have doubts that things like &&, || really do the same that we are used to. There may be more such subtleties. Over all, it seems the ASL 2.0 ops do what the ASL designers *believed* their C equivalents would do :D
OTOH, I wouldn't mind keeping per-op changes. Even if we only compare the AML after a dozen changes, the commit message can say so and we'd submit them all at once? The tricky part seems to be to find which hunks result in AML changes and which don't. How the patches that don't result in AML changes are organized doesn't seem to matter that much.