Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45939 )
Change subject: fw_config: Convert fw_config to a 64-bit field ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45939/1/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/45939/1/src/ec/google/chromeec/ec.c... PS1, Line 854: return -1
I think this should be good. […]
and given the current EC API, I'm not sure we can distinguish between the cases.
https://review.coreboot.org/c/coreboot/+/45939/2/util/sconfig/main.c File util/sconfig/main.c:
https://review.coreboot.org/c/coreboot/+/45939/2/util/sconfig/main.c@406 PS2, Line 406: /* Check that field is within 64 bits. */
do we need to limit field width to 32 bits? […]
Why is that a problem?
https://review.coreboot.org/c/coreboot/+/45939/2/util/sconfig/main.c@554 PS2, Line 554: (uint64_t)(option->value << (uint64_t)field->start_bit)
hmm... looks like you're casting to uint64_t after the bits are truncated: […]
You raise a good point, I think actually perhaps option->value should be a 64-bit number, but I'm not sure if that means the parser needs to be changed as well, need to check on that.