From: Varadarajan Narayanan <quic_varada@quicinc.com>
To: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc: <andersson@kernel.org>, <mturquette@baylibre.com>,
<sboyd@kernel.org>, <robh@kernel.org>, <krzk+dt@kernel.org>,
<conor+dt@kernel.org>, <konrad.dybcio@linaro.org>,
<djakov@kernel.org>, <dmitry.baryshkov@linaro.org>,
<quic_anusha@quicinc.com>, <linux-arm-msm@vger.kernel.org>,
<linux-clk@vger.kernel.org>, <devicetree@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <linux-pm@vger.kernel.org>
Subject: Re: [PATCH v10 1/6] interconnect: icc-clk: Allow user to specify master/slave ids
Date: Tue, 30 Apr 2024 12:00:05 +0530 [thread overview]
Message-ID: <ZjCP7VQ97lS6SeHS@hu-varada-blr.qualcomm.com> (raw)
In-Reply-To: <cfd60a71-b0c1-49d5-82ce-0c52764cea14@linaro.org>
On Mon, Apr 29, 2024 at 12:08:40PM +0100, Bryan O'Donoghue wrote:
> On 29/04/2024 10:13, Varadarajan Narayanan wrote:
> > for (i = 0, j = 0; i < num_clocks; i++) {
> > qp->clocks[i].clk = data[i].clk;
> > - node = icc_node_create(first_id + j);
> > + node = icc_node_create(first_id + data[i].master_id);
>
> You have a few conditionals in the way down the end of the existing for()
> loop but then you hit this
>
> onecell->nodes[j++] = node;
> }
>
> which means that this
>
> node = icc_node_create(first_id + data[i].master_id);
>
> is not analogous to this
>
> node = icc_node_create(first_id + j);
>
> So for any loop of this for() where j was incremented previously you would
> not _not_ have the same node ids after your change.
>
> In other words dropping the j index will result in different node numbering.
>
> Is that
>
> a) intended
Yes.
> b) correct
Currently, drivers/clk/qcom/clk-cbf-8996.c is the only user of
icc-clk. And, it had exactly one master and one slave node.
For this the auto generated master (= 1) and slave (= 0) was
enough.
However, when drivers/clk/qcom/gcc-ipq9574.c wanted to make use
of the icc-clk framework, it had more number of master and slave
nodes and the auto generated ids did not suit the usage.
Hence wanted to move away from the auto generated method. And
instead use the ids specified by the caller.
> and slave ids" which it does but it _also_ changes the autogenerated ids.
>
> So could you either a) fix that or b) justify it, in your commit log.
Will change the commit log and post a new version.
> Also I think the 8996 specific change should be in its own patch.
Earlier it was separate. Was squashed into this based on
community feedback. Please refer to https://lore.kernel.org/linux-arm-msm/CAA8EJpqaXU=H6Nhz2_WTYHS1A0bi1QrMdp7Y+s6HUELioCzbeg@mail.gmail.com/
> TBH I'm not sure the autogen change is on-purpose or warranted and for
> certain the commit log is not elucidating on which is the intended case.
>
> I think you should rewrite this patch in two ways
>
> 1. Fix the autogen case or
> 1. Justify the change for the autogen case.
> 2. Separate drivers/clk/qcom/clk-cbf-8996.c into its own patch that
> applies directly after changing the core
>
> Perhaps you've already gone through this debate with other reviewers but
> then you haven't captured that in your cover letter or commit log so at a
> minimum, please do that.
Will update the commit log and post a new version.
Thanks for the inputs.
-Varada
next prev parent reply other threads:[~2024-04-30 6:30 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-29 9:13 [PATCH v10 0/6] Add interconnect driver for IPQ9574 SoC Varadarajan Narayanan
2024-04-29 9:13 ` [PATCH v10 1/6] interconnect: icc-clk: Allow user to specify master/slave ids Varadarajan Narayanan
2024-04-29 11:08 ` Bryan O'Donoghue
2024-04-30 6:30 ` Varadarajan Narayanan [this message]
2024-04-29 11:09 ` Georgi Djakov
2024-04-29 9:13 ` [PATCH v10 2/6] dt-bindings: interconnect: Add Qualcomm IPQ9574 support Varadarajan Narayanan
2024-04-29 11:10 ` Georgi Djakov
2024-04-29 9:13 ` [PATCH v10 3/6] interconnect: icc-clk: Add devm_icc_clk_register Varadarajan Narayanan
2024-04-29 11:10 ` Georgi Djakov
2024-04-29 9:13 ` [PATCH v10 4/6] clk: qcom: common: Add interconnect clocks support Varadarajan Narayanan
2024-04-29 9:13 ` [PATCH v10 5/6] clk: qcom: ipq9574: Use icc-clk for enabling NoC related clocks Varadarajan Narayanan
2024-04-29 9:13 ` [PATCH v10 6/6] arm64: dts: qcom: ipq9574: Add icc provider ability to gcc Varadarajan Narayanan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZjCP7VQ97lS6SeHS@hu-varada-blr.qualcomm.com \
--to=quic_varada@quicinc.com \
--cc=andersson@kernel.org \
--cc=bryan.odonoghue@linaro.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=djakov@kernel.org \
--cc=dmitry.baryshkov@linaro.org \
--cc=konrad.dybcio@linaro.org \
--cc=krzk+dt@kernel.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=mturquette@baylibre.com \
--cc=quic_anusha@quicinc.com \
--cc=robh@kernel.org \
--cc=sboyd@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).