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 447EBC54734 for ; Tue, 27 Aug 2024 10:51:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Subject:CC:To: From:Date:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=2+dHxbL6hWlk4ReGNJMMsetPTpffRqiwhjXgTYYWBJQ=; b=r8nZdFQXp0GM9AMWyR67iKyb/a t2iAkyP23c1H46N+IoRz9Z4PIShvRwLoSUovH2w3Di3pCVy1MqKymKJII33P1u75qqKeVcfTGQkOW y9cdaW+vYtxLaJdKocD5ZlytTW/zCYBPg0FD8xymj11lpHmvPmNRiVROkgygYBO4FY4voBAZLJRef Aimx7tXPvTDL/zbaVW9XWqtwiL5YiyyJvoqqf4IYGjlBSUuUH2s6w2NXT0VoLLZ9JZTxKWTU3tGZL sDUIWMqbsB6h0NbMNUboEcenCUZDOgxn4M5kF7WF3jsSKCcMbNJpwuOaCtxrllbdPPHp8PXzR2vdm SafJ9EzQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sitnX-0000000AsoO-3WOy; Tue, 27 Aug 2024 10:51:40 +0000 Received: from frasgout.his.huawei.com ([185.176.79.56]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sitmi-0000000Asg6-480J for linux-arm-kernel@lists.infradead.org; Tue, 27 Aug 2024 10:50:51 +0000 Received: from mail.maildlp.com (unknown [172.18.186.216]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4WtPPp5lG1z6K98Q; Tue, 27 Aug 2024 18:47:26 +0800 (CST) Received: from lhrpeml500005.china.huawei.com (unknown [7.191.163.240]) by mail.maildlp.com (Postfix) with ESMTPS id 05E8F140447; Tue, 27 Aug 2024 18:50:42 +0800 (CST) Received: from localhost (10.203.177.66) by lhrpeml500005.china.huawei.com (7.191.163.240) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Tue, 27 Aug 2024 11:50:41 +0100 Date: Tue, 27 Aug 2024 11:50:40 +0100 From: Jonathan Cameron To: Yangtao Li CC: , , , , , , , , , , , , , , , , , , , , , , , , , Subject: Re: [net-next v3 4/9] net: mdio: hisi-femac: Convert to devm_clk_get_enabled() Message-ID: <20240827115040.000017b0@Huawei.com> In-Reply-To: <20240827095712.2672820-5-frank.li@vivo.com> References: <20240827095712.2672820-1-frank.li@vivo.com> <20240827095712.2672820-5-frank.li@vivo.com> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.203.177.66] X-ClientProxiedBy: lhrpeml500006.china.huawei.com (7.191.161.198) To lhrpeml500005.china.huawei.com (7.191.163.240) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240827_035049_348974_15A04C18 X-CRM114-Status: GOOD ( 20.65 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, 27 Aug 2024 03:57:07 -0600 Yangtao Li wrote: > Convert devm_clk_get(), clk_prepare_enable() to a single > call to devm_clk_get_enabled(), as this is exactly > what this function does. > > Signed-off-by: Yangtao Li Mixing an matching devm and otherwise can lead to subtle bugs and generally makes the code harder to review I'd also use devm_mdiobus_alloc_size() and devm_mdiobus_register() and drop the remove() callback entirely. I would not take this patch on its own as it causes a reordering of cleanup we probably don't want. As a general rule, single action devm cleanup series (so only using one new type) fall into this trap. Much better to look at all the improvements in each driver that are enabled by new infrastructure rather than doing a single type change series like this one. Thanks, Jonathan > --- > drivers/net/mdio/mdio-hisi-femac.c | 18 +++++------------- > 1 file changed, 5 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/mdio/mdio-hisi-femac.c b/drivers/net/mdio/mdio-hisi-femac.c > index 6703f626ee83..f6fcb9e11624 100644 > --- a/drivers/net/mdio/mdio-hisi-femac.c > +++ b/drivers/net/mdio/mdio-hisi-femac.c > @@ -21,7 +21,6 @@ > #define BIT_WR_DATA_OFFSET 16 > > struct hisi_femac_mdio_data { > - struct clk *clk; > void __iomem *membase; > }; > > @@ -74,6 +73,7 @@ static int hisi_femac_mdio_probe(struct platform_device *pdev) > struct device_node *np = pdev->dev.of_node; > struct mii_bus *bus; > struct hisi_femac_mdio_data *data; > + struct clk *clk; > int ret; > > bus = mdiobus_alloc_size(sizeof(*data)); > @@ -93,26 +93,20 @@ static int hisi_femac_mdio_probe(struct platform_device *pdev) > goto err_out_free_mdiobus; > } > > - data->clk = devm_clk_get(&pdev->dev, NULL); > - if (IS_ERR(data->clk)) { > - ret = PTR_ERR(data->clk); > + clk = devm_clk_get_prepared(&pdev->dev, NULL); > + if (IS_ERR(clk)) { > + ret = PTR_ERR(clk); > goto err_out_free_mdiobus; > } > > - ret = clk_prepare_enable(data->clk); > - if (ret) > - goto err_out_free_mdiobus; > - > ret = of_mdiobus_register(bus, np); > if (ret) > - goto err_out_disable_clk; > + goto err_out_free_mdiobus; > > platform_set_drvdata(pdev, bus); > > return 0; > > -err_out_disable_clk: > - clk_disable_unprepare(data->clk); > err_out_free_mdiobus: > mdiobus_free(bus); > return ret; > @@ -121,10 +115,8 @@ static int hisi_femac_mdio_probe(struct platform_device *pdev) > static void hisi_femac_mdio_remove(struct platform_device *pdev) > { > struct mii_bus *bus = platform_get_drvdata(pdev); > - struct hisi_femac_mdio_data *data = bus->priv; > > mdiobus_unregister(bus); > - clk_disable_unprepare(data->clk); > mdiobus_free(bus); > } >