Re: [PATCH v4 2/2] clk: qcom: gcc-msm8939: Add MSM8939 Generic Clock Controller

On 14/05/2020 22:31, Stephen Boyd wrote:

Quoting Bryan O'Donoghue (2020-05-12 04:50:23)

quoted

This patch adds support for the MSM8939 GCC. The MSM8939 is based on the
MSM8916. MSM8939 is compatible in several ways with MSM8916 but, has
additional functional blocks added which require additional PLL sources. In
some cases functional blocks from the MSM8916 have different clock sources
or different supported frequencies.

Cc: Andy Gross <agross@kernel.org>
Cc: Bjorn Andersson <redacted>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: linux-arm-msm@vger.kernel.org
Cc: linux-clk@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Shawn Guo <redacted>

Is this a co-developed-by tag?

Yep. I'm squashing down about 30-some internal patches to this one patch 
here including one or two from Shawn in this set.

I wasn't quite sure what the etiquette on Co-developed was i.e. it 
wasn't something git allowed me to specify with a "git commit -s 
--co-developed="xyz"" so I just retained the SOB.

Looking through git logs I see an example

I'll apply a
Co-developed-by: Shawn Guo <redacted>

for v5.

quoted

+static int gcc_msm8939_probe(struct platform_device *pdev)
+{
+       int ret;
+       struct regmap *regmap;
+
+       ret = qcom_cc_probe(pdev, &gcc_msm8939_desc);
+       if (ret)
+               return ret;
+
+       regmap = dev_get_regmap(&pdev->dev, NULL);
+       clk_pll_configure_sr_hpm_lp(&gpll3, regmap, &gpll3_config, true);
+       clk_pll_configure_sr_hpm_lp(&gpll4, regmap, &gpll4_config, true);

We should probably configure these before registering the clks. Can you
do the usual, map registers, configure stuff, and then
qcom_cc_really_probe()?

I think so. If there was a good reason to configure the plls after the 
registration, I can't recall what that was, maybe the original flow from 
downstream ...

quoted

+
+MODULE_DESCRIPTION("Qualcomm GCC MSM8939 Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:gcc-msm8939");

The module alias isn't needed right?

Nope g/msm8916/s//msm8939/g - I can zap that.

Thanks for the review.

---
bod