lkml.org 
[lkml]   [2016]   [Aug]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/3] clk: keystone: Add sci-clk driver support
    On 08/31, Tero Kristo wrote:
    > On 24/08/16 11:34, Stephen Boyd wrote:
    > >On 08/19, Nishanth Menon wrote:
    > >>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.

    > >>+ *
    > >>+ * 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.

    >
    > >>+ }
    > >>+
    > >>+ 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

    \
     
     \ /
      Last update: 2016-09-17 09:58    [W:8.450 / U:1.932 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site