Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39612 )
Change subject: sc7180: clock: Add display external clock in coreboot ......................................................................
Patch Set 26:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/39612/15/src/soc/qualcomm/sc7180/cl... PS15, Line 374: int
We would like to keep such cleanups for the next project as we have passed CS.
In this case, considering that existing APIs in this file also use raw integers for return values, I think it makes sense to continue doing that for the new additions. According to my understanding cb_err_t is an option for code to use that wants to, not a requirement.
That said, in general this patch is under review and not yet submitted so "sorry we don't want to touch this anymore" isn't really a valid argument.
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/39612/22/src/soc/qualcomm/sc7180/cl... PS22, Line 214: ;
We would like to take such cleanups in the next project as it does not break any functionality.
Sorry, I don't understand (or agree with) this. We are still actively adding new feature code here so clearly this project isn't "done" yet. We haven't cut a stabilization branch on the Chrome OS side yet either. This is an older function but it is now being reused in a new context, so while I admittedly could've thought of this in the original review already, it becomes more important now (since we want to make sure that no problem during display initialization can prevent the system from booting).
If you're worried about this causing unintended problems I am okay with using a very large timeout here (say 100ms or something), I don't really care about the exact number, I just want to make sure it cannot hang the boot completely.