[U-Boot] [PATCH 1/3] core: Add function to get device for ofnode

[U-Boot] [PATCH 1/3] core: Add function to get device for ofnode

[Mario Six]

It's sometimes useful to get the device associated with a given ofnode.
Implement a function to implement this lookup operation.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/core/ofnode.c | 15 +++++++++++++++
 include/dm/ofnode.h   |  8 ++++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
index 4e4532651f..ca002063b3 100644
--- a/drivers/core/ofnode.c
+++ b/drivers/core/ofnode.c
@@ -16,6 +16,21 @@
 #include <linux/err.h>
 #include <linux/ioport.h>
 
+struct udevice *ofnode_dev(ofnode node)
+{
+	struct uclass *uc;
+	struct udevice *dev;
+
+	list_for_each_entry(uc, &gd->uclass_root, sibling_node) {
+		list_for_each_entry(dev, &uc->dev_head, uclass_node) {
+			if (ofnode_equal(dev_ofnode(dev), node))
+				return dev;
+		}
+	}
+
+	return NULL;
+}
+
 int ofnode_read_u32(ofnode node, const char *propname, u32 *outp)
 {
 	assert(ofnode_valid(node));
diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
index 0d008404f9..aec205eb80 100644
--- a/include/dm/ofnode.h
+++ b/include/dm/ofnode.h
@@ -193,6 +193,14 @@ static inline ofnode ofnode_null(void)
 	return node;
 }
 
+/**
+ * ofnode_dev() - Get the device associated with a given ofnode
+ *
+ * @node:	valid node reference to get the corresponding device for
+ * @return a pointer to the udevice if OK, NULL on error
+ */
+struct udevice *ofnode_dev(ofnode node);
+
 /**
  * ofnode_read_u32() - Read a 32-bit integer from a property
  *
-- 
2.16.1

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

[Mario Six]

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

Signed-off-by: Mario Six <mario.six@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
+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));
+	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);
+
+/**
+ * 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);
+
+#endif
+
 #endif
-- 
2.16.1

[U-Boot] [PATCH 3/3] core: Add dev_{disable,enable}_by_path

[Mario Six]

We cannot use device structures to disable devices, since getting
them with the API functions would bind and activate the device, which
would fail if the underlying device does not exist.

Hence, add a function to disable devices by path in a live device tree.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/core/device.c | 36 ++++++++++++++++++++++++++++++++++++
 include/dm/device.h   | 20 ++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/core/device.c b/drivers/core/device.c
index 940a153c58..c627453bb9 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -724,3 +724,39 @@ bool of_machine_is_compatible(const char *compat)
 
 	return !fdt_node_check_compatible(fdt, 0, compat);
 }
+
+#ifdef CONFIG_OF_LIVE
+int dev_disable_by_path(const char *path)
+{
+	ofnode node = np_to_ofnode(of_find_node_by_path(path));
+	struct udevice *dev = ofnode_dev(node);
+	int res;
+
+	res = device_remove(dev, DM_REMOVE_NORMAL);
+	if (res)
+		return res;
+
+	res = device_unbind(dev);
+	if (res)
+		return res;
+
+	return ofnode_disable(node);
+}
+
+int dev_enable_by_path(const char *path)
+{
+	ofnode node = np_to_ofnode(of_find_node_by_path(path));
+	ofnode pnode = ofnode_get_parent(node);
+	struct udevice *parent = ofnode_dev(pnode);
+	int res;
+
+	if (!parent)
+		return -EINVAL;
+
+	res = ofnode_enable(node);
+	if (res)
+		return res;
+
+	return lists_bind_fdt(parent, node, NULL);
+}
+#endif /* CONFIG_OF_LIVE */
diff --git a/include/dm/device.h b/include/dm/device.h
index 7786b1cf4e..f55907966a 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -586,6 +586,26 @@ bool device_is_compatible(struct udevice *dev, const char *compat);
  */
 bool of_machine_is_compatible(const char *compat);
 
