Re: [PATCH V2 4/9] clk: imx: Support building i.MX common clock driver as module

[Stephen Boyd]

Quoting Anson Huang (2020-06-09 00:32:08)
> diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
> index 1af8cff..1291f9b 100644
> --- a/drivers/clk/imx/Makefile
> +++ b/drivers/clk/imx/Makefile
> @@ -1,25 +1,10 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> -obj-$(CONFIG_MXC_CLK) += \
> -       clk.o \
> -       clk-busy.o \
> -       clk-composite-8m.o \
> -       clk-cpu.o \
> -       clk-composite-7ulp.o \
> -       clk-divider-gate.o \
> -       clk-fixup-div.o \
> -       clk-fixup-mux.o \
> -       clk-frac-pll.o \
> -       clk-gate-exclusive.o \
> -       clk-gate2.o \
> -       clk-pfd.o \
> -       clk-pfdv2.o \
> -       clk-pllv1.o \
> -       clk-pllv2.o \
> -       clk-pllv3.o \
> -       clk-pllv4.o \
> -       clk-sscg-pll.o \
> -       clk-pll14xx.o
> +mxc-clk-objs += clk.o clk-busy.o clk-composite-8m.o clk-cpu.o clk-composite-7ulp.o \
> +               clk-divider-gate.o clk-fixup-div.o clk-fixup-mux.o clk-frac-pll.o \
> +               clk-gate-exclusive.o clk-gate2.o clk-pfd.o clk-pfdv2.o clk-pllv1.o \
> +               clk-pllv2.o clk-pllv3.o clk-pllv4.o clk-sscg-pll.o clk-pll14xx.o
> +obj-$(CONFIG_MXC_CLK) += mxc-clk.o

I enjoyed it when the files were all on their own line. Can you keep it
like that and just add mxc-clk-objs += to all the lines and remove the
trailing slash?

>  
>  mxc-clk-scu-objs += clk-scu.o clk-lpcg-scu.o
>  obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> index d2b5af8..73e064b 100644
> --- a/drivers/clk/imx/clk-composite-8m.c
> +++ b/drivers/clk/imx/clk-composite-8m.c
> @@ -243,3 +243,4 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
>         kfree(mux);
>         return ERR_CAST(hw);
>  }
> +EXPORT_SYMBOL_GPL(imx8m_clk_hw_composite_flags);

Are all these files including <linux/export.h>? Because they should
unless they're using something from <linux/module.h> like
MODULE_LICENSE().

> diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> index 87ab8db..cc894b5 100644
> --- a/drivers/clk/imx/clk.c
> +++ b/drivers/clk/imx/clk.c
> @@ -143,16 +148,24 @@ void imx_cscmr1_fixup(u32 *val)
>  static int imx_keep_uart_clocks;
>  static struct clk ** const *imx_uart_clocks;
>  
> -static int __init imx_keep_uart_clocks_param(char *str)
> +static int __maybe_unused imx_keep_uart_clocks_param(char *str)
>  {
>         imx_keep_uart_clocks = 1;
>  
>         return 0;
>  }
> +
> +#ifdef MODULE
> +__setup_param("earlycon", imx_keep_uart_earlycon,
> +             imx_keep_uart_clocks_param);
> +__setup_param("earlyprintk", imx_keep_uart_earlyprintk,
> +             imx_keep_uart_clocks_param);

Why not fix __setup_param() to take the same number of arguments in
either case? That macro looks broken.

> +#else
>  __setup_param("earlycon", imx_keep_uart_earlycon,
>               imx_keep_uart_clocks_param, 0);
>  __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
>               imx_keep_uart_clocks_param, 0);
> +#endif
>  
>  void imx_register_uart_clocks(struct clk ** const clks[])
>  {