On 08/31, Tero Kristo wrote:

On 24/08/16 11:34, Stephen Boyd wrote:

quoted

On 08/19, Nishanth Menon wrote:

quoted

diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
new file mode 100644
index 000000000000..6c43e097e6d6
--- /dev/null
+++ b/drivers/clk/keystone/sci-clk.c

@@ -0,0 +1,539 @@
+	return (int)new_rate;

determine rate should return a negative number on failure and 0
on success. The actual rate that was found should go into
req->rate. This looks broken.

Yea it seems broken, I wonder how we haven't seen any issues with
this in testing.... Apparently positive return values from this are
interpreted as success. Having a quick look at clk.c seems to
confirm this.

Anyway, will fix.

True, perhaps we should fix that so we don't use a long to hold
the int return value either.

quoted

quoted

+ *
+ * Gets a handle to an existing TI SCI clock, or builds a new clock
+ * entry and registers it with the common clock framework. Called from
+ * the common clock framework, when a corresponding of_clk_get call is
+ * executed, or recursively from itself when parsing parent clocks.
+ * Returns a pointer to the clock struct, or ERR_PTR value in failure.
+ */

Please move this driver to clk_hw_register() and friends. This on
the fly clk generation is scary considering how we hold locks
while the provider is asked to give us the pointer, so allocating
and registering clks (basically reentering the CCF again) could
lead to a locking nightmare. Best to avoid that.

Ok, so just converting the driver to use provider->get_hw should be
enough? This seems to be a relatively new API in the CCF. Will look
at that.

Hopefully it will simplify things greatly.

quoted

quoted

+	}
+
+	snprintf(name, 20, "%s:%d:%d", dev_name(provider->dev), sci_clk->dev_id,
+		 sci_clk->clk_id);

I hope we don't make dev_name() longer than 20 characters

Shall I just increase the size of the buffer and add a length check?
Using kmalloc or something here seems overkill, as the name gets
copied by CCF anyway.

There's kasprintf() which would always make it long enough. I
don't know if it really matters though.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html