Re: [U-Boot] [PATCH 2/3] core: Add functions to set properties in live-tree
Hi Simon,
On Thu, Apr 12, 2018 at 6:42 PM, Simon Glass wrote:
> Hi Mario,
>
> On 10 April 2018 at 05:23, Mario Six wrote:
>> Hi Simon,
>>
>> On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass wrote:
>>> Hi Mario,
>>>
>>> On 28 March 2018 at 20:37, Mario Six 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
---
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
+ * @
Re: [U-Boot] [PATCH 2/3] core: Add functions to set properties in live-tree
Hi Mario,
On 10 April 2018 at 05:23, Mario Six wrote:
> Hi Simon,
>
> On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass wrote:
>> Hi Mario,
>>
>> On 28 March 2018 at 20:37, Mario Six 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
>>> ---
>>> 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 b
Re: [U-Boot] [PATCH 2/3] core: Add functions to set properties in live-tree
Hi Simon,
On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass wrote:
> Hi Mario,
>
> On 28 March 2018 at 20:37, Mario Six 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
>> ---
>> 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 s
Re: [U-Boot] [PATCH 2/3] core: Add functions to set properties in live-tree
Hi Mario,
On 28 March 2018 at 20:37, Mario Six 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
> ---
> 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?
> +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?
> + 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
