On 3/4/20 1:58 AM, Rick Chen wrote: > Hi Sean > >> Due to the large number of clocks, I decided to use the CCF. The overall >> structure is modeled after the imx code. Clocks are stored in several >> arrays. There are some translation macros (FOOIFY()) which allow for more >> dense packing. A possible improvement could be to only store the >> parameters we need, instead of the whole CCF struct. >> >> Signed-off-by: Sean Anderson <sean...@gmail.com> >> --- > > Please checkpatch and fix > total: 4 errors, 4 warnings, 18 checks, 662 lines checked > > Thanks > Rick >
Here is the output of checkpatch > drivers/clk/kendryte/clk.c:82: warning: static const char * array should > probably be static const char * const > drivers/clk/kendryte/clk.c:83: warning: static const char * array should > probably be static const char * const These arrays can't have both consts because it needs to have the actual name of the in0 clock added. > drivers/clk/kendryte/clk.c:122: check: Please use a blank line after > function/struct/union/enum declarations This is due to using macros in the style #define FOO_LIST \ FOO(bar) \ FOO(baz) #define FOO(x) FOO_##x, enum foo_ids { FOO_LIST }; #undef FOO I think sticking the undefinition of FOO immediately after the closing enum bracket makes it clearer that FOO is only used with that definition during the declaration of that enum. It is closing the scope, so to speak. If you'd like I can add a newline after enums declared this way. > drivers/clk/kendryte/clk.c:124: error: space prohibited before open square > bracket '[' This is due to macros declared like #define FOO(x) [FOO_##x] = { \ .y = (x), \ } Where there is clearly a space before the [, but it is necessary for the macro. I could declare it like #define FOO(X) \ [FOO_##x] = { \ .y = (x), \ } but I think that the former style is more elegant. However, this can also be changed if needed. > drivers/clk/kendryte/clk.c:133: check: Please use a blank line after > function/struct/union/enum declarations > drivers/clk/kendryte/clk.c:180: check: Please use a blank line after > function/struct/union/enum declarations > drivers/clk/kendryte/clk.c:182: error: space prohibited before open square > bracket '[' > drivers/clk/kendryte/clk.c:189: check: Please use a blank line after > function/struct/union/enum declarations > drivers/clk/kendryte/clk.c:208: check: Please use a blank line after > function/struct/union/enum declarations > drivers/clk/kendryte/clk.c:210: error: space prohibited before open square > bracket '[' > drivers/clk/kendryte/clk.c:210: check: Macro argument reuse 'parents' - > possible side-effects? No possible side-effects here, since this macro argument doesn't make sense unless it is a compile-time constant. > drivers/clk/kendryte/clk.c:220: check: Please use a blank line after > function/struct/union/enum declarations > drivers/clk/kendryte/clk.c:230: check: Please use a blank line after > function/struct/union/enum declarations > drivers/clk/kendryte/clk.c:235: check: Please use a blank line after > function/struct/union/enum declarations > drivers/clk/kendryte/clk.c:241: check: Macro argument reuse 'id' - possible > side-effects? > drivers/clk/kendryte/clk.c:249: check: Macro argument reuse 'id' - possible > side-effects? > drivers/clk/kendryte/clk.c:292: check: Please use a blank line after > function/struct/union/enum declarations > drivers/clk/kendryte/clk.c:306: check: Please use a blank line after > function/struct/union/enum declarations > drivers/clk/kendryte/clk.c:329: error: do not initialise statics to false > drivers/clk/kendryte/clk.c:361: check: Macro argument reuse 'clocks' - > possible side-effects? > drivers/clk/kendryte/clk.c:386: warning: line over 80 characters > drivers/clk/kendryte/clk.c:397: warning: line over 80 characters Unfortunately, I don't see any way to keep these two lines under 80 characters without seriously sacrificing readability. For reference, the lines look like clk_dm(K210_CLK_PLL2, clk_register_composite_struct("pll2", pll2_sels, ARRAY_SIZE(pll2_sels), &k210_clk_comps[COMPIFY(K210_CLK_PLL2)])); The only way to further reduce the length would be to split the array access over two lines, which I think harms readability too much. > drivers/clk/kendryte/clk.c:399: check: Macro argument reuse 'id' - possible > side-effects? > drivers/clk/kendryte/clk.c:410: check: Macro argument reuse 'id' - possible > side-effects? > drivers/clk/kendryte/clk.c:438: check: Macro argument reuse 'id' - possible > side-effects? > drivers/clk/kendryte/clk.c:447: check: Macro argument reuse 'id' - possible > side-effects? > <unknown>:0: warning: DT binding docs and includes should be a separate > patch. See: Documentation/devicetree/bindings/submitting-patches.txt > <unknown>:0: warning: DT binding docs and includes should be a separate > patch. See: Documentation/devicetree/bindings/submitting-patches.txt AFAIK U-Boot has no such policy. --Sean