RE: [PATCH V4 5/5] clk: imx8qxp: Support building i.MX8QXP clock driver as module

[Anson Huang]

> Subject: Re: [PATCH V4 5/5] clk: imx8qxp: Support building i.MX8QXP clock
> driver as module
> 
> On Thu, Jul 2, 2020 at 10:20 AM Anson Huang <Anson.Huang@xxxxxxx>
> wrote:
> >
> > Change configuration to "tristate", use device_initcall() instead of
> > builtin_platform_driver(), add module author, description and license
> > to support building i.MX8QXP clock drivers as module.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@xxxxxxx>
> > ---
> > Changes since V3:
> >         - use device_initcall() instead of builtin_platform_driver();
> >         - add module author/description;
> >         - link common scu/lpcg clock driver to i.MX8QXP clock driver, then
> >           no need to have exports.
> > ---
> >  drivers/clk/imx/Kconfig            |  6 +++---
> >  drivers/clk/imx/Makefile           |  9 ++++-----
> >  drivers/clk/imx/clk-imx8qxp-lpcg.c | 10 +++++++++-
> >  drivers/clk/imx/clk-imx8qxp.c      | 11 ++++++++++-
> >  4 files changed, 26 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig index
> > 1867111..8340dfe 100644
> > --- a/drivers/clk/imx/Kconfig
> > +++ b/drivers/clk/imx/Kconfig
> > @@ -5,8 +5,8 @@ config MXC_CLK
> >         depends on ARCH_MXC
> >
> >  config MXC_CLK_SCU
> > -       bool
> > -       depends on IMX_SCU
> > +       tristate "IMX SCU clock"
> > +       depends on ARCH_MXC && IMX_SCU
> >
> >  config CLK_IMX1
> >           bool "IMX1 CCM Clock Driver"
> > @@ -127,7 +127,7 @@ config CLK_IMX8MQ
> >             Build the driver for i.MX8MQ CCM Clock Driver
> >
> >  config CLK_IMX8QXP
> > -       bool "IMX8QXP SCU Clock"
> > +       tristate "IMX8QXP SCU Clock"
> >         depends on ARCH_MXC && IMX_SCU && ARM64
> >         select MXC_CLK_SCU
> >         help
> > diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile index
> > 17f5d12..79e53f2 100644
> > --- a/drivers/clk/imx/Makefile
> > +++ b/drivers/clk/imx/Makefile
> > @@ -21,15 +21,14 @@ mxc-clk-objs += clk-pll14xx.o  mxc-clk-objs +=
> > clk-sscg-pll.o
> >  obj-$(CONFIG_MXC_CLK) += mxc-clk.o
> >
> > -obj-$(CONFIG_MXC_CLK_SCU) += \
> > -       clk-scu.o \
> > -       clk-lpcg-scu.o
> > -
> >  obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
> >  obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
> >  obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
> >  obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
> > -obj-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
> > +
> > +obj-$(CONFIG_MXC_CLK_SCU) += clk-imx-scu.o clk-imx-lpcg-scu.o
> > +clk-imx-scu-$(CONFIG_CLK_IMX8QXP) += clk-scu.o clk-imx8qxp.o
> > +clk-imx-lpcg-scu-$(CONFIG_CLK_IMX8QXP) += clk-lpcg-scu.o
> > +clk-imx8qxp-lpcg.o
> >
> >  obj-$(CONFIG_CLK_IMX1)   += clk-imx1.o
> >  obj-$(CONFIG_CLK_IMX21)  += clk-imx21.o diff --git
> > a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > index 04c8ee3..5b6648e 100644
> > --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > @@ -231,4 +231,12 @@ static struct platform_driver
> imx8qxp_lpcg_clk_driver = {
> >         .probe = imx8qxp_lpcg_clk_probe,  };
> >
> > -builtin_platform_driver(imx8qxp_lpcg_clk_driver);
> > +static int __init imx8qxp_lpcg_clk_init(void)
> 
> Does __init work for module?

__init is NOT working/necessary for module, but if the driver is built-in, it is better to
have it, checked other drivers which support module build, they all have it, so I think
it is better to keep it especially when the driver supports both built-in and loadable module.

drivers/clk/qcom/gcc-sdm845.c
drivers/clk/qcom/gcc-msm8939.c

> 
> > +{
> > +       return platform_driver_register(&imx8qxp_lpcg_clk_driver);
> > +}
> > +device_initcall(imx8qxp_lpcg_clk_init);
> 
> Any reason to change to device_initcall which looks a bit strange?
> Is it because the following line?
> +obj-$(CONFIG_MXC_CLK_SCU) += clk-imx-scu.o clk-imx-lpcg-scu.o
> But it looks to me they're still two modules. Aren't they?

It is suggested by Arnd to NOT use builtin_platform_driver(), in order to support module
unload, although the clock driver normally does NOT support remove, but it is better to
follow the right way.

The change in Makefile is just to link scu/lpcg library to i.MX8QXP clk driver, so that we can
get rid of exports and below 2 .ko are needed for all i.MX SoCs with SCU inside as per your
saying of i.MX8QXP clock driver can be extended for future SoCs with SCU.

clk-imx-lpcg-scu.ko
clk-imx-scu.ko 

Thanks,
Anson