On 01/22/2015 02:01 AM, Stephen Boyd wrote:
> On 01/21, Tomeu Vizoso wrote:
>> @@ -2075,10 +2210,12 @@ struct clk *clk_register(struct device *dev, struct
>> clk_hw *hw)
>> }
>> }
>>
>> - ret = __clk_init(dev, clk);
>> + hw->clk = __clk_create_clk(hw, NULL, NULL);
>> + ret = __clk_init(dev, hw->clk);
>> if (!ret)
>> - return clk;
>> + return hw->clk;
>>
>> + kfree(hw->clk);
>> fail_parent_names_copy:
>> while (--i >= 0)
>> kfree(clk->parent_names[i]);
>
> Sigh, this patch is so huge I keep finding more things. Sorry. It
> looks like __clk_create_clk() can return an error pointer, which
> we then send directly to __clk_init. First off, we shouldn't
> kfree() that pointer if it's an error pointer. Second, we
> shouldn't crash in __clk_init() in such a situation so there
> needs to be some sort of check somewhere.
Oops, done. I have reused the fail_parent_names_copy label as the
less-bad possibility. Probably the error labels should be named after
the target code and not after what the source code does, as per the
latest CodingStyle additions.
> BTW, please try and fixup checkpatch warnings.
What were you thinking of specifically? I'm running it with
--max-line-length=106 and the other warnings are in clk-test.c that I
still have to polish when I get some time.
>> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
>> index da4bda8..fac3244 100644
>> --- a/drivers/clk/clkdev.c
>> +++ b/drivers/clk/clkdev.c
>> @@ -69,20 +70,22 @@ struct clk *of_clk_get(struct device_node *np, int index)
> [...]
>> -struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
>> +static struct clk *__of_clk_get_by_name(struct device_node *np, const char
>> *name)
>
> It would be nice if this returned an already __clk_create_clk()ed
> pointer.
>
>> {
>> struct clk *clk = ERR_PTR(-ENOENT);
>>
>> @@ -119,7 +122,33 @@ struct clk *of_clk_get_by_name(struct device_node *np,
>> const char *name)
> [...]
>> +struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
>> +{
>> + struct clk *clk = __of_clk_get_by_name(np, name);
>> +
>> + if (!IS_ERR(clk))
>> + clk = __clk_create_clk(__clk_get_hw(clk), np->full_name, name);
>
> Because we do it here where we know we're CONFIG_COMMON_CLK=y.
>
>> +
>> + return clk;
>> +}
>> EXPORT_SYMBOL(of_clk_get_by_name);
>> +
>> +#else /* defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) */
>> +
>> +static struct clk *__of_clk_get_by_name(struct device_node *np, const char
>> *name)
>> +{
>> + return ERR_PTR(-ENOENT);
>> +}
>> #endif
>>
>> /*
>> @@ -185,9 +229,13 @@ struct clk *clk_get(struct device *dev, const char
>> *con_id)
>> struct clk *clk;
>>
>> if (dev) {
>> - clk = of_clk_get_by_name(dev->of_node, con_id);
>> - if (!IS_ERR(clk))
>> + clk = __of_clk_get_by_name(dev->of_node, con_id);
>> + if (!IS_ERR(clk)) {
>> +#if defined(CONFIG_COMMON_CLK)
>> + clk = __clk_create_clk(__clk_get_hw(clk), dev_id,
>> con_id);
>> +#endif
>
> And we do it here where we could remove the #ifdef.
Yeah, I tried to reduce the ifdefing back then and this is the simplest
I could come up with. The reason for clk_get() to call
__clk_create_clk() directly is that it has more relevant information
with which to tag the per-user clk.
of_clk_get_by_name() has the name of the node but not the dev_id, which
in my testing looked as much less useful when debugging who did what to
a clock.
Thanks,
Tomeu
>> return clk;
>> + }
>> if (PTR_ERR(clk) == -EPROBE_DEFER)
>> return clk;
>> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
