Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson, Kyösti Mälkki, Fred Reitberger, Yu-Ping Wu, Felix Held. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58956 )
Change subject: [RFC] ChromeOS: Declare CHROMEOS_NO_GPIOS ......................................................................
Patch Set 7:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/58956/comment/049d901d_0a7ab6dc PS1, Line 9: Implementation of fill_lb_gpios() is not about VBOOT
fill_Lb_gpios() is an ugly piece of open-coding. The identifier strings should be enumerations, as the allowed choices come from the history of depthcharge so the specification is depthcharge git history?
No objections there in principle, but the interface is long-entrenched and the coreboot table struct is designed to take strings, etc... overall, I think the effort and churn of replacing it with an enum isn't really worth it. I do however agree that the risk of typos is stupid, and a really easy fix for that would be to just put a bunch of #defines in a file (ideally in commonlib/bsd so both coreboot and depthcharge can share it) to lock down the available strings, e.g.:
#define CHROMEOS_GPIO_LID "lid" #define CHROMEOS_GPIO_SPEAKER_ENABLE "speaker enable" ...etc....
I think that would solve the practical issue with much less churn, so if you have time to spend on a little cleanup here, how about doing that?
(The allowed choices are just specified by historical practice, yes, and technically every board is allowed to make up its own but for practical reasons we have been trying to keep using the same names for the same things across all boards. It does happen from time to time that a new board needs a shared GPIO for something that we don't have an existing concept for yet, though, so the option for easily adding new kinds of pins needs to remain.)
We should be able to remove the fill_lb_gpios. We override them in depthcharge anyway: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/plat...
Well, that looks like a temporary hack for early bring-up code, so I think in general my earlier suggestion still makes sense that usually every Chrome OS board needs fill_lb_gpios some point, and so there should be no option to leave it out (i.e. weak implementation). For bring-up, boards can just write an empty stub to fill out later. (Also, btw, you don't need to handcraft that fake_gpio stuff you have there, we do have new_gpio_low()/new_gpio_high() for that in depthcharge already.)
-
Julius Werner (Code Review)