On 31/08/2022 05.08, Simon Glass wrote:
Add this feature to the ofnode interface, supporting both livetree and flattree.
Signed-off-by: Simon Glass sjg@chromium.org
drivers/core/of_access.c | 50 ++++++++++++++++++++++++++++++++++++++++ drivers/core/ofnode.c | 30 ++++++++++++++++++++++++ include/dm/of_access.h | 12 ++++++++++ include/dm/ofnode.h | 12 ++++++++++ test/dm/ofnode.c | 17 ++++++++++++++ 5 files changed, 121 insertions(+)
diff --git a/drivers/core/of_access.c b/drivers/core/of_access.c index ee09bbb7550..765b21cecdb 100644 --- a/drivers/core/of_access.c +++ b/drivers/core/of_access.c @@ -931,3 +931,53 @@ int of_write_prop(struct device_node *np, const char *propname, int len,
return 0; }
+int of_add_subnode(struct device_node *parent, const char *name, int len,
struct device_node **childp)+{
- struct device_node *child, *new, *last_sibling = NULL;
- char *new_name, *full_name;
- int parent_fnl;
- __for_each_child_of_node(parent, child) {
if (!strncmp(child->name, name, len) && strlen(name) == len)
I'm confused. The documentation comment further down says that len is the length of name. So why this check? And further, the only caller (at least introduced here, haven't been through the whole series) indeed passes strlen(name).
So why is that even a parameter? Can't of_add_subnode() just do len=strlen(name) itself? And then of course the strlen(name)==len condition goes away, and the strncmp() can just be spelled strcmp() - which also looks much more natural when asking "does such a child node already exist".
return -EEXIST;
It seems natural to provide the existing node in *childp when -EEXIST. Callers that want to error out on -EEXIST can just do that and ignore that value, but callers that just wants to ensure there is a subnode with the given name can avoid doing a find_subnode() upfront, which should make much code much more compact.
last_sibling = child;- }
- /* Property does not exist -> append new property */
s/property/[sub]node/, or just delete the comment, it's obvious.
- new = calloc(1, sizeof(struct device_node));
- if (!new)
return -ENOMEM;- new_name = malloc(len + 1);
- if (!name) {
That's not the variable you want to test...
free(new);return -ENOMEM;- }
- strlcpy(new_name, name, len + 1);
- new->name = new_name;
new->name = memdup(name, len+1); if (!new->name) {...}
is a bit shorter.
- parent_fnl = parent->name ? strlen(parent->full_name) : 0;
- full_name = calloc(1, parent_fnl + 1 + len + 1);
- if (!full_name) {
free(new_name);free(new);return -ENOMEM;- }
- strcpy(full_name, parent->full_name);
Confused. Above, we use parent->name as a condition for whether parent->full_name exists (or at least whether we should compute its strlen). Here we unconditionally access parent->full_name. I don't know the rules for the root node, but I can kind of guess that it has ->name NULL and ->full_name set to "". Perhaps that could be clarified in the documentation for struct device_node?
And if I'm right, that means there's no point asking if parent->name is NULL or not; strlen(parent->full_name) gives the right answer regardless.
Rasmus