Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32460 )
Change subject: mediatek/mt8183: Add SPI GPIO driving setting ......................................................................
Patch Set 9:
(6 comments)
https://review.coreboot.org/#/c/32460/9/src/mainboard/google/kukui/bootblock... File src/mainboard/google/kukui/bootblock.c:
https://review.coreboot.org/#/c/32460/9/src/mainboard/google/kukui/bootblock... PS9, Line 24: gpio_set_spi_driving(CONFIG_BOOT_DEVICE_SPI_FLASH_BUS, SPI_PAD0_MASK, : 10); please move kukui-specific changes to a standalone one unless if that will break something.
i.e., have one CL to support setting SPI driving for 8183, and another CL to set higher driver in kukui.
also, maybe add some explain for how the 10 is calculated.
https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/gpio.c File src/soc/mediatek/mt8183/gpio.c:
https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/gpio.c@136 PS9, Line 136: gpio_set_spi_driving Is this a per-board design or per-soc logic?
If the settings below are only for Kukui then we should move it to mainboard/kukui.
But if it can be applied to all 8183 family then yes we can put it here.
https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/gpio.c@143 PS9, Line 143: ((milliamps >= 2) && (milliamps <= 16)); && has lower precedence so you don't need to quote, i.e..
assert(milliamps >= 2 && milliamps <= 16)
https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/gpio.c@162 PS9, Line 162: should we do some assert here if pad_select is something else?
https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/gpio.c@181 PS9, Line 181: } should we assert on any other values for bus?
https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/include/soc/... File src/soc/mediatek/mt8183/include/soc/gpio.h:
https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/include/soc/... PS9, Line 622: driving driving or milliamps ? the prototype should be consistent.