Re: [PATCH 2/9] clk: Introduce get_parent_hw clk op
From: Jeffrey Hugo
Date: Mon Feb 11 2019 - 11:09:10 EST
On 1/28/2019 11:10 PM, Stephen Boyd wrote:
The clk_ops::get_parent function is limited in ability to return errors
because it returns a u8. A "workaround" to return an error is to return
a number >= the number of parents of a clk. This will in turn cause the
framework to "orphan" the clk and make the parent of the clk NULL. This
isn't really correct, because if an error occurs while reading the
parents of a clk we should fail the clk registration, instead of
orphaning the clk and waiting for the clk to appear later.
We really need to have three different return values from the get_parent
clk op. Something valid for a clk that exists, something invalid for a
clk that doesn't exist and never will exist or can't be determined
because the register operation to read the parent failed, and something
for a clk that doesn't exist because the framework doesn't know about
what it is. Introduce a new clk_op that can express all of this by
returning a pointer to the clk_hw of the parent. It's expected that clk
provider drivers will return a valid pointer when the parent is
findable, an error pointer like EPROBE_DEFER if their parent provider
hasn't probed yet but is valid, a NULL pointer if they can't find the
clk but index is valid, and an error pointer with an appropriate error
code otherwise.
Cc: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
Cc: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
Cc: Russell King <linux@xxxxxxxxxxxxxxx>
Cc: Michael Turquette <mturquette@xxxxxxxxxxxx>
Signed-off-by: Stephen Boyd <sboyd@xxxxxxxxxx>
<snip>
+static int clk_init_parent(struct clk_core *core)
+{
+ core->parent = __clk_init_parent(core, true);
+ if (IS_ERR(core->parent))
+ return PTR_ERR(core->parent);
+
+ /*
+ * Populate core->parent if parent has already been clk_core_init'd. If
+ * parent has not yet been clk_core_init'd then place clk in the orphan
+ * list. If clk doesn't have any parents then place it in the root
+ * clk list.
+ *
+ * Every time a new clk is clk_init'd then we walk the list of orphan
+ * clocks and re-parent any that are children of the clock currently
+ * being clk_init'd.
+ */
+ if (core->parent) {
+ hlist_add_head(&core->child_node,
+ &core->parent->children);
+ core->orphan = core->parent->orphan;
+ } else if (!core->num_parents) {
+ hlist_add_head(&core->child_node, &clk_root_list);
+ core->orphan = false;
+ } else {
+ hlist_add_head(&core->child_node, &clk_orphan_list);
Missing "core->orphan = true;"?
The snippet below had that line. Its not clear why it appears to be
dropped here.
+ }
+
+ return 0;
+}
<snip>
@@ -3073,29 +3143,9 @@ static int __clk_core_init(struct clk_core *core)
"%s: invalid NULL in %s's .parent_names\n",
__func__, core->name);
- core->parent = __clk_init_parent(core);
-
- /*
- * Populate core->parent if parent has already been clk_core_init'd. If
- * parent has not yet been clk_core_init'd then place clk in the orphan
- * list. If clk doesn't have any parents then place it in the root
- * clk list.
- *
- * Every time a new clk is clk_init'd then we walk the list of orphan
- * clocks and re-parent any that are children of the clock currently
- * being clk_init'd.
- */
- if (core->parent) {
- hlist_add_head(&core->child_node,
- &core->parent->children);
- core->orphan = core->parent->orphan;
- } else if (!core->num_parents) {
- hlist_add_head(&core->child_node, &clk_root_list);
- core->orphan = false;
- } else {
- hlist_add_head(&core->child_node, &clk_orphan_list);
- core->orphan = true;
- }
+ ret = clk_init_parent(core);
+ if (ret)
+ goto out;
/*
* optional platform-specific magic
--
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.