From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 
Sent: Monday, October 31, 2016 12:28 PM

quoted

From: Noam Camus <redacted>

nps_setup_clocksource() should take node as only argument i.e.:
replace
int __init nps_setup_clocksource(struct device_node *node, struct clk 
*clk) with int __init nps_setup_clocksource(struct device_node *node)

This is also serve as preperation for next patch which adds support

s/preperation/preparation/

Thanks, will fix in V4 of this patch set
...

quoted

+static int nps_get_timer_clk(struct device_node *node,
+			     unsigned long *timer_freq,
+			     struct clk *clk)

This function prototype does not make sense. A pointer to a clock is passed for nothing here.

Thanks, I passed *clk in order for one to do rollback on error (pass clk to clk_disable_unprepare).
I will change prototype to **clk.

quoted

+{
+	int ret;
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk)) {
+		pr_err("timer missing clk");
+		return PTR_ERR(clk);
+	}
+
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		pr_err("Couldn't enable parent clk\n");
+		return ret;
+	}
+
+	*timer_freq = clk_get_rate(clk);
+

timer_freq check.

rollback on error.

Thanks, will fix at V4

...

quoted

-	if (ret) {
-		pr_err("Couldn't enable parent clock\n");
-		return ret;
-	}
-
-	nps_timer_rate = clk_get_rate(clk);
+	nps_get_timer_clk(node, &nps_timer_rate, clk);

Return code check ?

Thanks, will fix at V4
  
-Noam