Attention is currently required from: Filip Brozovic, Matt DeVillier, Sean Rhodes.
Angel Pons has posted comments on this change by Filip Brozovic. ( https://review.coreboot.org/c/coreboot/+/86039?usp=email )
Change subject: CFR: Add min/max/step values and hex display flag for number options ......................................................................
Patch Set 2:
(5 comments)
File src/commonlib/include/commonlib/cfr.h:
https://review.coreboot.org/c/coreboot/+/86039/comment/40900226_1e3aebb4?usp... : PS2, Line 94: * CFR_OPTFLAG_NUMBER_HEX: : * Displays a numeric option in hexadecimal instead of decimal notation. : * This flag is only valid for numeric options. Flags are meant to be applicable to all option types. This one is not.
Instead of that, consider adding display format flags to the numeric option itself, which you have to modify anyway.
https://review.coreboot.org/c/coreboot/+/86039/comment/55562134_1c02661f?usp... : PS2, Line 139: uint32_t min; : uint32_t max; : uint32_t step; This changes the structure layout in a non-backwards compatible way. I can't recall if there is a version number anywhere, but there would need to be one.
Are `min` and `max` inclusive or exclusive. It's easier to check if we have to limit the range when both values are inclusive (see .c comment)
File src/drivers/option/cfr.c:
https://review.coreboot.org/c/coreboot/+/86039/comment/ec09b62f_d732af9d?usp... : PS2, Line 121: option->min = min; We should probably check that `min <= max` (both inclusive).
https://review.coreboot.org/c/coreboot/+/86039/comment/ee3159eb_c43b7d05?usp... : PS2, Line 122: 0xffffffff UINT32_MAX
https://review.coreboot.org/c/coreboot/+/86039/comment/1b899414_571028da?usp... : PS2, Line 123: step Let's assume a step size of 0 is equivalent to 1