Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32233 )
Change subject: chromeos: clean up "recovery" and "write protect" GPIOs ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/32233/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32233/1//COMMIT_MSG@15 PS1, Line 15: The cached value of the "recovery" GPIO is read only on certain : boards which have a physical recovery switch.
Re. "recovery" isn't read at all: […]
Oh okay... yes, when boards call it in flag_replace() explicitly like that, it is read.
https://review.coreboot.org/#/c/32233/1//COMMIT_MSG@18 PS1, Line 18: Most of these inaccuracies are from : non-inverted values on ACTIVE_LOW GPIOs.
Re. removing "recovery": see my comment response above.
I think we should replace all instances of "recovery" that are actually used for something with "presence", and then change the vboot/depthcharge code to use that flag for the developer mode confirmation instead (I think I mentioned this on a vboot CL somewhere too). Then we can get rid of "recovery". (This should make it easier to expand the system onto boards that use a different GPIO for that confirmation, like the power button on Sarien.)
This almost underlines the fact that we are doing something really strange here -- using a GPIO table to transfer a value from one firmware application to another.
I agree... using a GPIO table to transfer a value from something that isn't actually a GPIO anymore is weird. Would be good to get rid of that case, only use the table for actual GPIOs, sample them all in depthcharge when they're needed and maybe remove the "value" field from lb_gpio (although maybe that last one isn't really worth it).
However I think that would be best to put in a subsequent CL.
Sure, it's fine doing it in more steps, as long as we agree on where we want to end up.
As I mentioned in the offline email, it's also very hard to understand which GPIO needs an initial value and which doesn't; which GPIO is read in depthcharge onward and which isn't.
Another good reason to eliminate one of those two cases entirely.