Attention is currently required from: Furquan Shaikh, Maulik V Vaghela, Tim Wawrzynczak, Subrata Banik, Meera Ravindranath, Angel Pons, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56168 )
Change subject: drivers/intel/gma: Restructure Opregion version info code ......................................................................
Patch Set 6:
(1 comment)
File src/drivers/intel/gma/opregion.c:
https://review.coreboot.org/c/coreboot/+/56168/comment/be8bb134_30db9457 PS6, Line 261: 2
knowing that, Rev and Rsvd always having 0, can't we use below logic? Thoughts ?
union opregion_version { u32 data; struct fields{ u8 rsvd; u8 revision; u8 minor; u8 major; }; };
+---------------+---------------+------+------+------------------+ | 31:24 | 23:16 | 15:8 | 7:0 | Opregion version | +---------------+---------------+------+------+------------------+ | Major Version | Minor version | Rev | Rsvd | | | 1 | 0 | 0 | 0 | 1.0.0 | | 1 | 5 | 0 | 0 | 1.5.0 | | 2 | 3 | 0 | 0 | 2.3.0 | | A | F | 0 | 0 | 10.15 | +---------------+---------------+------+------+------------------+
config OPREGION_2_0_VERSION int default 2000
config OPREGION_2_1_VERSION int default 2100
config OPREGION_3_0_VERSION int default 3000
These base-10 integers don't result in the desired values. If anything, they should be written in hex as follows:
config OPREGION_2_0_VERSION hex default 0x02000000
config OPREGION_2_1_VERSION int default 0x02010000
config OPREGION_3_0_VERSION int default 0x03000000
IMHO, having boolean Kconfig options and encoding the version number in C code is easier to understand and maintain.