Re: [PATCH V6] clk: qcom: Add spmi_pmic clock divider support

[Tirupathi Reddy T <tirupath@codeaurora.org>]

On 11/18/2017 5:26 AM, Stephen Boyd wrote:
> On 11/17, Tirupathi Reddy wrote:
>> diff --git a/Documentation/devicetree/bindings/clock/clk-spmi-pmic-div.txt b/Documentation/devicetree/bindings/clock/clk-spmi-pmic-div.txt
>> new file mode 100644
>> index 0000000..2cf2aba
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/clk-spmi-pmic-div.txt
> Please move this to qcom,spmi-clkdiv.txt like other qcom bindings.
Addressed in next patch version[7]
>
>> @@ -0,0 +1,59 @@
>> +Qualcomm Technologies, Inc. SPMI PMIC clock divider (clkdiv)
>> +
>> +clkdiv configures the clock frequency of a set of outputs on the PMIC.
>> +These clocks are typically wired through alternate functions on
>> +gpio pins.
>> +
>> +=======================
>> +Properties
>> +=======================
>> +
>> +- compatible
>> +	Usage:      required
>> +	Value type: <string>
>> +	Definition: must be "qcom,spmi-clkdiv".
>> +
>> +- reg
>> +	Usage:      required
>> +	Value type: <prop-encoded-array>
>> +	Definition: base address of CLKDIV peripherals.
>> +
>> +- qcom,num-clkdivs
>> +	Usage:      required
>> +	Value type: <u32>
>> +	Definition: number of CLKDIV peripherals.
> Would it work if we read the registers and looked for the clkdiv
> subtype ID? If we read something that doesn't match, break and
> stop adding clks? Or does reading the next "peripheral" cause
> some sort of crash and burn scenario where the device see an
> access control violation? Would be interesting to do it that way
> and avoid needing a new property in DT.
I feel it is better to go with the current design as we beforehand know the
#div-clk peripherals in a PMIC. Reading the next peripheral might result in
an unknown failure if there is no real peripheral there (PMIC peripherals
are not in contiguous address space).
>> +
>> +
>> +	parent_name = of_clk_get_parent_name(of_node, 0);
>> +	if (!parent_name) {
>> +		dev_err(dev, "missing parent clock\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	init.parent_names = &parent_name;
>> +	init.num_parents = 1;
>> +	init.ops = &clk_spmi_pmic_div_ops;
>> +
>> +	for (i = 0, clkdiv = cc->clks; i < nclks; i++) {
>> +		spin_lock_init(&clkdiv[i].lock);
>> +		clkdiv[i].base = start + i * 0x100;
>> +		clkdiv[i].regmap = regmap;
>> +		clkdiv[i].cxo_period_ns = NSEC_PER_SEC / cxo_hz;
>> +		init.name = kasprintf(GFP_KERNEL, "div_clk%d", i + 1);
> Hmm. Maybe we should just have this on the stack. 20 characters
> should be plenty?
Addressed in next patch version[7]
>> +		clkdiv[i].hw.init = &init;
>> +
>> +		ret = devm_clk_hw_register(dev, &clkdiv[i].hw);
>> +		kfree(init.name); /* clk framework made a copy */
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return of_clk_add_hw_provider(of_node, spmi_pmic_div_clk_hw_get, cc);
>> +}
>> +
>> +static int spmi_pmic_clkdiv_remove(struct platform_device *pdev)
>> +{
>> +	of_clk_del_provider(pdev->dev.of_node);
>> +
>> +	return 0;
> This can use devm now.
Addressed in next patch version.
>> +}
>> +
>> +static const struct of_device_id spmi_pmic_clkdiv_match_table[] = {
>> +	{ .compatible = "qcom,spmi-clkdiv" },
>> +	{ /* sentinel */ }
>> +};
> Nice!
Thanks