Vinod Polimera has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39613 )
Change subject: Sc7180: Add display 10nm phy & pll programming support ......................................................................
Patch Set 31:
(9 comments)
addressed the comments
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi_phy.c:
https://review.coreboot.org/c/coreboot/+/39613/26/src/soc/qualcomm/sc7180/di... PS26, Line 92: DSI_LaneID_Max
No, this is not what I meant. […]
Done
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi_phy.c:
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... PS28, Line 95: uint32_t decdiv_start;
There are still plenty of fields (like this one) in here that aren't used in the code at all (anymor […]
Done
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... PS28, Line 453: lane_cfg.strength_override = 0;
What do you need these 6 fields for? I don't think you need struct dsi_phy_laneconfig_type at all to […]
Done
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... PS28, Line 495: wmb();
You don't need this wmb() either.
Done
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... PS28, Line 650: phy_cfg->dsi_clksel = 1;
Why do you need this field?
Done
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... PS28, Line 760: printk(BIOS_INFO, "Desired bitclock:%u\n", (uint32_t)desired_bclk);
Please add proper spacing and a unit, e.g. […]
Done
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... PS28, Line 780: phy_cfg.escape_freq = 19;
Why do you need this field?
Done
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... File src/soc/qualcomm/sc7180/display/dsi_phy_pll.c:
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... PS28, Line 34: };
A lot of unused fields in this file too (e.g. all the ssc_ stuff?). […]
Done
https://review.coreboot.org/c/coreboot/+/39613/28/src/soc/qualcomm/sc7180/di... PS28, Line 59: static inline u64 div_u64(u64 dividend, u32 divisor)
Why do we have these two functions, anyway? I don't really see what they add. Why write […]
Done