| Message ID | 20230206152928.918562-10-angelogioacchino.delregno@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MediaTek clocks: full module build and cleanups | expand |
On Mon, Feb 6, 2023 at 11:29 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> 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. 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? > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > --- > 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? I'd just leave it alone though. ChenYu > .probe = mtk_clk_simple_probe, > .remove = mtk_clk_simple_remove, > .driver = { > @@ -1036,10 +1036,4 @@ static struct platform_driver clk_mt2712_simple_drv = { > .of_match_table = of_match_clk_mt2712_simple, > }, > }; > - > -static int __init clk_mt2712_init(void) > -{ > - return platform_driver_register(&clk_mt2712_simple_drv); > -} > - > -arch_initcall(clk_mt2712_init); > +module_platform_driver(clk_mt2712_drv); > -- > 2.39.1 >
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@collabora.com> 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; - 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) - 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. > 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... >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >> --- >> 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! :-) Cheers, Angelo
On Tue, Feb 7, 2023 at 5:00 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> 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@collabora.com> 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. > > 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@collabora.com> > >> --- > >> 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
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@collabora.com> 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@collabora.com> 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 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? 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()... >>> 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@collabora.com> >>>> --- >>>> 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
On Tue, Feb 7, 2023 at 6:50 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> 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@collabora.com> 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@collabora.com> 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@collabora.com> > >>>> --- > >>>> 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 > > >
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 = { .probe = mtk_clk_simple_probe, .remove = mtk_clk_simple_remove, .driver = { @@ -1036,10 +1036,4 @@ static struct platform_driver clk_mt2712_simple_drv = { .of_match_table = of_match_clk_mt2712_simple, }, }; - -static int __init clk_mt2712_init(void) -{ - return platform_driver_register(&clk_mt2712_simple_drv); -} - -arch_initcall(clk_mt2712_init); +module_platform_driver(clk_mt2712_drv);
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. Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> --- drivers/clk/mediatek/clk-mt2712.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)