Re: [PATCH 3/6] clk: mediatek: reset: Return reset data pointer on register

From: Yassine Oudjana <yassine.oudjana@gmail.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Cc: "Michael Turquette" <mturquette@baylibre.com>,
	"Stephen Boyd" <sboyd@kernel.org>,
	"Matthias Brugger" <matthias.bgg@gmail.com>,
	"Philipp Zabel" <p.zabel@pengutronix.de>,
	"Yassine Oudjana" <y.oudjana@protonmail.com>,
	"Chen-Yu Tsai" <wenst@chromium.org>,
	"Miles Chen" <miles.chen@mediatek.com>,
	"Chun-Jie Chen" <chun-jie.chen@mediatek.com>,
	"José Expósito" <jose.exposito89@gmail.com>,
	"Rex-BC Chen" <rex-bc.chen@mediatek.com>,
	linux-mediatek@lists.infradead.org, linux-clk@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	~postmarketos/upstreaming@lists.sr.ht
Subject: Re: [PATCH 3/6] clk: mediatek: reset: Return reset data pointer on register
Date: Fri, 20 May 2022 13:41:33 +0400	[thread overview]
Message-ID: <9LD6CR.FM55IKDYS0IC2@gmail.com> (raw)
In-Reply-To: <5b5f6656-8694-dc78-ef42-7ce301849aa4@collabora.com>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel