Re: [PATCH v1 09/45] clk: mediatek: mt2712: Change to use module_platform_driver macro

[Chen-Yu Tsai]

On Tue, Feb 7, 2023 at 6:50 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
>
> Il 07/02/23 10:30, Chen-Yu Tsai ha scritto:
> > On Tue, Feb 7, 2023 at 5:00 PM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
> >>
> >> Il 07/02/23 07:33, Chen-Yu Tsai ha scritto:
> >>> On Mon, Feb 6, 2023 at 11:29 PM AngeloGioacchino Del Regno
> >>> <angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
> >>>>
> >>>> Now that all of the clocks in clk-mt2712.c are using the common
> >>>> mtk_clk_simple_{probe,remove}() callbacks we can safely migrate
> >>>> to module_platform_driver.
> >>>
> >>> Instead of splitting the conversion into a module among many patches,
> >>> I'd do it in one go. With one patch we get a working module instead
> >>> of a half-baked one half way through the series.
> >>>
> >>
> >> If you really want I can eventually do that in one go - in any case, the
> >> sense of having this split in multiple commits is:
> >>    - Bisectability: topckgen/mcucfg migration being faulty would point at
> >>                     one commit doing just that, making it easier for whoever
> >>                     is trying to debug that to find what could've gone wrong;
> >
> > This part I agree with.
> >
> >>    - Slow changes:  A driver being a platform_driver doesn't mean that it *has*
> >>                     to be compiled as a module: infact, we can use the .remove()
> >>                     callback even with built-in drivers (as you can remove one
> >>                     and re-add it during runtime from sysfs)
> >
> > I think the part that tripped me up was that in this patch's case it
> > was already a platform driver, just a builtin one (without the
> > builtin_platform_driver sugar).
> >
> >>    - Signaling completion:
> >>                     Saying "this is complete" in this case is performed in the
> >>                     last patches of the series, where only the Kconfig is being
> >>                     changed to allow the module build for (most)all.
> >
> > I'm concerned about people randomly cherry-picking patches. Unfortunately
> > not everyone lives on mainline, us included. (I'm sure Android has it
> > worse.) Many won't see the complete patch series, doubly so if we merge
> > it in stages. Better we give one complete patch that converts the
> > boilerplate code from "can't work as module" to "can work as module".
> > I do agree we should keep all the other cleanups and migration to
> > simple/pdev_probe separate for bisectability.
> >
>
> One complete patch meaning that migrating to mtk_clk_simple_probe() should be
> squashed with moving apmixedsys away?
>
> So one patch doing the *big* change, and then one changing the driver to use
> the module_platform_driver() macro and tristate in Kconfig?

I'd also add MOD_DEVICE_TABLE. Module autoloading doesn't work otherwise.

The rest of the MODULE_INFO stuff I don't really have a preference on,
but I don't know if there would be any issues with loading a module
that doesn't have MODULE_LICENSE. Maybe the default is "GPL"?

> I would be more comfortable changing the order of commits at this point,
> apmixedsys error handling Fixes -> apmixedsys moved in its own file ->
> migrate others to mtk_clk_simple_probe() *and* Kconfig changes
>
> What do you think?

Sounds good. That way a) apmixed sys error handling could be cleanly
backported if anyone cares, and b) code movement is contained in one patch.

> Thing is, apmixedsys is not a simple_probe driver and will never be, so
> it feels wrong to move that inside of a commit that converts to simple_probe()...

Agreed.

Thanks
ChenYu

> >>> The subject could say "Convert X driver from builtin to module". And
> >>> instead of "migrate to module_platform_driver", the body could say
> >>> "convert to module by switching to module_platform_driver, and adding
> >>> missing MODULE_* statements". I believe this constitutes one logical
> >>> change. Maybe the accompanying Kconfig change should be included as
> >>> well?
> >>>
> >>
> >> But again, I don't have *really strong* opinions on this, if not preferences
> >> for how I'd like to see the changes getting in: this series brings big changes
> >> that would be done in many more commits if they were scattered in more series.
> >> Another point about having this conversion performed in multiple commits is
> >> showing how it was done and how to replicate it for a different driver...
> >
> > In the past I've seen some comments from other maintainers about keeping
> > (module|builtin)_X_driver consistent with its Kconfig entry. That sort of
> > plays into my argument that this bit should be kept atomic.
> >
> > There are a couple patches where you convert directly from CLK_OF_DECLARE
> > to module_platform_driver. We could work those out case by case?
> >
> >>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> >>>> ---
> >>>>    drivers/clk/mediatek/clk-mt2712.c | 10 ++--------
> >>>>    1 file changed, 2 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/drivers/clk/mediatek/clk-mt2712.c b/drivers/clk/mediatek/clk-mt2712.c
> >>>> index c5fd76d1b9df..65c1cbcbd54e 100644
> >>>> --- a/drivers/clk/mediatek/clk-mt2712.c
> >>>> +++ b/drivers/clk/mediatek/clk-mt2712.c
> >>>> @@ -1028,7 +1028,7 @@ static const struct of_device_id of_match_clk_mt2712_simple[] = {
> >>>>           { /* sentinel */ }
> >>>>    };
> >>>>
> >>>> -static struct platform_driver clk_mt2712_simple_drv = {
> >>>> +static struct platform_driver clk_mt2712_drv = {
> >>>
> >>> Why the name change? If you do change the name, could you also change
> >>> the of match table's name as well to be consistent, and also mention
> >>> the change in the commit log?
> >>
> >> It simply looked like being a good idea, as "simple" made sense when we had two
> >> platform_driver in one file, one using simple_probe, one using a custom probe
> >> function.
> >> The latter going away forever means that there's no more distinction to do
> >> between the two, hence my rename here...
> >>
> >> Regarding the of_match_table name change... I'm sorry, I genuinely forgot to
> >> change it, my intention was infact to actually be consistent... :-)
> >>
> >>>
> >>> I'd just leave it alone though.
> >>
> >> I had to explain my reasoning about all of the above, so I'll just wait for
> >> your opinion again before going for a v2! :-)
> >
> > Thanks again for working on this.
> >
> > ChenYu
>
>
>