Hi Jon,
On Mon, Mar 7, 2016 at 11:45 AM, Jon Hunter <jonathanh@xxxxxxxxxx> wrote:
> On 07/03/16 08:48, Geert Uytterhoeven wrote:
>> On Fri, Mar 4, 2016 at 4:33 PM, Jon Hunter <jonathanh@xxxxxxxxxx> wrote:
>>> The PM clocks framework requires clients to pass either a con-id or a
>>> valid clk pointer in order to add a clock to a device. Add a new
>>> function pm_clk_add_clks_of() to allows device clocks to be retrieved
>>> from device-tree and populated for a given device. Note that there
>>> should be no need to add a stub function for this new function when
>>> CONFIG_OF is not selected because the OF functions used already have
>>> stubs functions.
>>>
>>> In order to handle errors encountered when adding clocks from
>>> device-tree, add a function pm_clk_remove_clk() to remove any clocks
>>> (using a pointer to the clk structure) that have been added
>>> successfully before the error occurred.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@xxxxxxxxxx>
>>> ---
>>> drivers/base/power/clock_ops.c | 85 ++++++++++++++++++++++++++++++++++++++++++
>>> include/linux/pm_clock.h | 9 +++++
>>> 2 files changed, 94 insertions(+)
>>>
>>> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
>>> index 272a52ebafc0..5aa7365699c1 100644
>>> --- a/drivers/base/power/clock_ops.c
>>> +++ b/drivers/base/power/clock_ops.c
>>> @@ -138,6 +138,58 @@ int pm_clk_add_clk(struct device *dev, struct clk *clk)
>>> }
>>>
>>> /**
>>> + * pm_clk_add_clks_of - Start using device clock(s) for power management.
>>> + * @dev: Device whose clock(s) is going to be used for power management.
>>> + *
>>> + * Add a series of clocks described in the 'clocks' device-tree node for
>>> + * a device to the list of clocks used for the power management of @dev.
>>
>> This adds all clocks referenced by the device node, while not all clocks may
>> be suitable for power management, and some of them may be under explicit
>> driver control.
>> Cfr. drivers/clk/shmobile/renesas-cpg-mssr.c:cpg_mssr_attach_dev(), which
>> looks for module clocks, or core clocks explicitly listed in core_pm_clks[].
>>
>> What about adding an optional filter function to filter for suitable clocks?
>
> Yes that sounds reasonable. Are you filtering on clock ID? Would it work
> to allow the caller to pass a filter function which takes a struct *clk
> as it's only argument? Or do you prefer we call
> of_parse_phandle_with_args(np, "clocks", "#clock-cells", i, &clkspec)
> and return the clkspec info?
cpg_mssr_attach_dev() filters on clkspec.
As pm_clk_add_clks_of() has "_of" in the name (BTW, should it be called
of_pm_clk_add_clks()), I think passing the clkspec makes more sense than
passing a struct clk *.
>>> +int pm_clk_add_clks_of(struct device *dev)
>>> +{
>>> + struct clk **clks;
>>> + unsigned int i, count;
>>> + int ret;
>>> +
>>> + if (!dev || !dev->of_node)
>>> + return -EINVAL;
>>> +
>>> + count = of_count_phandle_with_args(dev->of_node, "clocks",
>>> + "#clock-cells");
>>> + if (count == 0)
>>> + return -ENODEV;
>>> +
>>> + clks = kcalloc(count, sizeof(*clks), GFP_KERNEL);
>>> + if (!clks)
>>> + return -ENOMEM;
>>> +
>>
>> Don't you have to call pm_clk_create() here, to allocate a pm_subsys_data
>> structure that pm_clk_add_clk() can populate?
>
> This function will return an error when pm_clk_add_clk() is called if
> the user has not called pm_clk_create() first. It seems more natural to
> me that the user should call pm_clk_create() before calling
> pm_clk_add_clks_of() which is the same convention used for adding clocks
> by the other APIs to add clocks.
OK. That way the caller can add more clocks if needed. I missed that case.
>>> + for (i = 0; i < count; i++) {
>>> + clks[i] = of_clk_get(dev->of_node, i);
>>> + if (IS_ERR(clks[i])) {
>>> + ret = PTR_ERR(clks[i]);
>>> + goto error;
>>> + }
>>> +
>>> + ret = pm_clk_add_clk(dev, clks[i]);
>>> + if (ret) {
>>> + clk_put(clks[i]);
>>> + goto error;
>>> + }
>>> + }
>>> +
>>> + kfree(clks);
>>> +
>>> + return 0;
>>> +
>>> +error:
>>> + while (i--)
>>> + pm_clk_remove_clk(dev, clks[i]);
>>
>> Just pm_clk_destroy(dev) to destroy all at once?
>
> Right, however, this will remove all clocks associated with the device
> and not just those we have added. It is probably very unlikely that
> someone would call pm_clk_add_clks_of() and had previously called
> pm_clk_add(), but it seems best to not make any assumptions and only
> remove the clocks that were actually added. I guess that it would be
> possible to check to see if there were any clocks already added when
> this function is called and return an error in that case.
>
> All of that said, I do agree that it may be simpler, if we do call
> pm_clk_create() from within pm_clk_add_clks_of() and this would allow us
> to pmc_clk_destroy() in the error path. So if this is preferred I could
> do that, but I think that I would not allow clocks to be added if
> clock_list is not empty when this function is called.
Given the above, your code is fine.
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
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
