Messages in this thread | | | From | Walter Chang (張維哲) <> | Subject | Re: [PATCH linux-next v3 4/4] clocksource/drivers/timer-mediatek: Make timer-mediatek become loadable module | Date | Thu, 20 Apr 2023 01:46:39 +0000 |
| |
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
| |