+#ifdef CONFIG_OF_LIVE
+
+/**
+ * dev_disable_by_path() - Disable a device given its device tree path
+ *
+ * @path:	The device tree path identifying the device to be disabled
+ * @return 0 on success, -ve on error
+ */
+int dev_disable_by_path(const char *path);
+
+/**
+ * dev_enable_by_path() - Enable a device given its device tree path
+ *
+ * @path:	The device tree path identifying the device to be enabled
+ * @return 0 on success, -ve on error
+ */
+int dev_enable_by_path(const char *path);
+
+#endif /* CONFIG_OF_LIVE */
+
 /**
  * device_is_on_pci_bus - Test if a device is on a PCI bus
  *
-- 
2.16.1

[U-Boot] [PATCH 1/3] core: Add function to get device for ofnode

[Simon Glass]

Hi Mario,

On 28 March 2018 at 20:37, Mario Six <mario.six@gdsys.cc> wrote:
> It's sometimes useful to get the device associated with a given ofnode.
> Implement a function to implement this lookup operation.

Where would you use this? Can you not use phandles to find the device?
Or uclass_get_device_by_ofnode() ?

>
> Signed-off-by: Mario Six <mario.six@gdsys.cc>
> ---
>  drivers/core/ofnode.c | 15 +++++++++++++++
>  include/dm/ofnode.h   |  8 ++++++++
>  2 files changed, 23 insertions(+)
>
> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
> index 4e4532651f..ca002063b3 100644
> --- a/drivers/core/ofnode.c
> +++ b/drivers/core/ofnode.c
> @@ -16,6 +16,21 @@
>  #include <linux/err.h>
>  #include <linux/ioport.h>
>
> +struct udevice *ofnode_dev(ofnode node)

Can you please add a test for this?

This seems like an internal function since it does not probe the
device. So how about putting it in device.h:

device_get_by_ofnode() - does probe the device it returns
device_find_by_ofnode() - doesn't probe

> +{
> +       struct uclass *uc;
> +       struct udevice *dev;
> +
> +       list_for_each_entry(uc, &gd->uclass_root, sibling_node) {
> +               list_for_each_entry(dev, &uc->dev_head, uclass_node) {
> +                       if (ofnode_equal(dev_ofnode(dev), node))
> +                               return dev;
> +               }
> +       }
> +
> +       return NULL;
> +}
> +
>  int ofnode_read_u32(ofnode node, const char *propname, u32 *outp)
>  {
>         assert(ofnode_valid(node));
> diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
> index 0d008404f9..aec205eb80 100644
> --- a/include/dm/ofnode.h
> +++ b/include/dm/ofnode.h
> @@ -193,6 +193,14 @@ static inline ofnode ofnode_null(void)
>         return node;
>  }
>
> +/**
> + * ofnode_dev() - Get the device associated with a given ofnode
> + *
> + * @node:      valid node reference to get the corresponding device for
> + * @return a pointer to the udevice if OK, NULL on error
> + */
> +struct udevice *ofnode_dev(ofnode node);
> +
>  /**
>   * ofnode_read_u32() - Read a 32-bit integer from a property
>   *
> --
> 2.16.1
>

Regards,
Simon

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

[Simon Glass]

Hi Mario,

On 28 March 2018 at 20:37, Mario Six <mario.six@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@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?

> +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] [PATCH 3/3] core: Add dev_{disable,enable}_by_path

[Simon Glass]

Hi Mario,

On 28 March 2018 at 20:37, Mario Six <mario.six@gdsys.cc> wrote:
> We cannot use device structures to disable devices, since getting
> them with the API functions would bind and activate the device, which
> would fail if the underlying device does not exist.
>
> Hence, add a function to disable devices by path in a live device tree.

What is the use case here? Is it to disable something after it has
already been bound / probed? Why can this not be done in the device
tree before U-Boot starts?

Also this needs a test.

>
> Signed-off-by: Mario Six <mario.six@gdsys.cc>
> ---
>  drivers/core/device.c | 36 ++++++++++++++++++++++++++++++++++++
>  include/dm/device.h   | 20 ++++++++++++++++++++
>  2 files changed, 56 insertions(+)
>
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index 940a153c58..c627453bb9 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -724,3 +724,39 @@ bool of_machine_is_compatible(const char *compat)
>
>         return !fdt_node_check_compatible(fdt, 0, compat);
>  }
> +
> +#ifdef CONFIG_OF_LIVE
> +int dev_disable_by_path(const char *path)
> +{
> +       ofnode node = np_to_ofnode(of_find_node_by_path(path));

Please see ofnode_path()

> +       struct udevice *dev = ofnode_dev(node);
> +       int res;
> +
> +       res = device_remove(dev, DM_REMOVE_NORMAL);
> +       if (res)
> +               return res;
> +
> +       res = device_unbind(dev);
> +       if (res)
> +               return res;
> +
> +       return ofnode_disable(node);
> +}
> +
> +int dev_enable_by_path(const char *path)
> +{
> +       ofnode node = np_to_ofnode(of_find_node_by_path(path));
> +       ofnode pnode = ofnode_get_parent(node);
> +       struct udevice *parent = ofnode_dev(pnode);
> +       int res;
> +
> +       if (!parent)
> +               return -EINVAL;
> +
> +       res = ofnode_enable(node);
> +       if (res)
> +               return res;
> +
> +       return lists_bind_fdt(parent, node, NULL);
> +}
> +#endif /* CONFIG_OF_LIVE */
> diff --git a/include/dm/device.h b/include/dm/device.h
> index 7786b1cf4e..f55907966a 100644
> --- a/include/dm/device.h
> +++ b/include/dm/device.h
> @@ -586,6 +586,26 @@ bool device_is_compatible(struct udevice *dev, const char *compat);
>   */
>  bool of_machine_is_compatible(const char *compat);
>
> +#ifdef CONFIG_OF_LIVE
> +
> +/**
> + * dev_disable_by_path() - Disable a device given its device tree path
> + *
> + * @path:      The device tree path identifying the device to be disabled
> + * @return 0 on success, -ve on error
> + */
> +int dev_disable_by_path(const char *path);
> +
> +/**
> + * dev_enable_by_path() - Enable a device given its device tree path
> + *
> + * @path:      The device tree path identifying the device to be enabled
> + * @return 0 on success, -ve on error
> + */
> +int dev_enable_by_path(const char *path);
> +
> +#endif /* CONFIG_OF_LIVE */
> +
>  /**
>   * device_is_on_pci_bus - Test if a device is on a PCI bus
>   *
> --
> 2.16.1
>

Regards,
Simon

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

[Mario Six]

Hi Simon,

On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Mario,
>
> On 28 March 2018 at 20:37, Mario Six <mario.six@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@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] [PATCH 1/3] core: Add function to get device for ofnode

[Mario Six]

Hi Simon,

On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Mario,
>
> On 28 March 2018 at 20:37, Mario Six <mario.six@gdsys.cc> wrote:
>> It's sometimes useful to get the device associated with a given ofnode.
>> Implement a function to implement this lookup operation.
>
> Where would you use this? Can you not use phandles to find the device?
> Or uclass_get_device_by_ofnode() ?
>

The function is used with the dev_{enable,disable}_by_path in the next patch:
If I used any of the uclass_* functions or similar, the device would be probed,
which is not what I want, since the device may not actually be physically
present.

>>
>> Signed-off-by: Mario Six <mario.six@gdsys.cc>
>> ---
>>  drivers/core/ofnode.c | 15 +++++++++++++++
>>  include/dm/ofnode.h   |  8 ++++++++
>>  2 files changed, 23 insertions(+)
>>
>> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
>> index 4e4532651f..ca002063b3 100644
>> --- a/drivers/core/ofnode.c
>> +++ b/drivers/core/ofnode.c
>> @@ -16,6 +16,21 @@
>>  #include <linux/err.h>
>>  #include <linux/ioport.h>
>>
>> +struct udevice *ofnode_dev(ofnode node)
>
> Can you please add a test for this?
>
> This seems like an internal function since it does not probe the
> device. So how about putting it in device.h:
>
> device_get_by_ofnode() - does probe the device it returns
> device_find_by_ofnode() - doesn't probe
>
>> +{
>> +       struct uclass *uc;
>> +       struct udevice *dev;
>> +
>> +       list_for_each_entry(uc, &gd->uclass_root, sibling_node) {
>> +               list_for_each_entry(dev, &uc->dev_head, uclass_node) {
>> +                       if (ofnode_equal(dev_ofnode(dev), node))
>> +                               return dev;
>> +               }
>> +       }
>> +
>> +       return NULL;
>> +}
>> +
>>  int ofnode_read_u32(ofnode node, const char *propname, u32 *outp)
>>  {
>>         assert(ofnode_valid(node));
>> diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
>> index 0d008404f9..aec205eb80 100644
>> --- a/include/dm/ofnode.h
>> +++ b/include/dm/ofnode.h
>> @@ -193,6 +193,14 @@ static inline ofnode ofnode_null(void)
>>         return node;
>>  }
>>
>> +/**
>> + * ofnode_dev() - Get the device associated with a given ofnode
>> + *
>> + * @node:      valid node reference to get the corresponding device for
>> + * @return a pointer to the udevice if OK, NULL on error
>> + */
>> +struct udevice *ofnode_dev(ofnode node);
>> +
>>  /**
>>   * ofnode_read_u32() - Read a 32-bit integer from a property
>>   *
>> --
>> 2.16.1
>>
>
> Regards,
> Simon
>

Everything else will be addressed in v2. Thanks for reviewing!

Best regards,

Mario

[U-Boot] [PATCH 3/3] core: Add dev_{disable,enable}_by_path

[Mario Six]

Hi Simon,

On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Mario,
>
> On 28 March 2018 at 20:37, Mario Six <mario.six@gdsys.cc> wrote:
>> We cannot use device structures to disable devices, since getting
>> them with the API functions would bind and activate the device, which
>> would fail if the underlying device does not exist.
>>
>> Hence, add a function to disable devices by path in a live device tree.
>
> What is the use case here? Is it to disable something after it has
> already been bound / probed? Why can this not be done in the device
> tree before U-Boot starts?
>

The devices that may not be in the tree are all disabled in the device tree.
Then, we detect which devices are actually there, and enable those that are.
That way we can use a single device tree for lots of actual boards, which are
all very minor variants of each other.

> Also this needs a test.
>
>>
>> Signed-off-by: Mario Six <mario.six@gdsys.cc>
>> ---
>>  drivers/core/device.c | 36 ++++++++++++++++++++++++++++++++++++
>>  include/dm/device.h   | 20 ++++++++++++++++++++
>>  2 files changed, 56 insertions(+)
>>
>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>> index 940a153c58..c627453bb9 100644
>> --- a/drivers/core/device.c
>> +++ b/drivers/core/device.c
>> @@ -724,3 +724,39 @@ bool of_machine_is_compatible(const char *compat)
>>
>>         return !fdt_node_check_compatible(fdt, 0, compat);
>>  }
>> +
>> +#ifdef CONFIG_OF_LIVE
>> +int dev_disable_by_path(const char *path)
>> +{
>> +       ofnode node = np_to_ofnode(of_find_node_by_path(path));
>
> Please see ofnode_path()
>
>> +       struct udevice *dev = ofnode_dev(node);
>> +       int res;
>> +
>> +       res = device_remove(dev, DM_REMOVE_NORMAL);
>> +       if (res)
>> +               return res;
>> +
>> +       res = device_unbind(dev);
>> +       if (res)
>> +               return res;
>> +
>> +       return ofnode_disable(node);
>> +}
>> +
>> +int dev_enable_by_path(const char *path)
>> +{
>> +       ofnode node = np_to_ofnode(of_find_node_by_path(path));
>> +       ofnode pnode = ofnode_get_parent(node);
>> +       struct udevice *parent = ofnode_dev(pnode);
>> +       int res;
>> +
>> +       if (!parent)
>> +               return -EINVAL;
>> +
>> +       res = ofnode_enable(node);
>> +       if (res)
>> +               return res;
>> +
>> +       return lists_bind_fdt(parent, node, NULL);
>> +}
>> +#endif /* CONFIG_OF_LIVE */
>> diff --git a/include/dm/device.h b/include/dm/device.h
>> index 7786b1cf4e..f55907966a 100644
>> --- a/include/dm/device.h
>> +++ b/include/dm/device.h
>> @@ -586,6 +586,26 @@ bool device_is_compatible(struct udevice *dev, const char *compat);
>>   */
>>  bool of_machine_is_compatible(const char *compat);
>>
>> +#ifdef CONFIG_OF_LIVE
>> +
>> +/**
>> + * dev_disable_by_path() - Disable a device given its device tree path
>> + *
>> + * @path:      The device tree path identifying the device to be disabled
>> + * @return 0 on success, -ve on error
>> + */
>> +int dev_disable_by_path(const char *path);
>> +
>> +/**
>> + * dev_enable_by_path() - Enable a device given its device tree path
>> + *
>> + * @path:      The device tree path identifying the device to be enabled
>> + * @return 0 on success, -ve on error
>> + */
>> +int dev_enable_by_path(const char *path);
>> +
>> +#endif /* CONFIG_OF_LIVE */
>> +
>>  /**
>>   * device_is_on_pci_bus - Test if a device is on a PCI bus
>>   *
>> --
>> 2.16.1
>>
>
> Regards,
> Simon

Everything else will be addressed in v2. Thanks for reviewing!

Best regards,

Mario

[U-Boot] [PATCH 1/3] core: Add function to get device for ofnode

[Simon Glass]

Hi Mario,

On 10 April 2018 at 05:34, Mario Six <mario.six@gdsys.cc> wrote:
> Hi Simon,
>
> On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Mario,
>>
>> On 28 March 2018 at 20:37, Mario Six <mario.six@gdsys.cc> wrote:
>>> It's sometimes useful to get the device associated with a given ofnode.
>>> Implement a function to implement this lookup operation.
>>
>> Where would you use this? Can you not use phandles to find the device?
>> Or uclass_get_device_by_ofnode() ?
>>
>
> The function is used with the dev_{enable,disable}_by_path in the next patch:
> If I used any of the uclass_* functions or similar, the device would be probed,
> which is not what I want, since the device may not actually be physically
> present.

So how about using uclass_find_device_by_ofnode() ?

Regards,
Simon

[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@gdsys.cc> wrote:
> Hi Simon,
>
> On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Mario,
>>
>> On 28 March 2018 at 20:37, Mario Six <mario.six@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@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] [PATCH 1/3] core: Add function to get device for ofnode

[Mario Six]

Hi Simon,

On Thu, Apr 12, 2018 at 6:42 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Mario,
>
> On 10 April 2018 at 05:34, Mario Six <mario.six@gdsys.cc> wrote:
>> Hi Simon,
>>
>> On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Mario,
>>>
>>> On 28 March 2018 at 20:37, Mario Six <mario.six@gdsys.cc> wrote:
>>>> It's sometimes useful to get the device associated with a given ofnode.
>>>> Implement a function to implement this lookup operation.
>>>
>>> Where would you use this? Can you not use phandles to find the device?
>>> Or uclass_get_device_by_ofnode() ?
>>>
>>
>> The function is used with the dev_{enable,disable}_by_path in the next patch:
>> If I used any of the uclass_* functions or similar, the device would be probed,
>> which is not what I want, since the device may not actually be physically
>> present.
>
> So how about using uclass_find_device_by_ofnode() ?
>

That would work for the disabling, true, but not for the enabling (which is
what is used in the upcoming board): Since the node is declared as disabled in
the DT, the device is not even bound (so uclass_find_device_by_ofnode) won't
return it.

A more elegant solution would be to have device_probe check again if the
underlying ofnode is disabled, and stop the probing if that's the case. In this
scenario the disabled devices would still be displayed in the tree, but never
probed, which is probably OK (I don't know if there would be any side effects
with iterating through devices, for example). But changing the behavior of such
elementary API functions is probably not a good idea.

> Regards,
> Simon

Best regards,
Mario

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

[Mario Six]

Hi Simon,

On Thu, Apr 12, 2018 at 6:42 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Mario,
>
> On 10 April 2018 at 05:23, Mario Six <mario.six@gdsys.cc> wrote:
>> Hi Simon,
>>
>> On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Mario,
>>>
>>> On 28 March 2018 at 20:37, Mario Six <mario.six@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@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] [PATCH 1/3] core: Add function to get device for ofnode

[Simon Glass]

Hi Mario,

On 18 April 2018 at 02:20, Mario Six <mario.six@gdsys.cc> wrote:
> Hi Simon,
>
> On Thu, Apr 12, 2018 at 6:42 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Mario,
>>
>> On 10 April 2018 at 05:34, Mario Six <mario.six@gdsys.cc> wrote:
>>> Hi Simon,
>>>
>>> On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass <sjg@chromium.org> wrote:
>>>> Hi Mario,
>>>>
>>>> On 28 March 2018 at 20:37, Mario Six <mario.six@gdsys.cc> wrote:
>>>>> It's sometimes useful to get the device associated with a given
ofnode.
>>>>> Implement a function to implement this lookup operation.
>>>>
>>>> Where would you use this? Can you not use phandles to find the device?
>>>> Or uclass_get_device_by_ofnode() ?
>>>>
>>>
>>> The function is used with the dev_{enable,disable}_by_path in the next
patch:
>>> If I used any of the uclass_* functions or similar, the device would be
probed,
>>> which is not what I want, since the device may not actually be
physically
>>> present.
>>
>> So how about using uclass_find_device_by_ofnode() ?
>>
>
> That would work for the disabling, true, but not for the enabling (which
is
> what is used in the upcoming board): Since the node is declared as
disabled in
> the DT, the device is not even bound (so uclass_find_device_by_ofnode)
won't
> return it.
>
> A more elegant solution would be to have device_probe check again if the
> underlying ofnode is disabled, and stop the probing if that's the case.
In this
> scenario the disabled devices would still be displayed in the tree, but
never
> probed, which is probably OK (I don't know if there would be any side
effects
> with iterating through devices, for example). But changing the behavior
of such
> elementary API functions is probably not a good idea.

That seems to be a different topic.

Fundamentally I don't see the difference between
uclass_find_device_by_ofnode() and your ofnode_dev().

If you want to enable something after probing you will have to call
device_bind() or similar. If that is your intent, I think you need a
different function from ofnode_dev(), since it also relies on the device
already being bound.

Regards,
Simon

[U-Boot] [PATCH 1/3] core: Add function to get device for ofnode

[Mario Six]

Hi Simon,

On Sun, Apr 22, 2018 at 10:13 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Mario,
>
> On 18 April 2018 at 02:20, Mario Six <mario.six@gdsys.cc> wrote:
>> Hi Simon,
>>
>> On Thu, Apr 12, 2018 at 6:42 PM, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Mario,
>>>
>>> On 10 April 2018 at 05:34, Mario Six <mario.six@gdsys.cc> wrote:
>>>> Hi Simon,
>>>>
>>>> On Fri, Mar 30, 2018 at 12:43 AM, Simon Glass <sjg@chromium.org> wrote:
>>>>> Hi Mario,
>>>>>
>>>>> On 28 March 2018 at 20:37, Mario Six <mario.six@gdsys.cc> wrote:
>>>>>> It's sometimes useful to get the device associated with a given
> ofnode.
>>>>>> Implement a function to implement this lookup operation.
>>>>>
>>>>> Where would you use this? Can you not use phandles to find the device?
>>>>> Or uclass_get_device_by_ofnode() ?
>>>>>
>>>>
>>>> The function is used with the dev_{enable,disable}_by_path in the next
> patch:
>>>> If I used any of the uclass_* functions or similar, the device would be
> probed,
>>>> which is not what I want, since the device may not actually be
> physically
>>>> present.
>>>
>>> So how about using uclass_find_device_by_ofnode() ?
>>>
>>
>> That would work for the disabling, true, but not for the enabling (which
> is
>> what is used in the upcoming board): Since the node is declared as
> disabled in
>> the DT, the device is not even bound (so uclass_find_device_by_ofnode)
> won't
>> return it.
>>
>> A more elegant solution would be to have device_probe check again if the
>> underlying ofnode is disabled, and stop the probing if that's the case.
> In this
>> scenario the disabled devices would still be displayed in the tree, but
> never
>> probed, which is probably OK (I don't know if there would be any side
> effects
>> with iterating through devices, for example). But changing the behavior
> of such
>> elementary API functions is probably not a good idea.
>
> That seems to be a different topic.
>
> Fundamentally I don't see the difference between
> uclass_find_device_by_ofnode() and your ofnode_dev().
>
> If you want to enable something after probing you will have to call
> device_bind() or similar. If that is your intent, I think you need a
> different function from ofnode_dev(), since it also relies on the device
> already being bound.
>

Ah, yes, I forgot that the *find* functions don't probe, sorry about that.

Yes, right, it won't be a problem to use uclass_find_device_by_ofnode
instead then.

> Regards,
> Simon
>

Best regards,
Mario