Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47309 )
Change subject: mb/google/zork: Add func to determine fingerprint gpio init ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/47309/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47309/2//COMMIT_MSG@9 PS2, Line 9: Because fingerprint sensor isn't a CBI field, it needs to be determined : by board and SKU. Why are we not adding a FW_CONFIG field for it? If firmware needs information about it for configuration, then it should be allocated space in the FW_CONFIG field.
https://review.coreboot.org/c/coreboot/+/47309/2/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/47309/2/src/mainboard/google/zork/v... PS2, Line 34: init Looking at the way this function is used, I think it should be named variant_has_fingerprint() just like the functions on lines 60-64.
https://review.coreboot.org/c/coreboot/+/47309/2/src/mainboard/google/zork/v... PS2, Line 34: int What does the return value indicate?
https://review.coreboot.org/c/coreboot/+/47309/2/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/berknip/variant.c:
https://review.coreboot.org/c/coreboot/+/47309/2/src/mainboard/google/zork/v... PS2, Line 10: if (ENV_RAMSTAGE) { : cfg = (struct soc_amd_picasso_config *)config_of_soc();
Meant to mark as resolved.
Why is the check for ENV_RAMSTAGE needed at all? variant_devtree_update() gets called only from ramstage already.
https://review.coreboot.org/c/coreboot/+/47309/2/src/mainboard/google/zork/v... PS2, Line 21: BERKNIP_SKU0_ID 0x5A030020 I don't think we should use SKU_ID for this. This defeats the purpose of having FW_CONFIG.