Re: [PATCH v1 2/2] timer: mt6873: porting Mediatek timer driver to loadable module

[Saravana Kannan <saravanak@google.com>]

On Wed, Jul 29, 2020 at 6:02 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Freddy,
>
> Freddy Hsin <freddy.hsin@mediatek.com> writes:
>
> again, please be more careful with subject lines. git log $FILE will
> give you a hint.
>
> > porting Mediatek timer driver to loadable module
>
> Repeating the sentence in the subject line is not giving any
> information. Also changelogs want to tell the WHY and not the WHAT. This
> also lacks any information why this is actually safe when booting such a
> system w/o this particular driver built in. What is early boot - up to
> module load - using as clocksource and timer?
>
> > diff --git a/drivers/clocksource/mmio.c b/drivers/clocksource/mmio.c
> > index 9de7515..5504569 100644
> > --- a/drivers/clocksource/mmio.c
> > +++ b/drivers/clocksource/mmio.c
> > @@ -21,6 +21,7 @@ u64 clocksource_mmio_readl_up(struct clocksource *c)
> >  {
> >       return (u64)readl_relaxed(to_mmio_clksrc(c)->reg);
> >  }
> > +EXPORT_SYMBOL(clocksource_mmio_readl_up);
>
> Again EXPORT_SYMBOL_GPL() and this wants to be a seperate patch. It has
> absolutely no business with the mediatek timer changes.
>
> >  u64 clocksource_mmio_readl_down(struct clocksource *c)
> >  {
> > @@ -46,7 +47,7 @@ u64 clocksource_mmio_readw_down(struct clocksource *c)
> >   * @bits:    Number of valid bits
> >   * @read:    One of clocksource_mmio_read*() above
> >   */
> > -int __init clocksource_mmio_init(void __iomem *base, const char *name,
> > +int clocksource_mmio_init(void __iomem *base, const char *name,
> >
> >       unsigned long hz, int rating, unsigned bits,
> >       u64 (*read)(struct clocksource *))
> >  {
> > @@ -68,3 +69,4 @@ int __init clocksource_mmio_init(void __iomem *base, const char *name,
> >
> >       return clocksource_register_hz(&cs->clksrc, hz);
> >  }
> > +EXPORT_SYMBOL(clocksource_mmio_init);
>
> See above.
>
> > diff --git a/drivers/clocksource/timer-mediatek.c b/drivers/clocksource/timer-mediatek.c
> > index 9318edc..5c89b6b 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"
> > @@ -309,5 +312,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_DESCRIPTION("MEDIATEK Module timer driver");
> > +MODULE_LICENSE("GPL v2");
> > +
> > +module_platform_driver(mtk_timer_driver);
> > +#else
> >  TIMER_OF_DECLARE(mtk_mt6577, "mediatek,mt6577-timer", mtk_gpt_init);
> >  TIMER_OF_DECLARE(mtk_mt6765, "mediatek,mt6765-timer", mtk_syst_init);
> > +#endif
>
> Sorry no. This is not going to happen.
>
> The above probe, match table and platform driver structs plus the module*
> thingies are going to be repeated in every single driver which is going
> to support module build. Tons of boilerplate copied over and over
> again.
>
> We had exactly the same before TIMER_OF_DECLARE() came around, so pretty
> please this want's to be some smart macro which handles all of this
> automatically.

Probably something like what I did for IRQCHIP?
https://lore.kernel.org/lkml/20200718000637.3632841-1-saravanak@google.com/

Also, one point that came up with IRQCHIP is that if these drivers can
be platform drivers, then they should stay that way even when it's
built in. I'm not sure if that has any other special implications for
timer code, but raising it in case you'd prefer that here too.


-Saravana