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: Code-Review+2
(3 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. It actually isn't at all right now, because the "recovery" GPIO isn't installed with resample_at_runtime set. It should be, though, so we can change it to that after this. For that to work it must always have an actual GPIO number defined in the table, though, so I'd suggest to just take out all the "recovery" GPIOs that don't in this patch.
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.
For the GPIOs that had the incorrect values because of ACTIVE_LOW nature of GPIOs, can the entry be […]
We should take the "recovery" out completely if it doesn't need to be resampled (see above). For write-protect, we could do it as you says. But another option would be to also read the write-protect GPIO in depthcharge, and then we wouldn't need the "value read by coreboot" field at all anymore (and the resample_at_runtime flag in depthcharge can go away because it's true for all GPIOs).
https://review.coreboot.org/#/c/32233/1/src/mainboard/google/gru/chromeos.c File src/mainboard/google/gru/chromeos.c:
https://review.coreboot.org/#/c/32233/1/src/mainboard/google/gru/chromeos.c@... PS1, Line 36: get_write_protect_state() ^ !wp_polarity, "write protect"}, I think in this case, leaving it as gpio_get() would be cleaner?