Hi Simon, On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass <s...@chromium.org> wrote: > Hi Mario, > > On 28 March 2018 at 20:37, Mario Six <mario....@gdsys.cc> wrote: >> Implement a set of functions to manipulate properties in a live device >> tree: >> >> * ofnode_set_property() to set generic properties of a node >> * ofnode_write_string() to set string properties of a node >> * ofnode_enable() to enable a node >> * ofnode_disable() to disable a node >> > > Please add a test for these functions. > >> Signed-off-by: Mario Six <mario....@gdsys.cc> >> --- >> drivers/core/ofnode.c | 58 >> +++++++++++++++++++++++++++++++++++++++++++++++++++ >> include/dm/ofnode.h | 49 +++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 107 insertions(+) >> >> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c >> index ca002063b3..90d05aa559 100644 >> --- a/drivers/core/ofnode.c >> +++ b/drivers/core/ofnode.c >> @@ -699,3 +699,61 @@ u64 ofnode_translate_address(ofnode node, const fdt32_t >> *in_addr) >> else >> return fdt_translate_address(gd->fdt_blob, >> ofnode_to_offset(node), in_addr); >> } >> + >> +#ifdef CONFIG_OF_LIVE > > Is this needed? >
See below, these functions don't make sense if there is no livetree. >> +int ofnode_set_property(ofnode node, const char *propname, int len, >> + const void *value) >> +{ >> + const struct device_node *np = ofnode_to_np(node); >> + struct property *pp; >> + struct property *new; >> + >> + if (!np) >> + return -EINVAL; >> + >> + for (pp = np->properties; pp; pp = pp->next) { >> + if (strcmp(pp->name, propname) == 0) { >> + /* Property exists -> change value */ >> + pp->value = (void *)value; >> + pp->length = len; >> + return 0; >> + } >> + } >> + >> + /* Property does not exist -> append new property */ >> + new = malloc(sizeof(struct property)); >> + >> + new->name = strdup(propname); >> + new->value = malloc(len); >> + memcpy(new->value, value, len); >> + new->length = len; >> + new->next = NULL; >> + >> + pp->next = new; >> + >> + return 0; >> +} >> + >> +int ofnode_write_string(ofnode node, const char *propname, const char >> *value) >> +{ >> + assert(ofnode_valid(node)); > > What does this function do if livetree is not enabled? > The idea was to not make them available if livetree is not enabled (hence the #ifdef CONFIG_OF_LIVE). Making them nops in case livetree is not available would work as well if that's preferable. >> + debug("%s: %s = %s", __func__, propname, value); >> + >> + return ofnode_set_property(node, propname, strlen(value) + 1, value); >> +} >> + >> +int ofnode_enable(ofnode node) >> +{ >> + assert(ofnode_valid(node)); >> + >> + return ofnode_write_string(node, "status", "okay"); >> +} >> + >> +int ofnode_disable(ofnode node) >> +{ >> + assert(ofnode_valid(node)); >> + >> + return ofnode_write_string(node, "status", "disable"); >> +} >> + >> +#endif >> diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h >> index aec205eb80..e82ebf19c5 100644 >> --- a/include/dm/ofnode.h >> +++ b/include/dm/ofnode.h >> @@ -689,4 +689,53 @@ int ofnode_read_resource_byname(ofnode node, const char >> *name, >> * @return the translated address; OF_BAD_ADDR on error >> */ >> u64 ofnode_translate_address(ofnode node, const fdt32_t *in_addr); >> + >> +#ifdef CONFIG_OF_LIVE >> + >> +/** >> + * ofnode_set_property() - Set a property of a ofnode >> + * >> + * @node: The node for whose property should be set >> + * @propname: The name of the property to set >> + * @len: The length of the new value of the property >> + * @value: The new value of the property >> + * @return 0 if successful, -ve on error >> + */ >> +int ofnode_set_property(ofnode node, const char *propname, int len, >> + const void *value); > > We should think about consistency here. Maybe > > ofnode_write_prop()? > >> + >> +/** >> + * ofnode_write_string() - Set a string property of a ofnode >> + * >> + * @node: The node for whose string property should be set >> + * @propname: The name of the string property to set >> + * @value: The new value of the string property >> + * @return 0 if successful, -ve on error >> + */ >> +int ofnode_write_string(ofnode node, const char *propname, const char >> *value); >> + >> +/** >> + * ofnode_enable() - Enable a device tree node given by its ofnode >> + * >> + * This function effectively sets the node's "status" property to "okay", >> hence >> + * making it available for driver model initialization. >> + * >> + * @node: The node to enable >> + * @return 0 if successful, -ve on error >> + */ >> +int ofnode_enable(ofnode node); >> + >> +/** >> + * ofnode_disable() - Disable a device tree node given by its ofnode >> + * >> + * This function effectively sets the node's "status" property to "disable", >> + * hence stopping it from being picked up by driver model initialization. >> + * >> + * @node: The node to disable >> + * @return 0 if successful, -ve on error >> + */ >> +int ofnode_disable(ofnode node); > > Would it be OK to have one function like ofnode_set_enabled(ofnode > node, bool enable) ? > > Regards, > Simon Everything else will be addressed in v2. Thanks for reviewing! Best regards, Mario _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot