jonzhang@fb.com has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: set bool type variable matching with key/value pair ......................................................................
Patch Set 3:
(2 comments)
Patch Set 2: Code-Review-1
(2 comments)
https://review.coreboot.org/c/coreboot/+/34634/2/src/drivers/vpd/vpd_fsp.c File src/drivers/vpd/vpd_fsp.c:
https://review.coreboot.org/c/coreboot/+/34634/2/src/drivers/vpd/vpd_fsp.c@2... PS2, Line 26: || value_len != 1
Why does value_len need to be passed in at all? It's not used elsewhere in this function. […]
In case of binary type variable, it is good to check the value length; this will find out situation where "HyperThreading" is set to "12", instead of "1". This function set_upd_bool() is the first function for checking/setting a specific UPD data type, more will be added, and thus keeping the input parameters related to key/value the same makes sense. This group of function is called in the callback as defined in src/drivers/vpd/vpd_decode.h, which has those input parameters related to key/value pair: /* Callback for vpd_decode_string to invoke. */ typedef int vpd_decode_callback( const u8 *key, u32 key_len, const u8 *value, u32 value_len, void *arg);
https://review.coreboot.org/c/coreboot/+/34634/2/src/drivers/vpd/vpd_fsp.c@4... PS2, Line 42: else : return false; : }
There's some controversy in the community about braces, but the current style guide dictates (basica […]
Done