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 A8D34E75426 for ; Tue, 3 Oct 2023 12:14:20 +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:In-Reply-To:MIME-Version:References: 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=HhkcxMVue8rE/MldwDrPlprdAmV9rKPli05uzKDsFLo=; b=rL80T4IdZxkxSC LTeKhShiVSPTMA2bqPD/xdlT4HKDI7FMgRg8ZMtII1CPbBN6e3UQ3g/HxWlFSNUAS+v7Ast99Vp9q w6r1ZGxYpf4gmhM+kcXWI28WhA6I+H6kOEbMyJzPlSoS2lo9SAmKMra3RjX5U+vJlDJlAeRSBOeO8 jtzT7bHMYMz/8Cj03jCuKsFmYDyfJ++zG6HcT4k93Mi8mXpjRwhhK33GcTAQbxMmjGtUrIWGU9htS y9f4KOVmW/OaFBgXGPuSJ3ux1VcctJ4ZXcfKDfpsc93TBoq+IT9RybfnMA3IBmfMad9iVRLblT/vc LcJnuzDNb67HwkCyxfxA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qneHa-00Eb49-1S; Tue, 03 Oct 2023 12:13:46 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qneHW-00Eb34-26 for linux-arm-kernel@lists.infradead.org; Tue, 03 Oct 2023 12:13:44 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 553C11FB; Tue, 3 Oct 2023 05:14:15 -0700 (PDT) Received: from pluto (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id BCDEA3F59C; Tue, 3 Oct 2023 05:13:35 -0700 (PDT) Date: Tue, 3 Oct 2023 13:13:33 +0100 From: Cristian Marussi To: "Peng Fan (OSS)" Cc: Sudeep Holla , Michael Turquette , Stephen Boyd , Peng Fan , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org Subject: Re: [PATCH v4 2/2] clk: scmi: add set/get_parent support Message-ID: References: <20231003-scmi-clock-v3-v4-0-358d7f916a05@nxp.com> <20231003-scmi-clock-v3-v4-2-358d7f916a05@nxp.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231003-scmi-clock-v3-v4-2-358d7f916a05@nxp.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231003_051342_785476_C2EA12D2 X-CRM114-Status: GOOD ( 26.74 ) 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 Tue, Oct 03, 2023 at 07:48:49PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan > > SCMI v3.2 adds set/get parent clock commands, so update the clk driver > to support them. > Hi, in general LGTM, BUT I have just spotted one more *bad* thing that have to be fixe down below which I have missed previously, my bad. > Signed-off-by: Peng Fan > --- > drivers/clk/clk-scmi.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 57 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c > index 2e1337b511eb..e7a27fda561b 100644 > --- a/drivers/clk/clk-scmi.c > +++ b/drivers/clk/clk-scmi.c > @@ -24,6 +24,7 @@ struct scmi_clk { > struct clk_hw hw; > const struct scmi_clock_info *info; > const struct scmi_protocol_handle *ph; > + struct clk_parent_data *parent_data; > }; > > #define to_scmi_clk(clk) container_of(clk, struct scmi_clk, hw) > @@ -78,6 +79,43 @@ static int scmi_clk_set_rate(struct clk_hw *hw, unsigned long rate, > return scmi_proto_clk_ops->rate_set(clk->ph, clk->id, rate); > } > > +static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index) > +{ > + struct scmi_clk *clk = to_scmi_clk(hw); > + > + return scmi_proto_clk_ops->parent_set(clk->ph, clk->id, parent_index); > +} > + > +static u8 scmi_clk_get_parent(struct clk_hw *hw) > +{ > + struct scmi_clk *clk = to_scmi_clk(hw); > + u32 parent_id, p_idx; > + int ret; > + > + ret = scmi_proto_clk_ops->parent_get(clk->ph, clk->id, &parent_id); > + if (ret) > + return 0; > + > + for (p_idx = 0; p_idx < clk->info->num_parents; p_idx++) { > + if (clk->parent_data[p_idx].index == parent_id) > + break; > + } > + > + if (p_idx == clk->info->num_parents) > + return 0; > + > + return p_idx; > +} > + > +static int scmi_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req) > +{ > + /* > + * Suppose all the requested rates are supported, and let firmware > + * to handle the left work. > + */ > + return 0; > +} > + > static int scmi_clk_enable(struct clk_hw *hw) > { > struct scmi_clk *clk = to_scmi_clk(hw); > @@ -139,6 +177,9 @@ static const struct clk_ops scmi_clk_ops = { > .set_rate = scmi_clk_set_rate, > .prepare = scmi_clk_enable, > .unprepare = scmi_clk_disable, > + .set_parent = scmi_clk_set_parent, > + .get_parent = scmi_clk_get_parent, > + .determine_rate = scmi_clk_determine_rate, > }; > > static const struct clk_ops scmi_atomic_clk_ops = { > @@ -148,6 +189,9 @@ static const struct clk_ops scmi_atomic_clk_ops = { > .enable = scmi_clk_atomic_enable, > .disable = scmi_clk_atomic_disable, > .is_enabled = scmi_clk_atomic_is_enabled, > + .set_parent = scmi_clk_set_parent, > + .get_parent = scmi_clk_get_parent, > + .determine_rate = scmi_clk_determine_rate, > }; > > static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk, > @@ -158,9 +202,10 @@ static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk, > > struct clk_init_data init = { > .flags = CLK_GET_RATE_NOCACHE, > - .num_parents = 0, > + .num_parents = sclk->info->num_parents, > .ops = scmi_ops, > .name = sclk->info->name, > + .parent_data = sclk->parent_data, > }; > > sclk->hw.init = &init; > @@ -250,6 +295,17 @@ static int scmi_clocks_probe(struct scmi_device *sdev) > else > scmi_ops = &scmi_clk_ops; > > + /* Initialize clock parent data. */ > + if (sclk->info->num_parents > 0) { > + sclk->parent_data = devm_kcalloc(dev, sclk->info->num_parents, > + sizeof(*sclk->parent_data), GFP_KERNEL); > + Here, you missed to check the return value of devm_kcalloc() before carrying on. > + for (int i = 0; i < sclk->info->num_parents; i++) { > + sclk->parent_data[i].index = sclk->info->parents[i]; > + sclk->parent_data[i].hw = hws[sclk->info->parents[i]]; > + } > + } > + Other than this, FWIW: Reviewed-by: Cristian Marussi Thanks, Cristian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel