[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <47dcce5a-f438-9f12-5b60-651dc5f2b92c@linaro.org>
Date: Fri, 13 Aug 2021 18:00:00 +0200
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Chunyan Zhang <zhang.lyra@...il.com>,
Thomas Gleixner <tglx@...utronix.de>
Cc: Saravana Kannan <saravanak@...gle.com>,
Baolin Wang <baolin.wang7@...il.com>,
Orson Zhai <orsonzhai@...il.com>,
Chunyan Zhang <chunyan.zhang@...soc.com>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/3] clocksource/drivers/sprd: Add module support to
Unisoc timer
On 15/07/2021 08:54, Chunyan Zhang wrote:
> From: Chunyan Zhang <chunyan.zhang@...soc.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@...il.com>
> Signed-off-by: Chunyan Zhang <chunyan.zhang@...soc.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 ?
> {
> 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.
> +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
Powered by blists - more mailing lists