Re: [PATCH v3 5/6] clk: qcom: Add MSM8998 Multimedia Clock Controller (MMCC) driver

[Jeffrey Hugo]

On 4/30/2019 9:43 PM, Bjorn Andersson wrote:
> On Tue 30 Apr 19:27 PDT 2019, Jeffrey Hugo wrote:
> > +static const struct of_device_id mmcc_msm8998_match_table[] = {
> > +	{ .compatible = "qcom,mmcc-msm8998" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, mmcc_msm8998_match_table);
> > +
> > +static int mmcc_msm8998_probe(struct platform_device *pdev)
> > +{
> > +	struct regmap *regmap;
> > +
>
> Don't you want to wait for "xo" here as well?

No, I don't want to.  As far as I recall, Stephen would like to make a 
clear divide between clock providers, and clock consumers.  Since we 
have the uart issue in gcc, and gcc is pretty critical to the entire 
SoC, it seems like there is a reason (not sure I'd call it "good") to 
wait for xo there.

Here, I'm less confident in the reasoning.  mmcc is not really critical 
to the SoC, and everything it services is "optional".  If you have a 
headless system with no display output, you won't even need it.  On 
system where there is a display, I expect the realistic driver ordering 
to be that everything which consumes a mmcc clock to come up well after 
xo is available.

In short, seems like a bit of a kludge to maybe avoid an issue which 
doesn't seem like would happen.

> > +	regmap = qcom_cc_map(pdev, &mmcc_msm8998_desc);
> > +	if (IS_ERR(regmap))
> > +		return PTR_ERR(regmap);
> > +
> > +	return qcom_cc_really_probe(pdev, &mmcc_msm8998_desc, regmap);
> > +}
> [..]
> > +MODULE_DESCRIPTION("QCOM MMCC MSM8998 Driver");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:mmcc-msm8998");
>
> MODULE_DEVICE_TABLE() will provide the alias for module auto loading, so
> drop this.

Huh.  I did not know that.  Will put on the list to fixup.

> Regards,
> Bjorn


--
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.