Attention is currently required from: Ravi kumar, Paul Menzel, mturney mturney. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56091 )
Change subject: soc/qualcomm/common/usb: Add support for common USB driver ......................................................................
Patch Set 38:
(16 comments)
File src/soc/qualcomm/common/Kconfig:
https://review.coreboot.org/c/coreboot/+/56091/comment/f4b04bcc_51a0f117 PS34, Line 15: endif
Everything you're adding should be within this if-endif block.
Done
https://review.coreboot.org/c/coreboot/+/56091/comment/9d15331f_ad00a966 PS34, Line 31: default n
If you follow my other comments I don't think you'll need any Kconfigs for this. […]
Done
File src/soc/qualcomm/common/include/soc/usb/qmp_usb_phy.h:
https://review.coreboot.org/c/coreboot/+/56091/comment/26d584c9_69f7c5ab PS38, Line 15: Don't put random blank lines in struct definitions.
File src/soc/qualcomm/common/include/soc/usb/qusb_phy.h:
https://review.coreboot.org/c/coreboot/+/56091/comment/18624a30_4705f40b PS38, Line 43: //#define USB3_PCS_PHYSTATUS BIT(6) No commented-out code
https://review.coreboot.org/c/coreboot/+/56091/comment/db7bb656_9d6252f7 PS38, Line 95: same here
File src/soc/qualcomm/common/include/soc/usb/snps_usb_phy.h:
https://review.coreboot.org/c/coreboot/+/56091/comment/a61d72bd_f3b30415 PS38, Line 14: spurious newline
File src/soc/qualcomm/common/usb/qmp_usb_phy.c:
https://review.coreboot.org/c/coreboot/+/56091/comment/92138e1d_163c71ef PS34, Line 7: #if CONFIG(QMPV4_USB_PHY)
Please factor these out into separate files (e.g. a qmpv3_usb_phy.c and a qmpv4_usb_phy.c). […]
Done
File src/soc/qualcomm/common/usb/qmpv3_usb_phy.c:
https://review.coreboot.org/c/coreboot/+/56091/comment/cea2e03d_29780dba PS38, Line 358: = { Don't put the = on a new line.
File src/soc/qualcomm/common/usb/qmpv4_usb_phy.c:
https://review.coreboot.org/c/coreboot/+/56091/comment/cd3c4278_92bb11ce PS38, Line 352: = { same
File src/soc/qualcomm/common/usb/qusb_phy.c:
https://review.coreboot.org/c/coreboot/+/56091/comment/5b51eaa4_6d9327c1 PS38, Line 6: void hs_usb_phy_init(void *board_data); This prototype is already in usb_common.h, just include that instead.
https://review.coreboot.org/c/coreboot/+/56091/comment/7bbf29c7_5b089ca4 PS38, Line 11: same here
https://review.coreboot.org/c/coreboot/+/56091/comment/2fbe873f_dadbec7c PS38, Line 27: Please try to be somewhat consistent with the continuation line indentation.
File src/soc/qualcomm/common/usb/snps_usb_phy.c:
https://review.coreboot.org/c/coreboot/+/56091/comment/495f85be_19ca4712 PS38, Line 33: void hs_usb_phy_init(void *board_data); same here
https://review.coreboot.org/c/coreboot/+/56091/comment/81c5a8c3_e65f1c52 PS38, Line 44: UTMI_PHY_CMN_CTRL_OVERRIDE_EN, This can still fit on the previous line
https://review.coreboot.org/c/coreboot/+/56091/comment/c17dabf0_13253a3f PS38, Line 48: POR, POR); This too (and many more times below).
https://review.coreboot.org/c/coreboot/+/56091/comment/b157b686_25e24932 PS38, Line 73: PARAM_OVRD_MASK, override_data->parameter_override_x2); What about parameter_override_x3?