lkml.org 
[lkml]   [2013]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4 2/9] clk: tegra: Add tegra specific clocks
    On 01/11/2013 04:48 AM, Hiroshi Doyu wrote:
    > Hi Prahant,
    >
    > Some nit-pick/cosmetic comments inlined...

    FYI, Prashant is on vacation for the next week or two, so I'll take over
    this series to clean up any last review comments.

    > Prashant Gaikwad <pgaikwad@nvidia.com> wrote @ Fri, 11 Jan 2013 08:46:20 +0100:
    >
    >> Add tegra specific clocks, pll, pll_out, peripheral,
    >> frac_divider, super.

    (it's a good idea to quote as little text as possible; paging through
    the whole patch to find your comments was slightly painful).

    >> diff --git a/drivers/clk/tegra/clk-audio-sync.c b/drivers/clk/tegra/clk-audio-sync.c

    >> +struct clk *tegra_clk_sync_source(const char *name, unsigned long rate,
    >> + unsigned long max_rate)
    >> +{
    >> + struct tegra_clk_sync_source *sync;
    >> + struct clk_init_data init;
    >> + struct clk *clk;
    >> +
    >> + sync = kzalloc(sizeof(struct tegra_clk_sync_source), GFP_KERNEL);
    >> + if (!sync) {
    >> + pr_err("%s: could not allocate sync source clk\n", __func__);
    >> + return ERR_PTR(-ENOMEM);
    >> + }
    >> +
    >> + sync->rate = rate;
    >> + sync->max_rate = max_rate;
    >> +
    >> + init.ops = &tegra_clk_sync_source_ops;
    >> + init.name = name;
    >> + init.flags = CLK_IS_ROOT;
    >> + init.parent_names = NULL;
    >> + init.num_parents = 0;
    >> +
    >> + sync->hw.init = &init;
    >> +
    >> + clk = clk_register(NULL, &sync->hw);
    >
    > The above usage of "init" from stack may be a bit
    > unfamilier. I can guess that its content is copied in clk_register()
    > but it's originally defined in stack. So I just prefer to writing this
    > as below. It may be somewhat explict that we know init is from stack.

    The issue you mention is more about whether "init" is copied from the
    stack or not; simplying changing the initialization to:

    > struct clk *tegra_clk_sync_source(const char *name, unsigned long rate,
    > unsigned long max_rate)
    > {
    > struct tegra_clk_sync_source *sync;
    > struct clk_init_data init = {
    > .ops = &tegra_clk_sync_source_ops;
    > .name = name;
    > .flags = CLK_IS_ROOT;
    > .parent_names = NULL;
    > .num_parents = 0;
    > };

    ... doesn't really address that, although it's a perfectly reasonable
    change.

    To address the copying issue, why not just add a comment:

    /*
    * This data pointed at by this field is copied by
    * clk_register(), so a pointer to the stack is OK.
    */
    sync->hw.init = &init;

    clk = clk_register(NULL, &sync->hw);

    I'll make the other changes you suggested.


    \
     
     \ /
      Last update: 2013-01-11 23:21    [W:3.737 / U:0.700 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site