Hi Simon,
On Thu, Apr 12, 2018 at 6:42 PM, Simon Glass <s...@chromium.org> wrote:
> 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.
>
OK, I'll use that approach in v2.
>>
>>>> + 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
Best regards,
Mario
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
