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: <20210715065455.392923-1-zhang.lyra@gmail.com> <20210715065455.392923-4-zhang.lyra@gmail.com>
 <47dcce5a-f438-9f12-5b60-651dc5f2b92c@linaro.org>
In-Reply-To: <47dcce5a-f438-9f12-5b60-651dc5f2b92c@linaro.org>
From:   Saravana Kannan <saravanak@google.com>
Date:   Fri, 13 Aug 2021 10:44:42 -0700
Message-ID: <CAGETcx9h-WV+zYzjtyMZ9SCQJiihP4O-1UapBQ=ur-oH4bncLg@mail.gmail.com>
Subject: Re: [PATCH v2 3/3] clocksource/drivers/sprd: Add module support to
 Unisoc timer
To:     Daniel Lezcano <daniel.lezcano@linaro.org>
Cc:     Chunyan Zhang <zhang.lyra@gmail.com>,
        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"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Aug 13, 2021 at 9:00 AM 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/tim=
er-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 =3D {
> >       },
> >  };
> >
> > -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 =3D {
> >       .flags  =3D CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_SUSPEND_NON=
STOP,
> >  };
> >
> > -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 de=
vice_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.

Wrt BEGIN/END macros, Chunyan is just trying to be consistent with
what was done for similar macros for IRQ. If you want two separate
drivers being registered for this case, that's ok, but the macros
should support having multiple compatible strings handled by the same
driver. And to do that, you'd need the BEGIN/END variants.

-Saravana

> 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 =E2=94=82 Open source software for AR=
M SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog