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 22:
(7 comments)
I'm gonna merge this for now so we can move forward with this patch train, please submit that GPIO pull fix as a separate CL.
https://review.coreboot.org/c/coreboot/+/38593/14/src/mainboard/google/trogd... File src/mainboard/google/trogdor/chromeos.c:
https://review.coreboot.org/c/coreboot/+/38593/14/src/mainboard/google/trogd... PS14, Line 32: gpio_output(GPIO_AMP_ENABLE, 1);
This should be initialized to 0. […]
No longer relevant.
https://review.coreboot.org/c/coreboot/+/38593/10/src/mainboard/google/trogd... File src/mainboard/google/trogdor/mainboard.c:
https://review.coreboot.org/c/coreboot/+/38593/10/src/mainboard/google/trogd... PS10, Line 38: GPIO_PULL_UP,GPIO_8MA, GPIO_OUTPUT);
Please fix these style errors
Done
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. […]
No longer relevant.
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 […]
No longer relevant.
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_config […]
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/38593/6/src/soc/qualcomm/sc7180/qi2... File src/soc/qualcomm/sc7180/qi2s.c:
https://review.coreboot.org/c/coreboot/+/38593/6/src/soc/qualcomm/sc7180/qi2... PS6, Line 16: #include <arch/cache.h> : #include <device/mmio.h> : #include <soc/addressmap.h> : #include <soc/gpio.h> : #include <soc/clock.h> : #include <symbols.h> : #include <assert.h> : #include <gpio.h> : #include <string.h> : #include <soc/qi2s.h>
Looks like you don't use all of those includes. […]
No longer relevant.