Re: [PATCHv10 15/41] CLK: TI: DRA7: Add APLL support

[Alexander Aring <alex.aring@gmail.com>]

Hi,

On Fri, Nov 29, 2013 at 09:00:29PM +0200, Tero Kristo wrote:
> On 11/26/2013 10:51 AM, Alexander Aring wrote:
> >Hi,
> >
> >On Tue, Nov 26, 2013 at 10:05:56AM +0200, Tero Kristo wrote:
> >>From: J Keerthy <j-keerthy@ti.com>
> >>
> >>The patch adds support for DRA7 PCIe APLL. The APLL
> >>sources the optional functional clocks for PCIe module.
> >>
> >>APLL stands for Analog PLL. This is different when comapred
> >>with DPLL meaning Digital PLL, the phase detection is done
> >>using an analog circuit.
> >>
> >>Signed-off-by: J Keerthy <j-keerthy@ti.com>
> >>Signed-off-by: Tero Kristo <t-kristo@ti.com>
> >>---
> >>  .../devicetree/bindings/clock/ti/apll.txt          |   31 +++
> >>  drivers/clk/ti/Makefile                            |    2 +-
> >>  drivers/clk/ti/apll.c                              |  239 ++++++++++++++++++++
> >>  3 files changed, 271 insertions(+), 1 deletion(-)
> >>  create mode 100644 Documentation/devicetree/bindings/clock/ti/apll.txt
> >>  create mode 100644 drivers/clk/ti/apll.c
> >>
> >>diff --git a/Documentation/devicetree/bindings/clock/ti/apll.txt b/Documentation/devicetree/bindings/clock/ti/apll.txt
> >>new file mode 100644
> >>index 0000000..7faf5a6
> >...
> >>+
> >>+static int __init of_dra7_apll_setup(struct device_node *node)
> >>+{
> >>+	const struct clk_ops *ops;
> >>+	struct clk *clk;
> >>+	const char *clk_name = node->name;
> >>+	int num_parents;
> >>+	const char **parent_names = NULL;
> >>+	u8 apll_flags = 0;
> >>+	struct dpll_data *ad;
> >>+	u32 idlest_mask = 0x1;
> >>+	u32 autoidle_mask = 0x3;
> >>+	int i;
> >>+	int ret;
> >>+
> >>+	ops = &apll_ck_ops;
> >>+	ad = kzalloc(sizeof(*ad), GFP_KERNEL);
> >>+	if (!ad)
> >>+		return -ENOMEM;
> >>+
> >>+	of_property_read_string(node, "clock-output-names", &clk_name);
> >>+
> >>+	num_parents = of_clk_get_parent_count(node);
> >>+	if (num_parents < 1) {
> >>+		pr_err("dra7 apll %s must have parent(s)\n", node->name);
> >>+		ret = -EINVAL;
> >>+		goto cleanup;
> >>+	}
> >>+
> >>+	parent_names = kzalloc(sizeof(char *) * num_parents, GFP_KERNEL);
> >>+
> >>+	for (i = 0; i < num_parents; i++)
> >>+		parent_names[i] = of_clk_get_parent_name(node, i);
> >>+
> >>+	ad->clk_ref = of_clk_get(node, 0);
> >>+	ad->clk_bypass = of_clk_get(node, 1);
> >>+
> >>+	if (IS_ERR(ad->clk_ref)) {
> >>+		pr_debug("ti,clk-ref for %s not found\n", clk_name);
> >>+		ret = -EAGAIN;
> >>+		goto cleanup;
> >>+	}
> >>+
> >>+	if (IS_ERR(ad->clk_bypass)) {
> >>+		pr_debug("ti,clk-bypass for %s not found\n", clk_name);
> >>+		ret = -EAGAIN;
> >>+		goto cleanup;
> >>+	}
> >>+
> >>+	ad->control_reg = ti_clk_get_reg_addr(node, 0);
> >>+	ad->idlest_reg = ti_clk_get_reg_addr(node, 1);
> >>+
> >>+	if (!ad->control_reg || !ad->idlest_reg) {
> >>+		ret = -EINVAL;
> >>+		goto cleanup;
> >>+	}
> >>+
> >>+	ad->idlest_mask = idlest_mask;
> >>+	ad->enable_mask = autoidle_mask;
> >>+
> >>+	clk = omap_clk_register_apll(NULL, clk_name, parent_names,
> >>+				num_parents, apll_flags, ad,
> >>+				NULL, ops);
> >>+
> >>+	if (!IS_ERR(clk)) {
> >>+		of_clk_add_provider(node, of_clk_src_simple_get, clk);
> >>+		return 0;
> >>+	}
> >>+
> >
> >Should we not also here do a cleanup for allocated memory?
> >
> >>+	return PTR_ERR(clk);
> 
> Yes, this should be changed to be ret = PTR_ERR(clk);
> 
ahh, ok. Just figure this out... I saw this on other patches in your
patchstack too sometimes. Please check this. :-)

- Alex