Messages in this thread | | | Date | Fri, 20 May 2022 13:41:33 +0400 | From | Yassine Oudjana <> | Subject | Re: [PATCH 3/6] clk: mediatek: reset: Return reset data pointer on register |
| |
On Fri, May 20 2022 at 10:42:40 +0200, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > Il 19/05/22 15:47, Yassine Oudjana ha scritto: >> From: Yassine Oudjana <y.oudjana@protonmail.com> >> >> Return a struct mtk_clk_rst_data * when registering a reset >> controller in preparation for adding an unregister helper >> that will take it as an argument. Make the necessary changes >> in drivers that do not currently discard the return value >> of register functions. >> >> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com> > > Hello Yassine, > > Thanks for your efforts on helping to make the MediaTek clocks better > - I agree > (and I'm not the only one..) that there's a lot of work to do on this > side. > > Though... I don't think that this is the right direction: you're > right about > properly unregistering (in patch 4/6) the reset controllers on > rmmod/failure > but I'm not sure that this kind of noise brings any benefit. > > Explaining: > You definitely saw that there's a new register _with_dev, which uses > devm ops > and that's going to automatically cleanup in case of removal/failure. > This is what we should do. > > Hence, my proposal is to drop patch 3/6, 4/6, 5/6 and (slowly, > steadily) migrate > all of the MediaTek clocks from CLK_OF_DECLARE() to platform drivers > (which also > means that we can eventually change them to tristate!), so that we > slowly remove > all users of all functions that are not "_with_dev", and that we > finally remove > all of these then-unused functions as well.
I've tried to make small (but hopefully not too small) steps with little improvements. Originally in MT6735 clock drivers v1, I only added reset controller unregister, and while rebasing on Rex-BC's reset cleanup series I found mtk_clk_simple_probe/remove while looking for references to mtk_register_reset_controller, so I thought of using it for my drivers resulting in this series adding support for the extra 4 clock types. I started finding other things that could be improved such as the other clock types not having register_*_with_dev(), but I had to avoid adding anything else since that would only make me find more things to improve and this series would've never been finished and sent.
With that said, if these patches could become an obstacle for later more complete reworks, then by all means drop them.
> > Making sure that I don't get misunderstood: > I'm not implying that this huge migration work is on your > shoulders! >
Of course. I would never be able to handle such a large task. Everyone currently helping with modernizing this common clock framework has my full respect. You are doing amazing work.
Thanks, Yassine
| |