diff mbox series

[linux-next,v3,4/4] clocksource/drivers/timer-mediatek: Make timer-mediatek become loadable module

Message ID 20230419074910.10809-5-walter.chang@mediatek.com
State New
Headers
Series Support timer drivers as loadable modules |

Commit Message

Walter Chang (張維哲) April 19, 2023, 7:49 a.m. UTC 
  From: Chun-Hung Wu <chun-hung.wu@mediatek.com>

Make the timer-mediatek driver which can register
an always-on timer as tick_broadcast_device on
MediaTek SoCs become loadable module in GKI.

Tested-by: Walter Chang <walter.chang@mediatek.com>
Signed-off-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>
---
 drivers/clocksource/Kconfig          |  2 +-
 drivers/clocksource/timer-mediatek.c | 39 ++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)

Comments

AngeloGioacchino Del Regno April 19, 2023, 8:10 a.m. UTC | #1 
Il 19/04/23 09:49, walter.chang@mediatek.com ha scritto:
> From: Chun-Hung Wu <chun-hung.wu@mediatek.com>
> 
> Make the timer-mediatek driver which can register
> an always-on timer as tick_broadcast_device on
> MediaTek SoCs become loadable module in GKI.
> 
> Tested-by: Walter Chang <walter.chang@mediatek.com>
> Signed-off-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>

I think I typoed your email when sending the example patch for the
conversion to platform_device. Check [1], it may be better to just
iterate through that? (please ignore the pure_initcall() part, that's
a mistake, it's never gonna happen as it automatically becomes a
module_init() call).

It depends on what maintainers think about that clocksource.h addition,
the patch got zero comments, so if you're interested in that perhaps we
can explicitly ask what would be the best option between yours and mine;
that addition is done only to avoid the big ifdef party that this patch
proposes and makes things a bit shorter if this timer modularization
goes on with more drivers, but I don't have strong opinions anyway.

In the meanwhile, just to eventually speed up integrating this, or the
other patch - I'll still give you a review of this one.

[1]: 
https://patchwork.kernel.org/project/linux-mediatek/patch/20230309132119.175650-1-angelogioacchino.delregno@collabora.com/

> ---
>   drivers/clocksource/Kconfig          |  2 +-
>   drivers/clocksource/timer-mediatek.c | 39 ++++++++++++++++++++++++++++
>   2 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 526382dc7482..a7413ad7b6ad 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -472,7 +472,7 @@ config SYS_SUPPORTS_SH_CMT
>   	bool
>   
>   config MTK_TIMER
> -	bool "Mediatek timer driver" if COMPILE_TEST
> +	tristate "Mediatek timer driver"

While at it, you could also fix the text, Mediatek -> MediaTek

>   	depends on HAS_IOMEM
>   	select TIMER_OF
>   	select CLKSRC_MMIO
> diff --git a/drivers/clocksource/timer-mediatek.c b/drivers/clocksource/timer-mediatek.c
> index 7bcb4a3f26fb..3448848682c0 100644
> --- a/drivers/clocksource/timer-mediatek.c
> +++ b/drivers/clocksource/timer-mediatek.c
> @@ -13,6 +13,9 @@
>   #include <linux/clocksource.h>
>   #include <linux/interrupt.h>
>   #include <linux/irqreturn.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
>   #include <linux/sched_clock.h>
>   #include <linux/slab.h>
>   #include "timer-of.h"
> @@ -337,5 +340,41 @@ static int __init mtk_gpt_init(struct device_node *node)
>   
>   	return 0;
>   }
> +
> +#ifdef MODULE

#ifndef MODULE
... two lines...
#else
... a bunch of lines ...
#endif

looks more readable. I'd go with that.

> +static int mtk_timer_probe(struct platform_device *pdev)
> +{
> +	int (*timer_init)(struct device_node *node);
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	timer_init = of_device_get_match_data(&pdev->dev);
> +	return timer_init(np);
> +}
> +
> +static const struct of_device_id mtk_timer_match_table[] = {
> +	{
> +		.compatible = "mediatek,mt6577-timer",
> +		.data = mtk_gpt_init,

Fits in one line!

> +	},
> +	{
> +		.compatible = "mediatek,mt6765-timer",
> +		.data = mtk_syst_init,

ditto.

> +	},
> +	{}

Always end with { /* sentinel */ }

> +};
> +
> +static struct platform_driver mtk_timer_driver = {
> +	.probe = mtk_timer_probe,
> +	.driver = {
> +		.name = "mtk-timer",

"mediatek-timer" looks nicer :-)

> +		.of_match_table = mtk_timer_match_table,
> +	},
> +};
> +module_platform_driver(mtk_timer_driver);
> +
> +MODULE_DESCRIPTION("MediaTek Module Timer driver");

