Attention is currently required from: Raul Rangel, Furquan Shaikh, Paul Menzel, Karthik Ramasubramanian. Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54639 )
Change subject: mb/google/guybrush: Add helpers for cbi fw_config settings. ......................................................................
Patch Set 5:
(1 comment)
File src/mainboard/google/guybrush/variants/guybrush/helpers.c:
https://review.coreboot.org/c/coreboot/+/54639/comment/0677250d_23dafd29 PS4, Line 44: get_variant_wwan_type
The helper functions at the baseboard level don't need to use fw_config at all. […]
Furquan, I understand what you're saying. I just disagree with the approach. You're making things more complex than the need to be.
The problem with variant_has_fpmcu() is that it's getting called before anything is initialized - that was what was causing the initial failure. The serial port is not configured, and probing it didn't work.
So for FPMCU, it's always just going to be whatever the fw_config says it is. It's either on in fw_config and there's a fingerprint sensor, or it's not present. There's no need to go look at the device in devicetree. If we had global fw_config, this wouldn't even be an issue, but because you didn't like that idea, now we need to go with a more complex solution of probing devices in devicetree.
It would be far easier, in my opinion to just fix fw_config to expect being probed for things that didn't exist on that platform and return that they were disabled.
What I'm saying is that your solution adds a lot of additional runtime complexity that isn't needed on a per platform basis without really helping in the bootblock where I need this code to be.
I'm sure I'm going to be on the losing end of this argument, but I don't think it's a good solution for this problem.
Is there a design doc that we can look at and comment on?