Messages in this thread | | | From | Chen-Yu Tsai <> | Date | Wed, 8 Feb 2023 16:24:01 +0800 | Subject | Re: [PATCH v1 09/45] clk: mediatek: mt2712: Change to use module_platform_driver macro |
| |
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 > > >
| |