diff mbox series

[04/17] power/reset: vexpress: Support building as a module

Message ID 20200419170810.5738-5-robh@kernel.org (mailing list archive)
State Not Applicable, archived
Headers show
Series Modularizing Versatile Express | expand

Commit Message

Rob Herring (Arm) April 19, 2020, 5:07 p.m. UTC
Enable building the VExpress power-off/reset driver as a module.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Sebastian Reichel <sre@kernel.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/power/reset/Kconfig             |  2 +-
 drivers/power/reset/vexpress-poweroff.c | 10 ++++------
 2 files changed, 5 insertions(+), 7 deletions(-)

Comments

Arnd Bergmann April 20, 2020, 3:23 p.m. UTC | #1
On Sun, Apr 19, 2020 at 7:08 PM Rob Herring <robh@kernel.org> wrote:
>
> Enable building the VExpress power-off/reset driver as a module.

Your change allows loading and unloading the driver, but actually
unloading is a bug with the current implementation, as there is no
'release' handler to undo the _vexpress_register_restart_handler()
function.

It should not be hard to add a release handler, or you could just
mark the function as non-unloadable by only having a module_init()
but no module_exit() function.

I suppose if you do the latter, there should also be a suppress_bind_attrs
flag in the device_driver. This is a preexisting bug.

       Arnd
Rob Herring (Arm) April 20, 2020, 5:23 p.m. UTC | #2
On Mon, Apr 20, 2020 at 10:24 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Sun, Apr 19, 2020 at 7:08 PM Rob Herring <robh@kernel.org> wrote:
> >
> > Enable building the VExpress power-off/reset driver as a module.
>
> Your change allows loading and unloading the driver, but actually
> unloading is a bug with the current implementation, as there is no
> 'release' handler to undo the _vexpress_register_restart_handler()
> function.

And also to save and restore pm_power_off...

> It should not be hard to add a release handler, or you could just
> mark the function as non-unloadable by only having a module_init()
> but no module_exit() function.
>
> I suppose if you do the latter, there should also be a suppress_bind_attrs
> flag in the device_driver. This is a preexisting bug.

I may just drop this patch. I wrote it and then realized I don't need
it as PSCI can be used instead.

Rob
Sudeep Holla April 22, 2020, 7:11 p.m. UTC | #3
On Sun, Apr 19, 2020 at 12:07:57PM -0500, Rob Herring wrote:
> Enable building the VExpress power-off/reset driver as a module.
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep
Sebastian Reichel April 28, 2020, 7:31 p.m. UTC | #4
Hi,

On Mon, Apr 20, 2020 at 12:23:13PM -0500, Rob Herring wrote:
> On Mon, Apr 20, 2020 at 10:24 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Sun, Apr 19, 2020 at 7:08 PM Rob Herring <robh@kernel.org> wrote:
> > >
> > > Enable building the VExpress power-off/reset driver as a module.
> >
> > Your change allows loading and unloading the driver, but actually
> > unloading is a bug with the current implementation, as there is no
> > 'release' handler to undo the _vexpress_register_restart_handler()
> > function.
> 
> And also to save and restore pm_power_off...

I'm fine with providing Acked-by for this for merging through a
different tree or taking it through my tree.

> > It should not be hard to add a release handler, or you could just
> > mark the function as non-unloadable by only having a module_init()
> > but no module_exit() function.
> >
> > I suppose if you do the latter, there should also be a suppress_bind_attrs
> > flag in the device_driver. This is a preexisting bug.
> 
> I may just drop this patch. I wrote it and then realized I don't need
> it as PSCI can be used instead.

So is the driver useless on arm64 and depends can be reduced to
arm32?

-- Sebastian
diff mbox series

Patch

diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index 890380302080..0ba18221ba40 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -182,7 +182,7 @@  config POWER_RESET_VERSATILE
 	  reference boards.
 
 config POWER_RESET_VEXPRESS
-	bool "ARM Versatile Express power-off and reset driver"
+	tristate "ARM Versatile Express power-off and reset driver"
 	depends on ARM || ARM64
 	depends on VEXPRESS_CONFIG
 	help
diff --git a/drivers/power/reset/vexpress-poweroff.c b/drivers/power/reset/vexpress-poweroff.c
index 90cbaa8341e3..8a6d3add894c 100644
--- a/drivers/power/reset/vexpress-poweroff.c
+++ b/drivers/power/reset/vexpress-poweroff.c
@@ -5,6 +5,7 @@ 
  */
 
 #include <linux/delay.h>
+#include <linux/module.h>
 #include <linux/notifier.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -87,6 +88,7 @@  static const struct of_device_id vexpress_reset_of_match[] = {
 	},
 	{}
 };
+MODULE_DEVICE_TABLE(of, vexpress_reset_of_match);
 
 static int _vexpress_register_restart_handler(struct device *dev)
 {
@@ -145,9 +147,5 @@  static struct platform_driver vexpress_reset_driver = {
 		.of_match_table = vexpress_reset_of_match,
 	},
 };
-
-static int __init vexpress_reset_init(void)
-{
-	return platform_driver_register(&vexpress_reset_driver);
-}
-device_initcall(vexpress_reset_init);
+module_platform_driver(vexpress_reset_driver);
+MODULE_LICENSE("GPL v2");