Messages in this thread | | | From | Chunyan Zhang <> | Date | Fri, 20 Aug 2021 15:46:22 +0800 | Subject | Re: [PATCH v2 3/3] clocksource/drivers/sprd: Add module support to Unisoc timer |
| |
Hi Daniel,
On Sat, 14 Aug 2021 at 00:00, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 15/07/2021 08:54, Chunyan Zhang wrote: > > From: Chunyan Zhang <chunyan.zhang@unisoc.com> > > > > Timers still have devices created for them. So, when compiling a timer > > driver as a module, implement it as a normal platform device driver. > > > > Original-by: Baolin Wang <baolin.wang7@gmail.com> > > Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com> > > --- > > drivers/clocksource/Kconfig | 2 +- > > drivers/clocksource/timer-sprd.c | 15 ++++++++++----- > > 2 files changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > > index eb661b539a3e..a5a5b7c883ec 100644 > > --- a/drivers/clocksource/Kconfig > > +++ b/drivers/clocksource/Kconfig > > @@ -461,7 +461,7 @@ config MTK_TIMER > > Support for Mediatek timer driver. > > > > config SPRD_TIMER > > - bool "Spreadtrum timer driver" if EXPERT > > + tristate "Spreadtrum timer driver" if EXPERT > > depends on HAS_IOMEM > > depends on (ARCH_SPRD || COMPILE_TEST) > > default ARCH_SPRD > > diff --git a/drivers/clocksource/timer-sprd.c b/drivers/clocksource/timer-sprd.c > > index 430cb99d8d79..a8a7d3ea3464 100644 > > --- a/drivers/clocksource/timer-sprd.c > > +++ b/drivers/clocksource/timer-sprd.c > > @@ -5,6 +5,8 @@ > > > > #include <linux/init.h> > > #include <linux/interrupt.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > > > #include "timer-of.h" > > > > @@ -141,7 +143,7 @@ static struct timer_of to = { > > }, > > }; > > > > -static int __init sprd_timer_init(struct device_node *np) > > +static int sprd_timer_init(struct device_node *np) > > Does the __init annotation really need to be removed ?
Yes, since sprd_timer_init() would be invoked by platform_timer_probe() which seems not able to be marked as __init.
> > > { > > int ret; > > > > @@ -190,7 +192,7 @@ static struct clocksource suspend_clocksource = { > > .flags = CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_SUSPEND_NONSTOP, > > }; > > > > -static int __init sprd_suspend_timer_init(struct device_node *np) > > +static int sprd_suspend_timer_init(struct device_node *np) > > { > > int ret; > > > > @@ -204,6 +206,9 @@ static int __init sprd_suspend_timer_init(struct device_node *np) > > return 0; > > } > > > > -TIMER_OF_DECLARE(sc9860_timer, "sprd,sc9860-timer", sprd_timer_init); > > -TIMER_OF_DECLARE(sc9860_persistent_timer, "sprd,sc9860-suspend-timer", > > - sprd_suspend_timer_init); > > +TIMER_PLATFORM_DRIVER_BEGIN(sprd_timer) > > +TIMER_MATCH("sprd,sc9860-timer", sprd_timer_init) > > +TIMER_MATCH("sprd,sc9860-suspend-timer", sprd_suspend_timer_init) > > +TIMER_PLATFORM_DRIVER_END(sprd_timer); > > Please replace the above by something like: > > TIMER_PLATFORM_DECLARE(sc9860_timer, > "sprd,sc9860-timer", > sprd_timer_init); > > TIMER_PLATFORM_DECLARE(sc9860_persistent_timer, > "sprd,sc9860-suspend-timer", > sprd_suspend_timer_init); > > Without TIMER_PLATFORM_DRIVER_BEGIN/END, and if possible the > MODULE_DESCRIPTION/LICENSE in the TIMER_PLATFORM_DECLARE macro itself. > The module description could be the first argument of the timer platform > declaration.
I hope I got your point, will address this in the next version. Let's see if changes are what you want then.
Thanks for your review, Chunyan
> > > +MODULE_DESCRIPTION("Unisoc broadcast timer module"); > > +MODULE_LICENSE("GPL"); > > > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog
| |