[PATCH v5 07/33] clk: Add K210 clock support

[Sean Anderson]

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 at 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