"MediaTek Timer driver" is enough, "Module" gets misleading if this gets compiled
as built in platform driver (instead of built in timer_of).

> +MODULE_LICENSE("GPL v2");
> +#else
>   TIMER_OF_DECLARE(mtk_mt6577, "mediatek,mt6577-timer", mtk_gpt_init);
>   TIMER_OF_DECLARE(mtk_mt6765, "mediatek,mt6765-timer", mtk_syst_init);
> +#endif
Walter Chang (張維哲) April 20, 2023, 1:46 a.m. UTC | #2 
On Wed, 2023-04-19 at 10:10 +0200, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 19/04/23 09:49, walter.chang@mediatek.com ha scritto:
> > From: Chun-Hung Wu <chun-hung.wu@mediatek.com>
> > 
> > Make the timer-mediatek driver which can register
> > an always-on timer as tick_broadcast_device on
> > MediaTek SoCs become loadable module in GKI.
> > 
> > Tested-by: Walter Chang <walter.chang@mediatek.com>
> > Signed-off-by: Chun-Hung Wu <chun-hung.wu@mediatek.com>
> 
> I think I typoed your email when sending the example patch for the
> conversion to platform_device. Check [1], it may be better to just
> iterate through that? (please ignore the pure_initcall() part, that's
> a mistake, it's never gonna happen as it automatically becomes a
> module_init() call).
> 
> It depends on what maintainers think about that clocksource.h
> addition,
> the patch got zero comments, so if you're interested in that perhaps
> we
> can explicitly ask what would be the best option between yours and
> mine;
> that addition is done only to avoid the big ifdef party that this
> patch
> proposes and makes things a bit shorter if this timer modularization
> goes on with more drivers, but I don't have strong opinions anyway.
> 
> In the meanwhile, just to eventually speed up integrating this, or
> the
> other patch - I'll still give you a review of this one.
> 
> [1]:
> 
https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20230309132119.175650-1-angelogioacchino.delregno@collabora.com/__;!!CTRNKA9wMg0ARbw!kellIP9qWQwZCUsbK9-s9WForDbz_5-CAGdMIzYbGSXgvXjVGnyGmlmGHeKEbXr_URQeKVHnb6eFAh-fBshGD1KTERh-mzSAlQ$

Thank you for providing an alternative implementation to make the timer
driver loadable. We will study whether this solution is feasible.

