| Message ID | 1593425623-31810-1-git-send-email-Anson.Huang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] reset: imx7: Support module build | expand |
On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com> wrote: > > Add module device table, module license to support module build. > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > --- > drivers/reset/Kconfig | 4 ++-- > drivers/reset/reset-imx7.c | 4 +++- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > index d9efbfd..033ab60 100644 > --- a/drivers/reset/Kconfig > +++ b/drivers/reset/Kconfig > @@ -65,9 +65,9 @@ config RESET_HSDK > This enables the reset controller driver for HSDK board. > > config RESET_IMX7 > - bool "i.MX7/8 Reset Driver" if COMPILE_TEST > + tristate "i.MX7/8 Reset Driver" > depends on HAS_IOMEM > - default SOC_IMX7D || (ARM64 && ARCH_MXC) > + depends on SOC_IMX7D || (ARM64 && ARCH_MXC) || COMPILE_TEST > select MFD_SYSCON You are dropping the 'default' line, so the driver is now disabled in a defconfig build, which is not mentioned in the patch description. Maybe make it 'default m'? config RESET_IMX7 tristate "i.MX7/8 Reset Driver" depends on HAS_IOMEM depends on SOC_IMX7D || (ARM64 && ARCH_MXC) || COMPILE_TEST default m if (SOC_IMX7D || (ARM64 && ARCH_MXC)) select MFD_SYSCON > @@ -395,3 +396,4 @@ static struct platform_driver imx7_reset_driver = { > }, > }; > builtin_platform_driver(imx7_reset_driver); > +MODULE_LICENSE("GPL v2"); Generally speaking: when you add a MODULE_LICENSE tag, please also add MODULE_AUTHOR and MODULE_DESCRIPTION. The 'builtin_platform_driver()' should work correctly but prevent unloading the module. Ideally please changed to 'module_platform_driver()' and add a .remove function for the platform_driver. Arnd
> Subject: Re: [PATCH 1/3] reset: imx7: Support module build > > On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com> > wrote: > > > > Add module device table, module license to support module build. > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > --- > > drivers/reset/Kconfig | 4 ++-- > > drivers/reset/reset-imx7.c | 4 +++- > > 2 files changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig index > > d9efbfd..033ab60 100644 > > --- a/drivers/reset/Kconfig > > +++ b/drivers/reset/Kconfig > > @@ -65,9 +65,9 @@ config RESET_HSDK > > This enables the reset controller driver for HSDK board. > > > > config RESET_IMX7 > > - bool "i.MX7/8 Reset Driver" if COMPILE_TEST > > + tristate "i.MX7/8 Reset Driver" > > depends on HAS_IOMEM > > - default SOC_IMX7D || (ARM64 && ARCH_MXC) > > + depends on SOC_IMX7D || (ARM64 && ARCH_MXC) || > COMPILE_TEST > > select MFD_SYSCON > > You are dropping the 'default' line, so the driver is now disabled in a defconfig > build, which is not mentioned in the patch description. > > Maybe make it 'default m'? > > config RESET_IMX7 > tristate "i.MX7/8 Reset Driver" > depends on HAS_IOMEM > depends on SOC_IMX7D || (ARM64 && ARCH_MXC) || > COMPILE_TEST > default m if (SOC_IMX7D || (ARM64 && ARCH_MXC)) > select MFD_SYSCON > > > @@ -395,3 +396,4 @@ static struct platform_driver imx7_reset_driver = { > > }, > > }; > > builtin_platform_driver(imx7_reset_driver); > > +MODULE_LICENSE("GPL v2"); > > Generally speaking: when you add a MODULE_LICENSE tag, please also add > MODULE_AUTHOR and MODULE_DESCRIPTION. > OK, will add them in V2. > The 'builtin_platform_driver()' should work correctly but prevent unloading > the module. Ideally please changed to 'module_platform_driver()' and add > a .remove function for the platform_driver. > The reset driver normally won't be removed since it is necessary for drivers which need it, it is just for Android GKI support, in this case, do we need to change it to module_platform_driver()? Thanks, Anson
On Mon, Jun 29, 2020 at 12:45 PM Anson Huang <anson.huang@nxp.com> wrote: > > Subject: Re: [PATCH 1/3] reset: imx7: Support module build > > On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com> wrote: > > The reset driver normally won't be removed since it is necessary for drivers which > need it, it is just for Android GKI support, in this case, do we need to change it to > module_platform_driver()? Please at least try to do it, or explain in the changelog what went wrong if it doesn't work. I don't think "because Android GKI" should ever be the sole justification for a patch. Making drivers loadable module is a good idea regardless of GKI, and if you do that, then make it behave like any other loadable module. Arnd
Hi, Arnd > Subject: Re: [PATCH 1/3] reset: imx7: Support module build > > On Mon, Jun 29, 2020 at 12:45 PM Anson Huang <anson.huang@nxp.com> > wrote: > > > Subject: Re: [PATCH 1/3] reset: imx7: Support module build On Mon, > > > Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com> wrote: > > > > The reset driver normally won't be removed since it is necessary for > > drivers which need it, it is just for Android GKI support, in this > > case, do we need to change it to module_platform_driver()? > > Please at least try to do it, or explain in the changelog what went wrong if it > doesn't work. > > I don't think "because Android GKI" should ever be the sole justification for a > patch. Making drivers loadable module is a good idea regardless of GKI, and if > you do that, then make it behave like any other loadable module. > OK, will do it in V2, BTW, I think there is nothing need to be done for .remove() callback, can I just skip it or need to add a blank callback anyway? Thanks, Anson
On Mon, Jun 29, 2020 at 1:32 PM Anson Huang <anson.huang@nxp.com> wrote: > > Subject: Re: [PATCH 1/3] reset: imx7: Support module build > > > > On Mon, Jun 29, 2020 at 12:45 PM Anson Huang <anson.huang@nxp.com> > > wrote: > > > > Subject: Re: [PATCH 1/3] reset: imx7: Support module build On Mon, > > > > Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com> wrote: > > > > > > The reset driver normally won't be removed since it is necessary for > > > drivers which need it, it is just for Android GKI support, in this > > > case, do we need to change it to module_platform_driver()? > > > > Please at least try to do it, or explain in the changelog what went wrong if it > > doesn't work. > > > > I don't think "because Android GKI" should ever be the sole justification for a > > patch. Making drivers loadable module is a good idea regardless of GKI, and if > > you do that, then make it behave like any other loadable module. > > > > OK, will do it in V2, BTW, I think there is nothing need to be done for .remove() callback, > can I just skip it or need to add a blank callback anyway? I think if everything is done through devm_() calls you should be able to have no .remove() callback at all, but I have not tried that. Arnd
Hi, Arnd > Subject: Re: [PATCH 1/3] reset: imx7: Support module build > > On Mon, Jun 29, 2020 at 1:32 PM Anson Huang <anson.huang@nxp.com> > wrote: > > > Subject: Re: [PATCH 1/3] reset: imx7: Support module build > > > > > > On Mon, Jun 29, 2020 at 12:45 PM Anson Huang > <anson.huang@nxp.com> > > > wrote: > > > > > Subject: Re: [PATCH 1/3] reset: imx7: Support module build On > > > > > Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com> > wrote: > > > > > > > > The reset driver normally won't be removed since it is necessary > > > > for drivers which need it, it is just for Android GKI support, in > > > > this case, do we need to change it to module_platform_driver()? > > > > > > Please at least try to do it, or explain in the changelog what went > > > wrong if it doesn't work. > > > > > > I don't think "because Android GKI" should ever be the sole > > > justification for a patch. Making drivers loadable module is a good > > > idea regardless of GKI, and if you do that, then make it behave like any > other loadable module. > > > > > > > OK, will do it in V2, BTW, I think there is nothing need to be done > > for .remove() callback, can I just skip it or need to add a blank callback > anyway? > > I think if everything is done through devm_() calls you should be able to have > no .remove() callback at all, but I have not tried that. > OK, I have confirmed that before in other drivers, so I will not have .remove callback() for this driver. Thanks, Anson
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig index d9efbfd..033ab60 100644 --- a/drivers/reset/Kconfig +++ b/drivers/reset/Kconfig @@ -65,9 +65,9 @@ config RESET_HSDK This enables the reset controller driver for HSDK board. config RESET_IMX7 - bool "i.MX7/8 Reset Driver" if COMPILE_TEST + tristate "i.MX7/8 Reset Driver" depends on HAS_IOMEM - default SOC_IMX7D || (ARM64 && ARCH_MXC) + depends on SOC_IMX7D || (ARM64 && ARCH_MXC) || COMPILE_TEST select MFD_SYSCON help This enables the reset controller driver for i.MX7 SoCs. diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c index d170fe6..4660c93 100644 --- a/drivers/reset/reset-imx7.c +++ b/drivers/reset/reset-imx7.c @@ -8,7 +8,7 @@ */ #include <linux/mfd/syscon.h> -#include <linux/mod_devicetable.h> +#include <linux/module.h> #include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/reset-controller.h> @@ -386,6 +386,7 @@ static const struct of_device_id imx7_reset_dt_ids[] = { { .compatible = "fsl,imx8mp-src", .data = &variant_imx8mp }, { /* sentinel */ }, }; +MODULE_DEVICE_TABLE(of, imx7_reset_dt_ids); static struct platform_driver imx7_reset_driver = { .probe = imx7_reset_probe, @@ -395,3 +396,4 @@ static struct platform_driver imx7_reset_driver = { }, }; builtin_platform_driver(imx7_reset_driver); +MODULE_LICENSE("GPL v2");
Add module device table, module license to support module build. Signed-off-by: Anson Huang <Anson.Huang@nxp.com> --- drivers/reset/Kconfig | 4 ++-- drivers/reset/reset-imx7.c | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-)