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 seanga2@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