Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38593 )
Change subject: sc7180: GPIO: Add I2S configuration for sc7180 ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... File src/soc/qualcomm/sc7180/qi2s.c:
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... PS1, Line 28: static void configure_gpios(void) This is mainboard-specific and belongs in ramstage. Please put it in src/mainboard/google/trogdor/mainboard.c.
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... PS1, Line 31: gpio_output(GPIO(49), 1); Do you need to do these when you're doing a gpio_configure() to a special function right afterwards anyway?
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... PS1, Line 36: gpio_configure(GPIO(23), 0, GPIO_PULL_UP, Looks like this one is just a normal enable GPIO, not a real I2S pin? then why the extra gpio_configure() with the pull and the drive strength? Shouldn't gpio_output() be enough for this one?
https://review.coreboot.org/c/coreboot/+/38593/1/src/soc/qualcomm/sc7180/qi2... PS1, Line 38: 1 Please use the actual special function macros for the respective pin (e.g. GPIO49_FUNC_xxx) for these. Looks like function 1 for these three pins is just listed as "reserved", but that clearly seems inaccurate, so please rename those to I2S1_BCLK/I2S1_LRCLK/I2S1_DIN or something like that (in sc7180/include/soc/gpio.h).