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