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 [off-list ref]
wrote:

quoted

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 <redacted>
---
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 = {

quoted

        .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

quoted

+{
+       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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel