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:
(5 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/54639/comment/d7d696f6_b2755899 PS4, Line 7: mb/google/guybrush: Add helpers for fw_config settings.
Nit: Could you please remove the dot/period at the end of the commit message summary.
Done
File src/mainboard/google/guybrush/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/54639/comment/871ac844_532646de PS1, Line 38: #define GUYBRUSH_WCN6856_WIFI 0x00
If you look at the settings in devicetree, the WLAN settings are: […]
Removed wifi. Aligned WWAN to match the CBI fields.
File src/mainboard/google/guybrush/variants/guybrush/helpers.c:
https://review.coreboot.org/c/coreboot/+/54639/comment/68ef597b_9c78b80a PS4, Line 32: if (config == UNDEFINED_FW_CONFIG || ! fw_config_probe(FW_CONFIG(FP, FP_PRESENT)))
That series is already +2ed. […]
For now I'm going to keep just doing the manual probe. I don't think the devicetree method is better in these cases unless we have global CBI fields.
If you think I'm wrong, let's talk about it more when I'm back in the office.
https://review.coreboot.org/c/coreboot/+/54639/comment/4bd41b38_0698ca23 PS4, Line 44: get_variant_wwan_type
BTW, I think I used the wrong part `WWAN_LG850GL` for the probe statement above. […]
You mention having this live in guybrush baseboard, but I don't see how that works if not every platform has WLAN and WWAN_FM350GL defined. Since we decided that fw_config would not be universal, that doesn't seem like it works.
Why is it preferable to go find the device then seeing whether it's enabled or disabled to just calling fw_config_probe?
I can see how it might be better in some cases, like if you can just rely on the enable/disable functions to do everything, but if you can't, like in this case, it just makes things harder, right?
Am I missing something?
https://review.coreboot.org/c/coreboot/+/54639/comment/7c93ab4a_f618e8e0 PS4, Line 60: get_variant_wifi_type
You're right, I don't need it right now, so I'll remove it.
Done
-
Martin Roth (Code Review)