Stephen Warren <swarren@xxxxxxxxxxxxx> wrote @ Fri, 11 Jan 2013 22:35:28 +0100:
> 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@xxxxxxxxxx> 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;
> > };
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
