From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 010BFC4332F for ; Fri, 30 Dec 2022 05:12:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229799AbiL3FMz (ORCPT ); Fri, 30 Dec 2022 00:12:55 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45736 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229379AbiL3FMs (ORCPT ); Fri, 30 Dec 2022 00:12:48 -0500 Received: from mail-vs1-xe32.google.com (mail-vs1-xe32.google.com [IPv6:2607:f8b0:4864:20::e32]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 89E2915FC9 for ; Thu, 29 Dec 2022 21:12:47 -0800 (PST) Received: by mail-vs1-xe32.google.com with SMTP id d185so20432611vsd.0 for ; Thu, 29 Dec 2022 21:12:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=j4sxQBZTeARJaOtpQ26mu6MKSoj0k8stp/j0GJKpAsI=; b=S2uvp+3QvQHmc53dc1Zb1zRkgnvTrqDFph6z1iQJ1dkE2S6Lmyw3pPi+ReQ+TZaYlj +bKXUOQpdkzuNh3oeBy/xi3M8XP7xyHXoZqCEIFP8mjmmnjMsQAnKKXwimC5lwdvax/e 7Thkl1MUfROcmT+3COFvA0FOA1TwikZ6me+j4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=j4sxQBZTeARJaOtpQ26mu6MKSoj0k8stp/j0GJKpAsI=; b=UlizNCBbJDTXBYBxHI2AJDsXfHTILSYBL1ThbKkoD7jcql60kAY49cShC4l9F9pjg3 c87N0k65kIqU8/6U6DJDMhEBEbFFTq6CkCDENtqyvo7zSvvJ5ACsbboeS359zlrpqj25 Wcp09EvhddR9LvGE4qyW1SwbRpMPGcE8RavprIh31T6/EQFL0xTC5GHy0IHzTyXyJNam AHb+64OPA9qJMDfiivV7NHekb5ExnukccgldOxpxt/FrEPsN6mpJmVi1FNDXktbd6NxZ vgPWuIzWrbDc9bMVNLyR+qqtKQmoLGGQX9nHoBVF/SniKvOOaaiUud8St8UqppSLBpCc MldQ== X-Gm-Message-State: AFqh2kpwRerHqoDoVG62MmxyD2LE2D3Q2K/2x5JGMB7ahtyD2sCOQasg 1rp+WhTJp0f7sYvS5OQd9uP2k4hmzcGC/aIhCgrSWw== X-Google-Smtp-Source: AMrXdXvBIpfvv7eYq0qOFA9b8lukSrK2ZsYP9FnFRdTNpKcBJc7aHUFbMHI+JTfSPUyiqECr7ENUw3wXDtbCuyv/VHU= X-Received: by 2002:a05:6102:3d9f:b0:3c4:4918:80c with SMTP id h31-20020a0561023d9f00b003c44918080cmr2639318vsv.9.1672377166648; Thu, 29 Dec 2022 21:12:46 -0800 (PST) MIME-Version: 1.0 References: <20221223094259.87373-1-angelogioacchino.delregno@collabora.com> <20221223094259.87373-12-angelogioacchino.delregno@collabora.com> In-Reply-To: <20221223094259.87373-12-angelogioacchino.delregno@collabora.com> From: Chen-Yu Tsai Date: Fri, 30 Dec 2022 13:12:35 +0800 Message-ID: Subject: Re: [PATCH v2 11/23] clk: mediatek: Switch to mtk_clk_simple_probe() where possible To: AngeloGioacchino Del Regno Cc: mturquette@baylibre.com, sboyd@kernel.org, matthias.bgg@gmail.com, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, johnson.wang@mediatek.com, miles.chen@mediatek.com, fparent@baylibre.com, chun-jie.chen@mediatek.com, sam.shih@mediatek.com, y.oudjana@protonmail.com, nfraprado@collabora.com, rex-bc.chen@mediatek.com, ryder.lee@kernel.org, daniel@makrotopia.org, jose.exposito89@gmail.com, yangyingliang@huawei.com, pablo.sun@mediatek.com, msp@baylibre.com, weiyi.lu@mediatek.com, ikjn@chromium.org, linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, kernel@collabora.com Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Dec 23, 2022 at 5:43 PM AngeloGioacchino Del Regno wrote: > > mtk_clk_simple_probe() is a function that registers mtk gate clocks > and, if reset data is present, a reset controller and across all of > the MTK clock drivers, such a function is duplicated many times: > switch to the common mtk_clk_simple_probe() function for all of the > clock drivers that are registering as platform drivers. > > Signed-off-by: AngeloGioacchino Del Regno > --- > drivers/clk/mediatek/clk-mt2701-aud.c | 26 +++---- > drivers/clk/mediatek/clk-mt2701-eth.c | 34 +++------ > drivers/clk/mediatek/clk-mt2701-g3d.c | 56 +++----------- > drivers/clk/mediatek/clk-mt2701-hif.c | 36 +++------ > drivers/clk/mediatek/clk-mt2712.c | 83 ++++++++------------- > drivers/clk/mediatek/clk-mt6779.c | 42 ++++++----- > drivers/clk/mediatek/clk-mt7622-aud.c | 49 +++---------- > drivers/clk/mediatek/clk-mt7622-eth.c | 82 ++++----------------- > drivers/clk/mediatek/clk-mt7622-hif.c | 85 ++++----------------- > drivers/clk/mediatek/clk-mt7629-hif.c | 85 ++++----------------- > drivers/clk/mediatek/clk-mt8183-audio.c | 19 +++-- > drivers/clk/mediatek/clk-mt8183.c | 75 ++++++++----------- > drivers/clk/mediatek/clk-mt8192-aud.c | 25 +++---- > drivers/clk/mediatek/clk-mt8192.c | 98 ++++++++----------------- > 14 files changed, 236 insertions(+), 559 deletions(-) This looks mostly good, however ... > diff --git a/drivers/clk/mediatek/clk-mt2701-aud.c b/drivers/clk/mediatek/clk-mt2701-aud.c > index ab13ab618fb5..1fd6d96b34dc 100644 > --- a/drivers/clk/mediatek/clk-mt2701-aud.c > +++ b/drivers/clk/mediatek/clk-mt2701-aud.c > @@ -76,6 +76,7 @@ static const struct mtk_gate_regs audio3_cg_regs = { > }; > > static const struct mtk_gate audio_clks[] = { > + GATE_DUMMY(CLK_DUMMY, "aud_dummy"), > /* AUDIO0 */ > GATE_AUDIO0(CLK_AUD_AFE, "audio_afe", "aud_intbus_sel", 2), > GATE_AUDIO0(CLK_AUD_HDMI, "audio_hdmi", "audpll_sel", 20), > @@ -138,29 +139,26 @@ static const struct mtk_gate audio_clks[] = { > GATE_AUDIO3(CLK_AUD_MEM_ASRC5, "audio_mem_asrc5", "asm_h_sel", 14), > }; > > +static const struct mtk_clk_desc audio_desc = { > + .clks = audio_clks, > + .num_clks = ARRAY_SIZE(audio_clks), > +}; > + > static const struct of_device_id of_match_clk_mt2701_aud[] = { > - { .compatible = "mediatek,mt2701-audsys", }, > - {} > + { .compatible = "mediatek,mt2701-audsys", .data = &audio_desc }, > + { /* sentinel */ } > }; > > static int clk_mt2701_aud_probe(struct platform_device *pdev) > { > - struct clk_hw_onecell_data *clk_data; > - struct device_node *node = pdev->dev.of_node; > int r; > > - clk_data = mtk_alloc_clk_data(CLK_AUD_NR); > - > - mtk_clk_register_gates(node, audio_clks, ARRAY_SIZE(audio_clks), > - clk_data, &pdev->dev); > - > - r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data); > + r = mtk_clk_simple_probe(pdev); > if (r) { > dev_err(&pdev->dev, > "could not register clock provider: %s: %d\n", > pdev->name, r); > - > - goto err_clk_provider; > + return r; > } > > r = devm_of_platform_populate(&pdev->dev); > @@ -170,13 +168,13 @@ static int clk_mt2701_aud_probe(struct platform_device *pdev) > return 0; > > err_plat_populate: > - of_clk_del_provider(node); > -err_clk_provider: > + mtk_clk_simple_remove(pdev); > return r; > } > > static struct platform_driver clk_mt2701_aud_drv = { > .probe = clk_mt2701_aud_probe, > + .remove = mtk_clk_simple_remove, I'm not a big fan of mixing devres and non-devres teardown code. Automatic devres teardown happens after the remove callback returns, so in this case you could have child devices being unregistered that touch clocks or resets that have already been unregistered and freed in the remove callback. > .driver = { > .name = "clk-mt2701-aud", > .of_match_table = of_match_clk_mt2701_aud, [...] > --- a/drivers/clk/mediatek/clk-mt2712.c > +++ b/drivers/clk/mediatek/clk-mt2712.c [...] > @@ -1482,7 +1459,11 @@ static struct platform_driver clk_mt2712_drv = { > > static int __init clk_mt2712_init(void) > { > - return platform_driver_register(&clk_mt2712_drv); > + int ret = platform_driver_register(&clk_mt2712_drv); > + > + if (ret) > + return ret; > + return platform_driver_register(&clk_mt2712_simple_drv); > } > > arch_initcall(clk_mt2712_init); Would this get cleaned up even more? I.e. have just one driver left and we could have the nice *_platform_driver() macros. Thanks ChenYu From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 3A382C4332F for ; Fri, 30 Dec 2022 05:14:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=wE2/oP2+2CVWZu9bGk6vgj+K1+eDuoQ6RbKh3pobFrk=; b=mqFIYyBufvqqpg kxA0z1szzWWWfBy1dSJ8Uqbn6I4g6MmUdZw/m1kR/OZlegRQk3FFUDAgwR/yxkmzNx7jSzwQjPJhH kBkPgpfGAkdIvMPf7RuGZY2v2VRjsaOimssEA3dmuaroXwfkyMTRp5GEisiwMvwAFcJGmJmignb8s 4u38HoMLLzCztDbj2QSGydBhLn4KCv1EsER05o4ddIKR+NfHLhxMz96CGreDV3X0pR7f3UsISyuXy huknJx7NF51cd9IwvVHe1UC+GIJ8nYEgISWrd5B1+kAbyiKkdzODoiuwDlV3NFGP2fFp+Wyo61hp2 bo+5np0DtTWSKnkFSjPQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pB7hO-005qCo-9j; Fri, 30 Dec 2022 05:12:54 +0000 Received: from mail-vs1-xe2b.google.com ([2607:f8b0:4864:20::e2b]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1pB7hI-005q8M-9v for linux-arm-kernel@lists.infradead.org; Fri, 30 Dec 2022 05:12:50 +0000 Received: by mail-vs1-xe2b.google.com with SMTP id a64so16871955vsc.2 for ; Thu, 29 Dec 2022 21:12:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=j4sxQBZTeARJaOtpQ26mu6MKSoj0k8stp/j0GJKpAsI=; b=S2uvp+3QvQHmc53dc1Zb1zRkgnvTrqDFph6z1iQJ1dkE2S6Lmyw3pPi+ReQ+TZaYlj +bKXUOQpdkzuNh3oeBy/xi3M8XP7xyHXoZqCEIFP8mjmmnjMsQAnKKXwimC5lwdvax/e 7Thkl1MUfROcmT+3COFvA0FOA1TwikZ6me+j4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=j4sxQBZTeARJaOtpQ26mu6MKSoj0k8stp/j0GJKpAsI=; b=ezLXwBL7A39NThpaT/tuqREolGRlVA/6vY0pMw351Lkdzwuy3/TjY0lgqm+y8eNPia N1xJKL3lndBFdmg477rlNhdS/M0J8mgKEtN9mTRT5eAwNu+Ik/zj4nTFzK2M91AgHcjj X/560IKUGWlm7DYxjsWmX5LWmdeh1wPZkIwGzNItGUhCIKXdC/0qoHQuYQ+ZnfSA2eir wTUgqhF6WMbjJr5uLguyo+nfX1YvELIR7KOct3FBHOxiUL/J6TNnxkaLe5S0FhqPR1qF F2qC3Z9R8DXsrR5skoSOvMlsi4uO7jzoIjvHU7CKrDObYTG0GfGQ0TYaYVxJUvD0GLAb WCLA== X-Gm-Message-State: AFqh2krN5tLEvPu6tGunJUrrYzGNC9VG4zrgACxzwudK+kxJwMUHD8wZ AwVtX8Vxk7owHMb3t+KBnTZZPWWoDL2eYUqR1d8N5w== X-Google-Smtp-Source: AMrXdXvBIpfvv7eYq0qOFA9b8lukSrK2ZsYP9FnFRdTNpKcBJc7aHUFbMHI+JTfSPUyiqECr7ENUw3wXDtbCuyv/VHU= X-Received: by 2002:a05:6102:3d9f:b0:3c4:4918:80c with SMTP id h31-20020a0561023d9f00b003c44918080cmr2639318vsv.9.1672377166648; Thu, 29 Dec 2022 21:12:46 -0800 (PST) MIME-Version: 1.0 References: <20221223094259.87373-1-angelogioacchino.delregno@collabora.com> <20221223094259.87373-12-angelogioacchino.delregno@collabora.com> In-Reply-To: <20221223094259.87373-12-angelogioacchino.delregno@collabora.com> From: Chen-Yu Tsai Date: Fri, 30 Dec 2022 13:12:35 +0800 Message-ID: Subject: Re: [PATCH v2 11/23] clk: mediatek: Switch to mtk_clk_simple_probe() where possible To: AngeloGioacchino Del Regno Cc: mturquette@baylibre.com, sboyd@kernel.org, matthias.bgg@gmail.com, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, johnson.wang@mediatek.com, miles.chen@mediatek.com, fparent@baylibre.com, chun-jie.chen@mediatek.com, sam.shih@mediatek.com, y.oudjana@protonmail.com, nfraprado@collabora.com, rex-bc.chen@mediatek.com, ryder.lee@kernel.org, daniel@makrotopia.org, jose.exposito89@gmail.com, yangyingliang@huawei.com, pablo.sun@mediatek.com, msp@baylibre.com, weiyi.lu@mediatek.com, ikjn@chromium.org, linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, kernel@collabora.com X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221229_211248_370737_15D7A420 X-CRM114-Status: GOOD ( 28.35 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, Dec 23, 2022 at 5:43 PM AngeloGioacchino Del Regno wrote: > > mtk_clk_simple_probe() is a function that registers mtk gate clocks > and, if reset data is present, a reset controller and across all of > the MTK clock drivers, such a function is duplicated many times: > switch to the common mtk_clk_simple_probe() function for all of the > clock drivers that are registering as platform drivers. > > Signed-off-by: AngeloGioacchino Del Regno > --- > drivers/clk/mediatek/clk-mt2701-aud.c | 26 +++---- > drivers/clk/mediatek/clk-mt2701-eth.c | 34 +++------ > drivers/clk/mediatek/clk-mt2701-g3d.c | 56 +++----------- > drivers/clk/mediatek/clk-mt2701-hif.c | 36 +++------ > drivers/clk/mediatek/clk-mt2712.c | 83 ++++++++------------- > drivers/clk/mediatek/clk-mt6779.c | 42 ++++++----- > drivers/clk/mediatek/clk-mt7622-aud.c | 49 +++---------- > drivers/clk/mediatek/clk-mt7622-eth.c | 82 ++++----------------- > drivers/clk/mediatek/clk-mt7622-hif.c | 85 ++++----------------- > drivers/clk/mediatek/clk-mt7629-hif.c | 85 ++++----------------- > drivers/clk/mediatek/clk-mt8183-audio.c | 19 +++-- > drivers/clk/mediatek/clk-mt8183.c | 75 ++++++++----------- > drivers/clk/mediatek/clk-mt8192-aud.c | 25 +++---- > drivers/clk/mediatek/clk-mt8192.c | 98 ++++++++----------------- > 14 files changed, 236 insertions(+), 559 deletions(-) This looks mostly good, however ... > diff --git a/drivers/clk/mediatek/clk-mt2701-aud.c b/drivers/clk/mediatek/clk-mt2701-aud.c > index ab13ab618fb5..1fd6d96b34dc 100644 > --- a/drivers/clk/mediatek/clk-mt2701-aud.c > +++ b/drivers/clk/mediatek/clk-mt2701-aud.c > @@ -76,6 +76,7 @@ static const struct mtk_gate_regs audio3_cg_regs = { > }; > > static const struct mtk_gate audio_clks[] = { > + GATE_DUMMY(CLK_DUMMY, "aud_dummy"), > /* AUDIO0 */ > GATE_AUDIO0(CLK_AUD_AFE, "audio_afe", "aud_intbus_sel", 2), > GATE_AUDIO0(CLK_AUD_HDMI, "audio_hdmi", "audpll_sel", 20), > @@ -138,29 +139,26 @@ static const struct mtk_gate audio_clks[] = { > GATE_AUDIO3(CLK_AUD_MEM_ASRC5, "audio_mem_asrc5", "asm_h_sel", 14), > }; > > +static const struct mtk_clk_desc audio_desc = { > + .clks = audio_clks, > + .num_clks = ARRAY_SIZE(audio_clks), > +}; > + > static const struct of_device_id of_match_clk_mt2701_aud[] = { > - { .compatible = "mediatek,mt2701-audsys", }, > - {} > + { .compatible = "mediatek,mt2701-audsys", .data = &audio_desc }, > + { /* sentinel */ } > }; > > static int clk_mt2701_aud_probe(struct platform_device *pdev) > { > - struct clk_hw_onecell_data *clk_data; > - struct device_node *node = pdev->dev.of_node; > int r; > > - clk_data = mtk_alloc_clk_data(CLK_AUD_NR); > - > - mtk_clk_register_gates(node, audio_clks, ARRAY_SIZE(audio_clks), > - clk_data, &pdev->dev); > - > - r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data); > + r = mtk_clk_simple_probe(pdev); > if (r) { > dev_err(&pdev->dev, > "could not register clock provider: %s: %d\n", > pdev->name, r); > - > - goto err_clk_provider; > + return r; > } > > r = devm_of_platform_populate(&pdev->dev); > @@ -170,13 +168,13 @@ static int clk_mt2701_aud_probe(struct platform_device *pdev) > return 0; > > err_plat_populate: > - of_clk_del_provider(node); > -err_clk_provider: > + mtk_clk_simple_remove(pdev); > return r; > } > > static struct platform_driver clk_mt2701_aud_drv = { > .probe = clk_mt2701_aud_probe, > + .remove = mtk_clk_simple_remove, I'm not a big fan of mixing devres and non-devres teardown code. Automatic devres teardown happens after the remove callback returns, so in this case you could have child devices being unregistered that touch clocks or resets that have already been unregistered and freed in the remove callback. > .driver = { > .name = "clk-mt2701-aud", > .of_match_table = of_match_clk_mt2701_aud, [...] > --- a/drivers/clk/mediatek/clk-mt2712.c > +++ b/drivers/clk/mediatek/clk-mt2712.c [...] > @@ -1482,7 +1459,11 @@ static struct platform_driver clk_mt2712_drv = { > > static int __init clk_mt2712_init(void) > { > - return platform_driver_register(&clk_mt2712_drv); > + int ret = platform_driver_register(&clk_mt2712_drv); > + > + if (ret) > + return ret; > + return platform_driver_register(&clk_mt2712_simple_drv); > } > > arch_initcall(clk_mt2712_init); Would this get cleaned up even more? I.e. have just one driver left and we could have the nice *_platform_driver() macros. Thanks ChenYu _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel