Hi Rob, > On May 10, 2016, at 00:47 , Rob Herring <robherring2@xxxxxxxxx> wrote: > > On Mon, May 9, 2016 at 1:11 PM, Pantelis Antoniou > <pantelis.antoniou@xxxxxxxxxxxx> wrote: >> Insert overlay symbols to the base tree when applied. >> This makes it possible to apply an overlay that references a label >> that a previously inserted overlay had. >> >> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx> >> Tested-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> >> >> --- >> drivers/of/overlay.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 102 insertions(+) >> >> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c >> index a274d65..a7956a2 100644 >> --- a/drivers/of/overlay.c >> +++ b/drivers/of/overlay.c >> @@ -514,6 +514,101 @@ static int of_free_overlay_info(struct of_overlay *ov) >> return 0; >> } >> >> +static int of_overlay_add_symbols( >> + struct device_node *tree, >> + struct of_overlay *ov) >> +{ >> + struct of_overlay_info *ovinfo; >> + struct device_node *root_sym = NULL; >> + struct device_node *child = NULL; >> + struct property *prop; >> + const char *path, *s; >> + char *new_path; >> + int i, len, err; >> + >> + /* this may fail (if no fixups are required) */ >> + root_sym = of_find_node_by_path("/__symbols__"); >> + >> + /* do nothing if no root symbols */ >> + if (!root_sym) >> + return 0; >> + >> + /* locate the symbols & fixups nodes on resolve */ >> + for_each_child_of_node(tree, child) { >> + if (of_node_cmp(child->name, "__symbols__") == 0) >> + goto found; > > Doesn't of_get_child_by_name work here? > No, we’re holding of_mutex. >> + } >> + /* no symbols, no problem */ >> + of_node_put(root_sym); >> + return 0; >> + >> +found: > > As Marek said, get rid of the goto. > >> + err = -EINVAL; >> + for_each_property_of_node(child, prop) { >> + >> + /* skip properties added automatically */ >> + if (of_prop_cmp(prop->name, "name") == 0) >> + continue; >> + >> + err = of_property_read_string(child, >> + prop->name, &path); >> + if (err != 0) { >> + pr_err("%s: Could not find symbol '%s'\n", >> + __func__, prop->name); > > Can you introduce and use a common pr_fmt prefix here and elsewhere > (just what you are adding). OK > >> + continue; >> + } >> + >> + > > Extra blank line. > >> + /* now find fragment index */ >> + s = path; >> + >> + /* compare paths to find fragment index */ >> + ovinfo = NULL; >> + len = -1; > > Move these into the for init. > >> + for (i = 0; i < ov->count; i++) { >> + ovinfo = &ov->ovinfo_tab[i]; >> + >> + pr_debug("%s: #%d: overlay->name=%s target->name=%s\n", >> + __func__, i, ovinfo->overlay->full_name, >> + ovinfo->target->full_name); >> + >> + len = strlen(ovinfo->overlay->full_name); >> + if (strncasecmp(path, ovinfo->overlay->full_name, >> + len) == 0 && path[len] == '/') >> + break; >> + } >> + >> + if (i >= ov->count) >> + continue; >> + >> + pr_debug("%s: found target at #%d\n", __func__, i); >> + new_path = kasprintf(GFP_KERNEL, "%s%s", >> + ovinfo->target->full_name, >> + path + len); >> + if (!new_path) { >> + pr_err("%s: Failed to allocate propname for \"%s\"\n", >> + __func__, prop->name); >> + continue; > > If this fails, is there much point in continuing? > No >> + } >> + >> + err = of_changeset_add_property_string(&ov->cset, root_sym, >> + prop->name, new_path); >> + >> + /* free always */ >> + kfree(new_path); >> + >> + if (err) { > > No need for brackets. > >> + pr_err("%s: Failed to add property for \"%s\"\n", >> + __func__, prop->name); >> + } >> + } >> + >> + of_node_put(child); >> + of_node_put(root_sym); >> + >> + return 0; >> +} >> + >> static LIST_HEAD(ov_list); >> static DEFINE_IDR(ov_idr); >> >> @@ -642,6 +737,13 @@ static int __of_overlay_create(struct device_node *tree, >> goto err_abort_trans; >> } >> >> + err = of_overlay_add_symbols(tree, ov); >> + if (err) { >> + pr_err("%s: of_overlay_add_symbols() failed for tree@%s\n", >> + __func__, tree->full_name); >> + goto err_abort_trans; >> + } >> + >> /* apply the changeset */ >> err = __of_changeset_apply(&ov->cset); >> if (err) { >> -- >> 1.7.12 >> Regards — Pantelis -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html