RE: [PATCH V4 5/5] clk: imx8qxp: Support building i.MX8QXP clock driver as module
> 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
> wrote:
> [...]
> > > > +{
> > > > + return platform_driver_register(_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
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 wrote:
[...]
> > > +{
> > > + return platform_driver_register(_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
RE: [PATCH V4 5/5] clk: imx8qxp: Support building i.MX8QXP clock driver as module
> 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
> 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
> > ---
> > 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(_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
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 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
> ---
> 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(_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 ");
> +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(_clk_driver);
> +}
> +device_initcall(imx8qxp_clk_init);
> +
> +MODULE_AUTHOR("Aisheng Dong ");
> +MODULE_DESCRIPTION("NXP i.MX8QXP clock driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
