Martin Roth 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 configura […]
I mentioned this in another message, but the decision was made not to add it, and a number of boards are already in use without the CBI field. Even if we add it now, we're still going to need to look at the SKU ids unless we decide just not to support those boards.
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: int
What does the return value indicate?
I'll add a comment here.
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 […]
sounds good. will update.
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();
Why is the check for ENV_RAMSTAGE needed at all? variant_devtree_update() gets called only from rams […]
It's not strictly required. I'll be glad to remove it if you want.
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.
Yes, I totally agree. I asked for a fingerprint bit in the FW_CONFIG, but when it was implemented, the decision was made that we didn't need to know that, so it wasn't added. Now we have a number of boards made without the bit showing whether or not we have the fingerprint sensor. It's *VERY* difficult to go back at this point and add that to all those boards. We could look at adding it to new boards going forward, but I think this will probably have to remain.