From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Anderson Date: Wed, 4 Mar 2020 09:54:10 -0500 Subject: [PATCH v5 07/33] clk: Add K210 clock support In-Reply-To: References: <20200228210552.615672-1-seanga2@gmail.com> <20200228210552.615672-8-seanga2@gmail.com> Message-ID: <48181d64-8cd2-550a-d2d9-d3515345ae65@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >> --- > > 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? > :0: warning: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.txt > :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