Messages in this thread | | | Subject | Re: [PATCH V6] clk: qcom: Add spmi_pmic clock divider support | From | Tirupathi Reddy T <> | Date | Tue, 21 Nov 2017 14:42:35 +0530 |
| |
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
| |