Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/25373 )
Change subject: sdm845: Add UART support ......................................................................
Patch Set 76:
(6 comments)
https://review.coreboot.org/#/c/25373/76/src/soc/qualcomm/sdm845/Kconfig File src/soc/qualcomm/sdm845/Kconfig:
https://review.coreboot.org/#/c/25373/76/src/soc/qualcomm/sdm845/Kconfig@38 PS76, Line 38: Convention is to indent 2 spaces here, not a tab (don't ask me why...)
https://review.coreboot.org/#/c/25373/76/src/soc/qualcomm/sdm845/include/soc... File src/soc/qualcomm/sdm845/include/soc/qcom_qup_se.h:
https://review.coreboot.org/#/c/25373/76/src/soc/qualcomm/sdm845/include/soc... PS76, Line 469: This should already not be in here in the first patch that adds this file, not added and then removed again.
https://review.coreboot.org/#/c/25373/76/src/soc/qualcomm/sdm845/uart.c File src/soc/qualcomm/sdm845/uart.c:
https://review.coreboot.org/#/c/25373/76/src/soc/qualcomm/sdm845/uart.c@76 PS76, Line 76: GENI_STATUS_S_GENI_CMD_ACTIVE_MASK) Fits on one line now? (If not, please align with opening parenthesis.)
https://review.coreboot.org/#/c/25373/76/src/soc/qualcomm/sdm845/uart.c@130 PS76, Line 130: (unsigned char) nit: unnecessary cast (I think?)
https://review.coreboot.org/#/c/25373/76/src/soc/qualcomm/sdm845/uart.c@143 PS76, Line 143: (unsigned int) nit: unnecessary cast
https://review.coreboot.org/#/c/25373/76/src/soc/qualcomm/sdm845/uart.c@148 PS76, Line 148: No space after a cast