| Message ID | 1593656074-10092-6-git-send-email-Anson.Huang@nxp.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Support building i.MX8 SoCs clock driver as module | expand |
On Thu, Jul 2, 2020 at 10:20 AM Anson Huang <Anson.Huang@nxp.com> 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@nxp.com> > --- > 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? > +{ > + 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? Regards Aisheng > + > +MODULE_AUTHOR("Aisheng Dong <aisheng.dong@nxp.com>"); > +MODULE_DESCRIPTION("NXP i.MX8QXP LPCG clock driver"); > +MODULE_LICENSE("GPL v2"); > diff --git a/drivers/clk/imx/clk-imx8qxp.c b/drivers/clk/imx/clk-imx8qxp.c > index 5e2903e..9bcf0d1 100644 > --- a/drivers/clk/imx/clk-imx8qxp.c > +++ b/drivers/clk/imx/clk-imx8qxp.c > @@ -151,4 +151,13 @@ static struct platform_driver imx8qxp_clk_driver = { > }, > .probe = imx8qxp_clk_probe, > }; > -builtin_platform_driver(imx8qxp_clk_driver); > + > +static int __init imx8qxp_clk_init(void) > +{ > + return platform_driver_register(&imx8qxp_clk_driver); > +} > +device_initcall(imx8qxp_clk_init); > + > +MODULE_AUTHOR("Aisheng Dong <aisheng.dong@nxp.com>"); > +MODULE_DESCRIPTION("NXP i.MX8QXP clock driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.7.4 >
> 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@nxp.com> > 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@nxp.com> > > --- > > 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
On Thu, Jul 2, 2020 at 11:55 AM Anson Huang <anson.huang@nxp.com> wrote: [...] > > > +{ > > > + 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. > By expanding builtin_platform_driver() marcro, you will find your patch is exactly doing the same thing as buildin_platform_driver() which obivously is unneccesary. #define builtin_platform_driver(__platform_driver) \ builtin_driver(__platform_driver, platform_driver_register) #define builtin_driver(__driver, __register, ...) \ static int __init __driver##_init(void) \ { \ return __register(&(__driver) , ##__VA_ARGS__); \ } \ device_initcall(__driver##_init); If we want to support unload, we need a .remove() callback as current clocks are not allocated by devm_(). If don't support, we probably can use builtin_platform_driver() first and switch to module_platform_driver() in the future once the driver supports release resource properly. Regards Aisheng > 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
> Subject: Re: [PATCH V4 5/5] clk: imx8qxp: Support building i.MX8QXP clock > driver as module > > On Thu, Jul 2, 2020 at 11:55 AM Anson Huang <anson.huang@nxp.com> > wrote: > [...] > > > > +{ > > > > + 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. > > > > By expanding builtin_platform_driver() marcro, you will find your patch is > exactly doing the same thing as buildin_platform_driver() which obivously is > unneccesary. > > #define builtin_platform_driver(__platform_driver) \ > builtin_driver(__platform_driver, platform_driver_register) #define > builtin_driver(__driver, __register, ...) \ > > static int __init __driver##_init(void) \ { \ > return __register(&(__driver) , ##__VA_ARGS__); \ } \ > device_initcall(__driver##_init); > > If we want to support unload, we need a .remove() callback as current clocks > are not allocated by devm_(). > If don't support, we probably can use builtin_platform_driver() first and > switch to module_platform_driver() in the future once the driver supports > release resource properly. > Yes, that is why I use the device_initcall() to make it exactly same as builtin_driver(), and also yes that i.MX clock driver does NOT support module unload, so .remove() is NOT implemented, I am fine with either way, just try to address Arnd's comment. Hi, Arnd What do you think? Do you agree to keep using the builtin_driver()? Thanks, Anson
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) +{ + return platform_driver_register(&imx8qxp_lpcg_clk_driver); +} +device_initcall(imx8qxp_lpcg_clk_init); + +MODULE_AUTHOR("Aisheng Dong <aisheng.dong@nxp.com>"); +MODULE_DESCRIPTION("NXP i.MX8QXP LPCG clock driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/clk/imx/clk-imx8qxp.c b/drivers/clk/imx/clk-imx8qxp.c index 5e2903e..9bcf0d1 100644 --- a/drivers/clk/imx/clk-imx8qxp.c +++ b/drivers/clk/imx/clk-imx8qxp.c @@ -151,4 +151,13 @@ static struct platform_driver imx8qxp_clk_driver = { }, .probe = imx8qxp_clk_probe, }; -builtin_platform_driver(imx8qxp_clk_driver); + +static int __init imx8qxp_clk_init(void) +{ + return platform_driver_register(&imx8qxp_clk_driver); +} +device_initcall(imx8qxp_clk_init); + +MODULE_AUTHOR("Aisheng Dong <aisheng.dong@nxp.com>"); +MODULE_DESCRIPTION("NXP i.MX8QXP clock driver"); +MODULE_LICENSE("GPL v2");
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@nxp.com> --- 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(-)