[U-Boot] [PATCH 2/3] core: Add functions to set properties in live-tree

[Simon Glass]

Hi Mario,

On 10 April 2018 at 05:23, Mario Six <mario.six at gdsys.cc> wrote:
> Hi Simon,
>
> On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Mario,
>>
>> On 28 March 2018 at 20:37, Mario Six <mario.six at 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.six at 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