> 
> > ---
> >   drivers/clocksource/Kconfig          |  2 +-
> >   drivers/clocksource/timer-mediatek.c | 39
> > ++++++++++++++++++++++++++++
> >   2 files changed, 40 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clocksource/Kconfig
> > b/drivers/clocksource/Kconfig
> > index 526382dc7482..a7413ad7b6ad 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -472,7 +472,7 @@ config SYS_SUPPORTS_SH_CMT
> >       bool
> > 
> >   config MTK_TIMER
> > -     bool "Mediatek timer driver" if COMPILE_TEST
> > +     tristate "Mediatek timer driver"
> 
> While at it, you could also fix the text, Mediatek -> MediaTek
> 
> >       depends on HAS_IOMEM
> >       select TIMER_OF
> >       select CLKSRC_MMIO
> > diff --git a/drivers/clocksource/timer-mediatek.c
> > b/drivers/clocksource/timer-mediatek.c
> > index 7bcb4a3f26fb..3448848682c0 100644
> > --- a/drivers/clocksource/timer-mediatek.c
> > +++ b/drivers/clocksource/timer-mediatek.c
> > @@ -13,6 +13,9 @@
> >   #include <linux/clocksource.h>
> >   #include <linux/interrupt.h>
> >   #include <linux/irqreturn.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> >   #include <linux/sched_clock.h>
> >   #include <linux/slab.h>
> >   #include "timer-of.h"
> > @@ -337,5 +340,41 @@ static int __init mtk_gpt_init(struct
> > device_node *node)
> > 
> >       return 0;
> >   }
> > +
> > +#ifdef MODULE
> 
> #ifndef MODULE
> ... two lines...
> #else
> ... a bunch of lines ...
> #endif
> 
> looks more readable. I'd go with that.
> 
> > +static int mtk_timer_probe(struct platform_device *pdev)
> > +{
> > +     int (*timer_init)(struct device_node *node);
> > +     struct device_node *np = pdev->dev.of_node;
> > +
> > +     timer_init = of_device_get_match_data(&pdev->dev);
> > +     return timer_init(np);
> > +}
> > +
> > +static const struct of_device_id mtk_timer_match_table[] = {
> > +     {
> > +             .compatible = "mediatek,mt6577-timer",
> > +             .data = mtk_gpt_init,
> 
> Fits in one line!
> 
> > +     },
> > +     {
> > +             .compatible = "mediatek,mt6765-timer",
> > +             .data = mtk_syst_init,
> 
> ditto.
> 
> > +     },
> > +     {}
> 
> Always end with { /* sentinel */ }
> 
> > +};
> > +
> > +static struct platform_driver mtk_timer_driver = {
> > +     .probe = mtk_timer_probe,
> > +     .driver = {
> > +             .name = "mtk-timer",
> 
> "mediatek-timer" looks nicer :-)
> 
> > +             .of_match_table = mtk_timer_match_table,
> > +     },
> > +};
> > +module_platform_driver(mtk_timer_driver);
> > +
> > +MODULE_DESCRIPTION("MediaTek Module Timer driver");
> 
> "MediaTek Timer driver" is enough, "Module" gets misleading if this
> gets compiled
> as built in platform driver (instead of built in timer_of).
> 

Thanks for your review, I will fix these in next patch.

Best regards,
Walter Chang
diff mbox series

Patch

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 526382dc7482..a7413ad7b6ad 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -472,7 +472,7 @@  config SYS_SUPPORTS_SH_CMT
 	bool
 
 config MTK_TIMER
-	bool "Mediatek timer driver" if COMPILE_TEST
+	tristate "Mediatek timer driver"
 	depends on HAS_IOMEM
 	select TIMER_OF
 	select CLKSRC_MMIO
diff --git a/drivers/clocksource/timer-mediatek.c b/drivers/clocksource/timer-mediatek.c
index 7bcb4a3f26fb..3448848682c0 100644
--- a/drivers/clocksource/timer-mediatek.c
+++ b/drivers/clocksource/timer-mediatek.c
@@ -13,6 +13,9 @@ 
 #include <linux/clocksource.h>
 #include <linux/interrupt.h>
 #include <linux/irqreturn.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
 #include <linux/sched_clock.h>
 #include <linux/slab.h>
 #include "timer-of.h"
@@ -337,5 +340,41 @@  static int __init mtk_gpt_init(struct device_node *node)
 
 	return 0;
 }
+
+#ifdef MODULE
+static int mtk_timer_probe(struct platform_device *pdev)
+{
+	int (*timer_init)(struct device_node *node);
+	struct device_node *np = pdev->dev.of_node;
+
+	timer_init = of_device_get_match_data(&pdev->dev);
+	return timer_init(np);
+}
+
+static const struct of_device_id mtk_timer_match_table[] = {
+	{
+		.compatible = "mediatek,mt6577-timer",
+		.data = mtk_gpt_init,
+	},
+	{
+		.compatible = "mediatek,mt6765-timer",
+		.data = mtk_syst_init,
+	},
+	{}
+};
+
+static struct platform_driver mtk_timer_driver = {
+	.probe = mtk_timer_probe,
+	.driver = {
+		.name = "mtk-timer",
+		.of_match_table = mtk_timer_match_table,
+	},
+};
+module_platform_driver(mtk_timer_driver);
+
+MODULE_DESCRIPTION("MediaTek Module Timer driver");
+MODULE_LICENSE("GPL v2");
+#else
 TIMER_OF_DECLARE(mtk_mt6577, "mediatek,mt6577-timer", mtk_gpt_init);
 TIMER_OF_DECLARE(mtk_mt6765, "mediatek,mt6765-timer", mtk_syst_init);
+#endif