Attention is currently required from: Raul Rangel, Kyösti Mälkki. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42807 )
Change subject: soc/amd/common: Separate single GPIO programming ......................................................................
Patch Set 10:
(4 comments)
File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42807/comment/bc6999a4_0f5dd237 PS1, Line 203: set_single_gpio
While true, I also find it visually too much like program_gpios.
Ack
File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42807/comment/3263151a_b85c0fca PS4, Line 195: soc_gpio_hook(gpio, mux);
Can we get rid of this? Or replace with some assert to remove those soc_route_sci() calls inside soc […]
yeah, calling soc_route_sci in soc_gpio_hook is rather unexpected. as far as i've seen the code in soc_gpio_hook was added to make sure that if gpio 2 is configured as wake pin (pin mux setting 0) the corresponding sci gets configured. introducing a PAD_NF_SCI macro and using that instead of the PAD_NF + soc_gpio_hook magic for the wake pin would allow removing the soc_gpio_hook. do you want to look into that or do you want me to open in internal ticket to look into that later?
File src/soc/amd/common/block/gpio_banks/gpio.c:
https://review.coreboot.org/c/coreboot/+/42807/comment/92d573b4_ca48f0db PS10, Line 184: gpio_list_ptr
Already ack'd the same comment in patchset #1.
the next patch changes this and since it reduces the noise in this patch, i consider this being a good idea
https://review.coreboot.org/c/coreboot/+/42807/comment/a340ad09_9e71f400 PS10, Line 184: set_single_gpio
Feel free to change it, I don't plan to.
changing this to program_single_gpio sounds like a good idea to me, but i'm ok with doing that in a follow-up patch