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

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 14/05/2020 22:31, Stephen Boyd wrote:
> Quoting Bryan O'Donoghue (2020-05-12 04:50:23)
>> 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 <bjorn.andersson@linaro.org>
>> 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 <shawn.guo@linaro.org>
> 
> 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 <shawn.guo@linaro.org>

for v5.

>> +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 ...

>> +
>> +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