Hi Mario, On 10 April 2018 at 05:23, Mario Six <mario....@gdsys.cc> wrote: > 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.
Right, but they should still compile? > >>> +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. Yes. Then they could return -ENOSYS, for example. Then we at least have a consistent API for both live/flat tree, instead of them splitting away from each other. > >>> + 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()? Yes >> >>> + >>> +/** >>> + * 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 Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot