21 comments:
File src/soc/qualcomm/sc7180/display/dsi_phy.c:
Patch Set #26, Line 92: DSI_LaneID_Max
Please stick to coding style (constants in ALL_CAPS)
Patch Set #26, Line 127: struct dsi_phy_configtype {
Do you need all these structures and do they need all these members? Where's the difference between configtype and configtype_info? You seem to always use them together, can't they just be one structure?
A lot of the fields in here seem pointless, e.g. cphy_mode. You just hardcode it to false in mdss_dsi_phy_pll_setup() and never change it anywhere else. Please remove all these fields and just hardcode their effects directly, you should only have fields in these structures that are actually determined at runtime.
Patch Set #26, Line 168: void mdss_dsi_phy_reset(void);
Same as in other patches.
Patch Set #26, Line 630: enum cb_err
If it cannot fail, just make it void.
Patch Set #26, Line 679: wmb(); /* ensure dsiclk_sel is always programmed before pll start */
write32() already contains explicit DMBs, you don't need to add explicit barriers.
Patch Set #26, Line 804: uint32_t
pointless cast
What is this supposed to do? fval is already unsigned.
KHz
Patch Set #26, Line 1034: {"disp_cc_mdss_esc0_clk", 0, 1, 0, 0, 0, 0},
Please use
.name = "disp_cc_mdss_esc0_clk", .clk_secondary_source = 1, ...
notation to clarify what each number means. (You don't need to explicitly initialize zeroes.)
Patch Set #26, Line 1055: clk_enable
Just put a literal 1 here, there's no point in this being a variable.
Patch Set #26, Line 1058: fialed
typo
File src/soc/qualcomm/sc7180/display/dsi_phy_pll.c:
Patch Set #26, Line 18: #define MHZ_3000 3000000000UL
Please use (3000 * MHz) directly in the code rather than hardcoding constants like this.
Patch Set #26, Line 20: # define do_div(n, base) ( \
Looks like you're not actually using this for what it is, so remove it (you just need a plain _tmp / __d below, not the whole remainder stuff that this macro does, and you should get rid of below macro anyway).
Patch Set #26, Line 30: #define DIV_ROUND_CLOSEST_ULL(x, divisor)( \
We already have a DIV_ROUND_CLOSEST() (in src/commonlib/include/commonlib/helpers.h, chain-included from <types.h>) that should work for all data types. Just use that.
Patch Set #26, Line 98: struct mdss_pll_resources *rsc
unused?
Patch Set #26, Line 183: 19200000
This is SRC_XO_HZ from <soc/clock.h>
Patch Set #26, Line 296: 19200000
This too.
Patch Set #26, Line 306: wmb();
Already done by each write32().
File src/soc/qualcomm/sc7180/include/soc/display/display_resources.h:
Patch Set #26, Line 9: MDP_External_Clock_Entry
Please stick to coding style (type names in lower_case).
Patch Set #26, Line 22: uint32_t clk_pll_m;
Looks like we never use MND for the DSI clocks we have, so do we need these fields?
File src/soc/qualcomm/sc7180/include/soc/display/dsi_phy_pll.h:
Patch Set #26, Line 16: struct mdss_pll_resources {
This is another one of these structs that looks way larger than necessary. Please hardcode any values from here that are always the same in our case, and then remove all members that aren't used.
To view, visit change 39613. To unsubscribe, or for help writing mail filters, visit settings.