Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210701085159.208143-1-zhang.lyra@gmail.com> <20210701085159.208143-4-zhang.lyra@gmail.com>
In-Reply-To: <20210701085159.208143-4-zhang.lyra@gmail.com>
From:   Saravana Kannan <saravanak@google.com>
Date:   Thu, 1 Jul 2021 12:17:33 -0700
Message-ID: <CAGETcx-5Y6g3yO1Xs0=+WEsr5Hw7_6W6XRnseC7u5SfKAXbpCA@mail.gmail.com>
Subject: Re: [PATCH 3/3] clocksource/drivers/sprd: Add module support to
 Unisoc timer
To:     Chunyan Zhang <zhang.lyra@gmail.com>
Cc:     Daniel Lezcano <daniel.lezcano@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Baolin Wang <baolin.wang7@gmail.com>,
        Orson Zhai <orsonzhai@gmail.com>,
        Chunyan Zhang <chunyan.zhang@unisoc.com>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jul 1, 2021 at 1:52 AM Chunyan Zhang <zhang.lyra@gmail.com> 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, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 39aa21d01e05..9f16c2779edb 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -447,7 +447,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..73c7b3f8c901 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)
>  {
>         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,15 @@ static int __init sprd_suspend_timer_init(struct device_node *np)
>         return 0;
>  }
>
> +#ifdef MODULE
> +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);
> +MODULE_DESCRIPTION("Unisoc broadcast timer module");
> +MODULE_LICENSE("GPL");
> +#else
>  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);
> +#endif

Would it be a problem if you removed the ifdef and dropped these
TIMER_OF_DECLARE? Doesn't look like either of these timers are needed
for the early scheduler timer.

-Saravana