Re: [PATCH v4 2/9] clk: tegra: Add tegra specific clocks

[Stephen Warren <swarren@wwwdotorg.org>]

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.