Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31689 )
Change subject: arch/x86/include/arch/acpi.h: Clean-up ......................................................................
Patch Set 3:
(14 comments)
https://review.coreboot.org/#/c/31689/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31689/3//COMMIT_MSG@7 PS3, Line 7: arch/x86/include/arch/acpi.h Too much information, `arch/x86/acpi:` should be enough
https://review.coreboot.org/#/c/31689/3//COMMIT_MSG@7 PS3, Line 7: arch/x86/include/arch/acpi.h: Clean-up This could be more specific. If I didn't miss anything, you are only changing comments? So `Clean up comments` maybe?
https://review.coreboot.org/#/c/31689/3/src/arch/x86/include/arch/acpi.h File src/arch/x86/include/arch/acpi.h:
https://review.coreboot.org/#/c/31689/3/src/arch/x86/include/arch/acpi.h@106 PS3, Line 106: union { /* TODO: Remove obsolete 'u8 resv' */ Why comment and not just do it (in a separate commit)?
https://review.coreboot.org/#/c/31689/3/src/arch/x86/include/arch/acpi.h@108 PS3, Line 108: in upper versions please be more specific, e.g. `since ACPI 2.0c`
https://review.coreboot.org/#/c/31689/3/src/arch/x86/include/arch/acpi.h@484 PS3, Line 484: u8 reserved; /* This value is 0 */ Sounds odd, maybe "Should be 0"? But actually, we wouldn't set anything else anyway, would we? so why a comment at all?
https://review.coreboot.org/#/c/31689/3/src/arch/x86/include/arch/acpi.h@574 PS3, Line 574: /* bits 22-31: reserved ACPI 5.0/6.3 */ Please don't. if anything, makes this something like "reserved since ACPI 5.0" or "reserved in ACPI 5.0+". Keeping this up to date only encourages future noise.
https://review.coreboot.org/#/c/31689/3/src/arch/x86/include/arch/acpi.h@588 PS3, Line 588: /* bits 2-16: reserved ACPI 6.3 */ same, but actually since when? Actually, how can this comment be useful?
https://review.coreboot.org/#/c/31689/3/src/arch/x86/include/arch/acpi.h@600 PS3, Line 600: PM_TABLET = 8, /* ACPI 5.0 & greater */ ah, that's better
https://review.coreboot.org/#/c/31689/3/src/arch/x86/include/arch/acpi.h@614 PS3, Line 614: his value is 0 who cares?
https://review.coreboot.org/#/c/31689/3/src/arch/x86/include/arch/acpi.h@613 PS3, Line 613: u8 version; /* FACS version */ : u8 resv1[3]; /* This value is 0 */ : u32 ospm_flags; /* 64BIT_WAKE_F */ : u8 resv2[24]; /* This value is 0 */ I was surprised to see that you added the earlier comments... please make sure that you are content with the result this time to avoid future noise.
https://review.coreboot.org/#/c/31689/3/src/arch/x86/include/arch/acpi.h@674 PS3, Line 674: (ACPI spec v6.3, Table 18-392) Please just drop the reference, it's not worth to keep this up to date. We have full-text search features in PDF viewers, don't we?
https://review.coreboot.org/#/c/31689/3/src/arch/x86/include/arch/acpi.h@687 PS3, Line 687: /* Generic Error Data Entry (ACPI spec v6.3, Table 18-392) */ same here
https://review.coreboot.org/#/c/31689/3/src/arch/x86/include/arch/acpi.h@713 PS3, Line 713: /* Generic Error Status Block (ACPI spec v6.3, Table 18-391) */ and here
https://review.coreboot.org/#/c/31689/3/src/arch/x86/include/arch/acpi.h@751 PS3, Line 751: /* These types are defined in ACPI 6.3 section 9.14 */ um, please just drop it