Stephen Warren [off-list ref] wrote @ Fri, 11 Jan 2013 22:35:28 +0100:

On 01/11/2013 04:48 AM, Hiroshi Doyu wrote:

quoted

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.

quoted

Prashant Gaikwad [off-list ref] wrote @ Fri, 11 Jan 2013 08:46:20 +0100:

quoted

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).

quoted

quoted

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

quoted

quoted

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

quoted

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

Also since "init" is allocated from stack, members are expected to be
initialized *zero*. The last 2 lines are not necessary in the above.

         struct clk_init_data init = {
                 .ops = &tegra_clk_sync_source_ops;
                 .name = name;
                 .flags = CLK_IS_ROOT;
         };

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

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

OK

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