[PATCH v2] software_node: Add support for fwnode_graph*() family of functions

[PATCH v2] software_node: Add support for fwnode_graph*() family of functions

[Daniel Scally]

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

This implements the remaining .graph_* callbacks in the
fwnode operations vector for the software nodes. That makes
the fwnode_graph*() functions available in the drivers also
when software nodes are used.

The implementation tries to mimic the "OF graph" as much as
possible, but there is no support for the "reg" device
property. The ports will need to have the index in their
name which starts with "port" (for example "port0", "port1",
...) and endpoints will use the index of the software node
that is given to them during creation. The port nodes can
also be grouped under a specially named "ports" subnode,
just like in DT, if necessary.

The remote-endpoints are reference properties under the
endpoint nodes that are named "remote-endpoint". 

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Co-developed-by: Daniel Scally <djrscally@gmail.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
changes in v2:
	- added software_node_device_is_available
	- altered software_node_get_next_child to get references
	- altered software_node_get_next_endpoint to release references
	to ports and avoid passing invalid combinations of swnodes to
	software_node_get_next_child
	- altered swnode_graph_find_next_port to release port rather than
	old
	
 drivers/base/swnode.c | 129 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 127 insertions(+), 2 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 010828fc785b..d69034b807e3 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -363,6 +363,11 @@ static void software_node_put(struct fwnode_handle *fwnode)
 	kobject_put(&swnode->kobj);
 }
 
+static bool software_node_device_is_available(const struct fwnode_handle *fwnode)
+{
+	return is_software_node(fwnode);
+}
+
 static bool software_node_property_present(const struct fwnode_handle *fwnode,
 					   const char *propname)
 {
@@ -450,7 +455,7 @@ software_node_get_next_child(const struct fwnode_handle *fwnode,
 		c = list_next_entry(c, entry);
 	else
 		c = list_first_entry(&p->children, struct swnode, entry);
-	return &c->fwnode;
+	return software_node_get(&c->fwnode);
 }
 
 static struct fwnode_handle *
@@ -536,9 +541,125 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 	return 0;
 }
 
+static struct fwnode_handle *
+swnode_graph_find_next_port(const struct fwnode_handle *parent,
+			    struct fwnode_handle *port)
+{
+	struct fwnode_handle *old = port;
+
+	while ((port = software_node_get_next_child(parent, old))) {
+		if (!strncmp(to_swnode(port)->node->name, "port", 4))
+			return port;
+		fwnode_handle_put(port);
+		old = port;
+	}
+
+	return NULL;
+}
+
+static struct fwnode_handle *
+software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
+				      struct fwnode_handle *endpoint)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	struct fwnode_handle *old = endpoint;
+	struct fwnode_handle *parent_of_old;
+	struct fwnode_handle *parent;
+	struct fwnode_handle *port;
+
+	if (!swnode)
+		return NULL;
+
+	if (endpoint) {
+		port = software_node_get_parent(endpoint);
+		parent = software_node_get_parent(port);
+	} else {
+		parent = software_node_get_named_child_node(fwnode, "ports");
+		if (!parent)
+			parent = software_node_get(&swnode->fwnode);
+
+		port = swnode_graph_find_next_port(parent, NULL);
+	}
+
+	for (; port; port = swnode_graph_find_next_port(parent, port)) {
+
+		if (old) {
+			parent_of_old = software_node_get_parent(old);
+
+			if (parent_of_old != port)
+				old = NULL;
+
+			fwnode_handle_put(parent_of_old);
+		}
+
+		endpoint = software_node_get_next_child(port, old);
+		fwnode_handle_put(old);
+		if (endpoint)
+			break;
+
+		fwnode_handle_put(port);
+	}
+
+	fwnode_handle_put(port);
+	software_node_put(parent);
+
+	return endpoint;
+}
+
+static struct fwnode_handle *
+software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	const struct software_node_ref_args *ref;
+	const struct property_entry *prop;
+
+	if (!swnode)
+		return NULL;
+
+	prop = property_entry_get(swnode->node->properties, "remote-endpoint");
+	if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
+		return NULL;
+
+	ref = prop->pointer;
+
+	return software_node_get(software_node_fwnode(ref[0].node));
+}
+
+static struct fwnode_handle *
+software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	struct fwnode_handle *parent;
+
+	if (!strcmp(swnode->parent->node->name, "ports"))
+		parent = &swnode->parent->parent->fwnode;
+	else
+		parent = &swnode->parent->fwnode;
+
+	return software_node_get(parent);
+}
+
+static int
+software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
+				   struct fwnode_endpoint *endpoint)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	int ret;
+
+	ret = kstrtou32(swnode->parent->node->name + 4, 10, &endpoint->port);
+	if (ret)
+		return ret;
+
+	endpoint->id = swnode->id;
+	endpoint->local_fwnode = fwnode;
+
+	return 0;
+}
+
 static const struct fwnode_operations software_node_ops = {
 	.get = software_node_get,
 	.put = software_node_put,
+	.device_is_available = software_node_device_is_available,
 	.property_present = software_node_property_present,
 	.property_read_int_array = software_node_read_int_array,
 	.property_read_string_array = software_node_read_string_array,
@@ -547,7 +668,11 @@ static const struct fwnode_operations software_node_ops = {
 	.get_parent = software_node_get_parent,
 	.get_next_child_node = software_node_get_next_child,
 	.get_named_child_node = software_node_get_named_child_node,
-	.get_reference_args = software_node_get_reference_args
+	.get_reference_args = software_node_get_reference_args,
+	.graph_get_next_endpoint = software_node_graph_get_next_endpoint,
+	.graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
+	.graph_get_port_parent = software_node_graph_get_port_parent,
+	.graph_parse_endpoint = software_node_graph_parse_endpoint,
 };
 
 /* -------------------------------------------------------------------------- */
-- 
2.17.1

Re: [PATCH v2] software_node: Add support for fwnode_graph*() family of functions

[Sakari Ailus]

Moi Daniel and Heikki,

On Wed, Sep 16, 2020 at 12:28:27AM +0100, Daniel Scally wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> This implements the remaining .graph_* callbacks in the
> fwnode operations vector for the software nodes. That makes
> the fwnode_graph*() functions available in the drivers also
> when software nodes are used.
> 
> The implementation tries to mimic the "OF graph" as much as
> possible, but there is no support for the "reg" device
> property. The ports will need to have the index in their
> name which starts with "port" (for example "port0", "port1",
> ...) and endpoints will use the index of the software node
> that is given to them during creation. The port nodes can
> also be grouped under a specially named "ports" subnode,
> just like in DT, if necessary.
> 
> The remote-endpoints are reference properties under the
> endpoint nodes that are named "remote-endpoint". 
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Co-developed-by: Daniel Scally <djrscally@gmail.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> changes in v2:
> 	- added software_node_device_is_available
> 	- altered software_node_get_next_child to get references
> 	- altered software_node_get_next_endpoint to release references
> 	to ports and avoid passing invalid combinations of swnodes to
> 	software_node_get_next_child
> 	- altered swnode_graph_find_next_port to release port rather than
> 	old
> 	
>  drivers/base/swnode.c | 129 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 127 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 010828fc785b..d69034b807e3 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -363,6 +363,11 @@ static void software_node_put(struct fwnode_handle *fwnode)
>  	kobject_put(&swnode->kobj);
>  }
>  
> +static bool software_node_device_is_available(const struct fwnode_handle *fwnode)
> +{
> +	return is_software_node(fwnode);

This basically tells whether the device is there. Are there software node
based devices, i.e. do you need this?

If you do really need this, then I guess this could just return true for
now as if you somehow get here, the node is a software node anyway.

> +}
> +
>  static bool software_node_property_present(const struct fwnode_handle *fwnode,
>  					   const char *propname)
>  {
> @@ -450,7 +455,7 @@ software_node_get_next_child(const struct fwnode_handle *fwnode,
>  		c = list_next_entry(c, entry);
>  	else
>  		c = list_first_entry(&p->children, struct swnode, entry);
> -	return &c->fwnode;
> +	return software_node_get(&c->fwnode);

This looks like a bugfix that probably should or could be backported. Could
you make it a separate patch, with a Fixes: tag?

>  }
>  
>  static struct fwnode_handle *
> @@ -536,9 +541,125 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
>  	return 0;
>  }
>  
> +static struct fwnode_handle *
> +swnode_graph_find_next_port(const struct fwnode_handle *parent,
> +			    struct fwnode_handle *port)
> +{
> +	struct fwnode_handle *old = port;
> +
> +	while ((port = software_node_get_next_child(parent, old))) {
> +		if (!strncmp(to_swnode(port)->node->name, "port", 4))
> +			return port;
> +		fwnode_handle_put(port);
> +		old = port;
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
> +				      struct fwnode_handle *endpoint)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	struct fwnode_handle *old = endpoint;
> +	struct fwnode_handle *parent_of_old;
> +	struct fwnode_handle *parent;
> +	struct fwnode_handle *port;
> +
> +	if (!swnode)
> +		return NULL;
> +
> +	if (endpoint) {
> +		port = software_node_get_parent(endpoint);
> +		parent = software_node_get_parent(port);
> +	} else {
> +		parent = software_node_get_named_child_node(fwnode, "ports");
> +		if (!parent)
> +			parent = software_node_get(&swnode->fwnode);
> +
> +		port = swnode_graph_find_next_port(parent, NULL);
> +	}
> +
> +	for (; port; port = swnode_graph_find_next_port(parent, port)) {
> +
> +		if (old) {
> +			parent_of_old = software_node_get_parent(old);
> +
> +			if (parent_of_old != port)
> +				old = NULL;
> +
> +			fwnode_handle_put(parent_of_old);
> +		}
> +
> +		endpoint = software_node_get_next_child(port, old);
> +		fwnode_handle_put(old);
> +		if (endpoint)
> +			break;
> +
> +		fwnode_handle_put(port);
> +	}
> +
> +	fwnode_handle_put(port);
> +	software_node_put(parent);

This probably should be fwnode_handle_put() as well as this is basically
corresponding the device (i.e. likely not a software node).

> +
> +	return endpoint;
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	const struct software_node_ref_args *ref;
> +	const struct property_entry *prop;
> +
> +	if (!swnode)
> +		return NULL;
> +
> +	prop = property_entry_get(swnode->node->properties, "remote-endpoint");
> +	if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
> +		return NULL;
> +
> +	ref = prop->pointer;
> +
> +	return software_node_get(software_node_fwnode(ref[0].node));
> +}
> +
> +static struct fwnode_handle *
> +software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	struct fwnode_handle *parent;
> +
> +	if (!strcmp(swnode->parent->node->name, "ports"))
> +		parent = &swnode->parent->parent->fwnode;
> +	else
> +		parent = &swnode->parent->fwnode;
> +
> +	return software_node_get(parent);
> +}
> +
> +static int
> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
> +				   struct fwnode_endpoint *endpoint)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	int ret;
> +
> +	ret = kstrtou32(swnode->parent->node->name + 4, 10, &endpoint->port);
> +	if (ret)
> +		return ret;
> +
> +	endpoint->id = swnode->id;
> +	endpoint->local_fwnode = fwnode;
> +
> +	return 0;
> +}
> +
>  static const struct fwnode_operations software_node_ops = {
>  	.get = software_node_get,
>  	.put = software_node_put,
> +	.device_is_available = software_node_device_is_available,
>  	.property_present = software_node_property_present,
>  	.property_read_int_array = software_node_read_int_array,
>  	.property_read_string_array = software_node_read_string_array,
> @@ -547,7 +668,11 @@ static const struct fwnode_operations software_node_ops = {
>  	.get_parent = software_node_get_parent,
>  	.get_next_child_node = software_node_get_next_child,
>  	.get_named_child_node = software_node_get_named_child_node,
> -	.get_reference_args = software_node_get_reference_args
> +	.get_reference_args = software_node_get_reference_args,
> +	.graph_get_next_endpoint = software_node_graph_get_next_endpoint,
> +	.graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
> +	.graph_get_port_parent = software_node_graph_get_port_parent,
> +	.graph_parse_endpoint = software_node_graph_parse_endpoint,
>  };
>  
>  /* -------------------------------------------------------------------------- */

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2] software_node: Add support for fwnode_graph*() family of functions

[Dan Scally]

Hi Sakari - thanks for the comments

On 16/09/2020 10:17, Sakari Ailus wrote:
> Moi Daniel and Heikki,
>
> On Wed, Sep 16, 2020 at 12:28:27AM +0100, Daniel Scally wrote:
>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>
>> This implements the remaining .graph_* callbacks in the
>> fwnode operations vector for the software nodes. That makes
>> the fwnode_graph*() functions available in the drivers also
>> when software nodes are used.
>>
>> The implementation tries to mimic the "OF graph" as much as
>> possible, but there is no support for the "reg" device
>> property. The ports will need to have the index in their
>> name which starts with "port" (for example "port0", "port1",
>> ...) and endpoints will use the index of the software node
>> that is given to them during creation. The port nodes can
>> also be grouped under a specially named "ports" subnode,
>> just like in DT, if necessary.
>>
>> The remote-endpoints are reference properties under the
>> endpoint nodes that are named "remote-endpoint". 
>>
>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> Co-developed-by: Daniel Scally <djrscally@gmail.com>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>> changes in v2:
>> 	- added software_node_device_is_available
>> 	- altered software_node_get_next_child to get references
>> 	- altered software_node_get_next_endpoint to release references
>> 	to ports and avoid passing invalid combinations of swnodes to
>> 	software_node_get_next_child
>> 	- altered swnode_graph_find_next_port to release port rather than
>> 	old
>> 	
>>  drivers/base/swnode.c | 129 +++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 127 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
>> index 010828fc785b..d69034b807e3 100644
>> --- a/drivers/base/swnode.c
>> +++ b/drivers/base/swnode.c
>> @@ -363,6 +363,11 @@ static void software_node_put(struct fwnode_handle *fwnode)
>>  	kobject_put(&swnode->kobj);
>>  }
>>  
>> +static bool software_node_device_is_available(const struct fwnode_handle *fwnode)
>> +{
>> +	return is_software_node(fwnode);
> This basically tells whether the device is there. Are there software node
> based devices, i.e. do you need this?
>
> If you do really need this, then I guess this could just return true for
> now as if you somehow get here, the node is a software node anyway.

I do think its better to include it; I'm targeting using this with
ipu3-cio2; the cio2_parse_firmware() call there doesn't pass
FWNODE_GRAPH_DEVICE_DISABLED to fwnode_graph_get_endpoint_by_id() so
this function does need to exist to be found by that call (or else
cio2_parse_firmware() would need to pass that flag...but I don't know
the effect of doing that on devices that aren't using software nodes so
it didn't seem like a good idea). I can change it to return true though sure

>> +}
>> +
>>  static bool software_node_property_present(const struct fwnode_handle *fwnode,
>>  					   const char *propname)
>>  {
>> @@ -450,7 +455,7 @@ software_node_get_next_child(const struct fwnode_handle *fwnode,
>>  		c = list_next_entry(c, entry);
>>  	else
>>  		c = list_first_entry(&p->children, struct swnode, entry);
>> -	return &c->fwnode;
>> +	return software_node_get(&c->fwnode);
> This looks like a bugfix that probably should or could be backported. Could
> you make it a separate patch, with a Fixes: tag?
Yes, sure. That does change how some of the other code would need to
work though if this patch were applied but not the separated one. Sorry;
not sure what's the best way to proceed in that case. Should I just note
that this patch depends on the prior application of the separated one?
>
>>  }
>>  
>>  static struct fwnode_handle *
>> @@ -536,9 +541,125 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
>>  	return 0;
>>  }
>>  
>> +static struct fwnode_handle *
>> +swnode_graph_find_next_port(const struct fwnode_handle *parent,
>> +			    struct fwnode_handle *port)
>> +{
>> +	struct fwnode_handle *old = port;
>> +
>> +	while ((port = software_node_get_next_child(parent, old))) {
>> +		if (!strncmp(to_swnode(port)->node->name, "port", 4))
>> +			return port;
>> +		fwnode_handle_put(port);
>> +		old = port;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static struct fwnode_handle *
>> +software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
>> +				      struct fwnode_handle *endpoint)
>> +{
>> +	struct swnode *swnode = to_swnode(fwnode);
>> +	struct fwnode_handle *old = endpoint;
>> +	struct fwnode_handle *parent_of_old;
>> +	struct fwnode_handle *parent;
>> +	struct fwnode_handle *port;
>> +
>> +	if (!swnode)
>> +		return NULL;
>> +
>> +	if (endpoint) {
>> +		port = software_node_get_parent(endpoint);
>> +		parent = software_node_get_parent(port);
>> +	} else {
>> +		parent = software_node_get_named_child_node(fwnode, "ports");
>> +		if (!parent)
>> +			parent = software_node_get(&swnode->fwnode);
>> +
>> +		port = swnode_graph_find_next_port(parent, NULL);
>> +	}
>> +
>> +	for (; port; port = swnode_graph_find_next_port(parent, port)) {
>> +
>> +		if (old) {
>> +			parent_of_old = software_node_get_parent(old);
>> +
>> +			if (parent_of_old != port)
>> +				old = NULL;
>> +
>> +			fwnode_handle_put(parent_of_old);
>> +		}
>> +
>> +		endpoint = software_node_get_next_child(port, old);
>> +		fwnode_handle_put(old);
>> +		if (endpoint)
>> +			break;
>> +
>> +		fwnode_handle_put(port);
>> +	}
>> +
>> +	fwnode_handle_put(port);
>> +	software_node_put(parent);
> This probably should be fwnode_handle_put() as well as this is basically
> corresponding the device (i.e. likely not a software node).
Yep good point, fwnode_handle_put() it is.
>> +
>> +	return endpoint;
>> +}
>> +
>> +static struct fwnode_handle *
>> +software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
>> +{
>> +	struct swnode *swnode = to_swnode(fwnode);
>> +	const struct software_node_ref_args *ref;
>> +	const struct property_entry *prop;
>> +
>> +	if (!swnode)
>> +		return NULL;
>> +
>> +	prop = property_entry_get(swnode->node->properties, "remote-endpoint");
>> +	if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
>> +		return NULL;
>> +
>> +	ref = prop->pointer;
>> +
>> +	return software_node_get(software_node_fwnode(ref[0].node));
>> +}
>> +
>> +static struct fwnode_handle *
>> +software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
>> +{
>> +	struct swnode *swnode = to_swnode(fwnode);
>> +	struct fwnode_handle *parent;
>> +
>> +	if (!strcmp(swnode->parent->node->name, "ports"))
>> +		parent = &swnode->parent->parent->fwnode;
>> +	else
>> +		parent = &swnode->parent->fwnode;
>> +
>> +	return software_node_get(parent);
>> +}
>> +
>> +static int
>> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>> +				   struct fwnode_endpoint *endpoint)
>> +{
>> +	struct swnode *swnode = to_swnode(fwnode);
>> +	int ret;
>> +
>> +	ret = kstrtou32(swnode->parent->node->name + 4, 10, &endpoint->port);
>> +	if (ret)
>> +		return ret;
>> +
>> +	endpoint->id = swnode->id;
>> +	endpoint->local_fwnode = fwnode;
>> +
>> +	return 0;
>> +}
>> +
>>  static const struct fwnode_operations software_node_ops = {
>>  	.get = software_node_get,
>>  	.put = software_node_put,
>> +	.device_is_available = software_node_device_is_available,
>>  	.property_present = software_node_property_present,
>>  	.property_read_int_array = software_node_read_int_array,
>>  	.property_read_string_array = software_node_read_string_array,
>> @@ -547,7 +668,11 @@ static const struct fwnode_operations software_node_ops = {
>>  	.get_parent = software_node_get_parent,
>>  	.get_next_child_node = software_node_get_next_child,
>>  	.get_named_child_node = software_node_get_named_child_node,
>> -	.get_reference_args = software_node_get_reference_args
>> +	.get_reference_args = software_node_get_reference_args,
>> +	.graph_get_next_endpoint = software_node_graph_get_next_endpoint,
>> +	.graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
>> +	.graph_get_port_parent = software_node_graph_get_port_parent,
>> +	.graph_parse_endpoint = software_node_graph_parse_endpoint,
>>  };
>>  
>>  /* -------------------------------------------------------------------------- */

Re: [PATCH v2] software_node: Add support for fwnode_graph*() family of functions

[Andy Shevchenko]

On Wed, Sep 16, 2020 at 02:22:10PM +0100, Dan Scally wrote:
> On 16/09/2020 10:17, Sakari Ailus wrote:
> > On Wed, Sep 16, 2020 at 12:28:27AM +0100, Daniel Scally wrote:

...

> >> @@ -450,7 +455,7 @@ software_node_get_next_child(const struct fwnode_handle *fwnode,
> >>  		c = list_next_entry(c, entry);
> >>  	else
> >>  		c = list_first_entry(&p->children, struct swnode, entry);
> >> -	return &c->fwnode;
> >> +	return software_node_get(&c->fwnode);
> > This looks like a bugfix that probably should or could be backported. Could
> > you make it a separate patch, with a Fixes: tag?
> Yes, sure. That does change how some of the other code would need to
> work though if this patch were applied but not the separated one. Sorry;
> not sure what's the best way to proceed in that case. Should I just note
> that this patch depends on the prior application of the separated one?

It's easy to achieve. You may create a series of two, where the second one
dependant on the first one and first one has a Fixes tag and subject to
backport. I guess that's what Sakari meant.

> >>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] software_node: Add support for fwnode_graph*() family of functions

[Kieran Bingham]

Hi Dan,

On 16/09/2020 14:22, Dan Scally wrote:
> Hi Sakari - thanks for the comments
> 
> On 16/09/2020 10:17, Sakari Ailus wrote:
>> Moi Daniel and Heikki,
>>
>> On Wed, Sep 16, 2020 at 12:28:27AM +0100, Daniel Scally wrote:
>>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>
>>> This implements the remaining .graph_* callbacks in the
>>> fwnode operations vector for the software nodes. That makes
>>> the fwnode_graph*() functions available in the drivers also
>>> when software nodes are used.
>>>
>>> The implementation tries to mimic the "OF graph" as much as
>>> possible, but there is no support for the "reg" device
>>> property. The ports will need to have the index in their
>>> name which starts with "port" (for example "port0", "port1",
>>> ...) and endpoints will use the index of the software node
>>> that is given to them during creation. The port nodes can
>>> also be grouped under a specially named "ports" subnode,
>>> just like in DT, if necessary.
>>>
>>> The remote-endpoints are reference properties under the
>>> endpoint nodes that are named "remote-endpoint". 
>>>
>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>> Co-developed-by: Daniel Scally <djrscally@gmail.com>
>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>> ---
>>> changes in v2:
>>> 	- added software_node_device_is_available
>>> 	- altered software_node_get_next_child to get references
>>> 	- altered software_node_get_next_endpoint to release references
>>> 	to ports and avoid passing invalid combinations of swnodes to
>>> 	software_node_get_next_child
>>> 	- altered swnode_graph_find_next_port to release port rather than
>>> 	old
>>> 	
>>>  drivers/base/swnode.c | 129 +++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 127 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
>>> index 010828fc785b..d69034b807e3 100644
>>> --- a/drivers/base/swnode.c
>>> +++ b/drivers/base/swnode.c
>>> @@ -363,6 +363,11 @@ static void software_node_put(struct fwnode_handle *fwnode)
>>>  	kobject_put(&swnode->kobj);
>>>  }
>>>  
>>> +static bool software_node_device_is_available(const struct fwnode_handle *fwnode)
>>> +{
>>> +	return is_software_node(fwnode);
>> This basically tells whether the device is there. Are there software node
>> based devices, i.e. do you need this?
>>
>> If you do really need this, then I guess this could just return true for
>> now as if you somehow get here, the node is a software node anyway.
> 
> I do think its better to include it; I'm targeting using this with
> ipu3-cio2; the cio2_parse_firmware() call there doesn't pass
> FWNODE_GRAPH_DEVICE_DISABLED to fwnode_graph_get_endpoint_by_id() so
> this function does need to exist to be found by that call (or else
> cio2_parse_firmware() would need to pass that flag...but I don't know
> the effect of doing that on devices that aren't using software nodes so
> it didn't seem like a good idea). I can change it to return true though sure
> 
>>> +}
>>> +
>>>  static bool software_node_property_present(const struct fwnode_handle *fwnode,
>>>  					   const char *propname)
>>>  {
>>> @@ -450,7 +455,7 @@ software_node_get_next_child(const struct fwnode_handle *fwnode,
>>>  		c = list_next_entry(c, entry);
>>>  	else
>>>  		c = list_first_entry(&p->children, struct swnode, entry);
>>> -	return &c->fwnode;
>>> +	return software_node_get(&c->fwnode);
>> This looks like a bugfix that probably should or could be backported. Could
>> you make it a separate patch, with a Fixes: tag?
> Yes, sure. That does change how some of the other code would need to
> work though if this patch were applied but not the separated one. Sorry;
> not sure what's the best way to proceed in that case. Should I just note
> that this patch depends on the prior application of the separated one?

I think the assumption is that this individual change to
software_node_property_present() should be in a patch on it's own
preceeding 'this' one.

Running git-blame on drivers/base/swnode.c identifies this line as
previously being added by: 59abd83672f70, so you would add the tag:

Fixes: 59abd83672f7 ("drivers: base: Introducing software nodes to the
firmware node framework")

to the 'fixing' patch, and that can be backported accordingly.

When patches are sent in a series, the dependency becomes implicit.
You can do this by specifying a range when you do `git format-patch`

If you want to save off the last '2' patches, you can use a range
shorthand of '-2':

for example:

  git format-patch -2 -v3 --cover-letter -o patches/swnode

As it's a 'series' we add a cover letter to group them, and that gives a
location to add some free-form text as you wish too.

--
Kieran


>>
>>>  }
>>>  
>>>  static struct fwnode_handle *
>>> @@ -536,9 +541,125 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
>>>  	return 0;
>>>  }
>>>  
>>> +static struct fwnode_handle *
>>> +swnode_graph_find_next_port(const struct fwnode_handle *parent,
>>> +			    struct fwnode_handle *port)
>>> +{
>>> +	struct fwnode_handle *old = port;
>>> +
>>> +	while ((port = software_node_get_next_child(parent, old))) {
>>> +		if (!strncmp(to_swnode(port)->node->name, "port", 4))
>>> +			return port;
>>> +		fwnode_handle_put(port);
>>> +		old = port;
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>> +static struct fwnode_handle *
>>> +software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
>>> +				      struct fwnode_handle *endpoint)
>>> +{
>>> +	struct swnode *swnode = to_swnode(fwnode);
>>> +	struct fwnode_handle *old = endpoint;
>>> +	struct fwnode_handle *parent_of_old;
>>> +	struct fwnode_handle *parent;
>>> +	struct fwnode_handle *port;
>>> +
>>> +	if (!swnode)
>>> +		return NULL;
>>> +
>>> +	if (endpoint) {
>>> +		port = software_node_get_parent(endpoint);
>>> +		parent = software_node_get_parent(port);
>>> +	} else {
>>> +		parent = software_node_get_named_child_node(fwnode, "ports");
>>> +		if (!parent)
>>> +			parent = software_node_get(&swnode->fwnode);
>>> +
>>> +		port = swnode_graph_find_next_port(parent, NULL);
>>> +	}
>>> +
>>> +	for (; port; port = swnode_graph_find_next_port(parent, port)) {
>>> +
>>> +		if (old) {
>>> +			parent_of_old = software_node_get_parent(old);
>>> +
>>> +			if (parent_of_old != port)
>>> +				old = NULL;
>>> +
>>> +			fwnode_handle_put(parent_of_old);
>>> +		}
>>> +
>>> +		endpoint = software_node_get_next_child(port, old);
>>> +		fwnode_handle_put(old);
>>> +		if (endpoint)
>>> +			break;
>>> +
>>> +		fwnode_handle_put(port);
>>> +	}
>>> +
>>> +	fwnode_handle_put(port);
>>> +	software_node_put(parent);
>> This probably should be fwnode_handle_put() as well as this is basically
>> corresponding the device (i.e. likely not a software node).
> Yep good point, fwnode_handle_put() it is.
>>> +
>>> +	return endpoint;
>>> +}
>>> +
>>> +static struct fwnode_handle *
>>> +software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
>>> +{
>>> +	struct swnode *swnode = to_swnode(fwnode);
>>> +	const struct software_node_ref_args *ref;
>>> +	const struct property_entry *prop;
>>> +
>>> +	if (!swnode)
>>> +		return NULL;
>>> +
>>> +	prop = property_entry_get(swnode->node->properties, "remote-endpoint");
>>> +	if (!prop || prop->type != DEV_PROP_REF || prop->is_inline)
>>> +		return NULL;
>>> +
>>> +	ref = prop->pointer;
>>> +
>>> +	return software_node_get(software_node_fwnode(ref[0].node));
>>> +}
>>> +
>>> +static struct fwnode_handle *
>>> +software_node_graph_get_port_parent(struct fwnode_handle *fwnode)
>>> +{
>>> +	struct swnode *swnode = to_swnode(fwnode);
>>> +	struct fwnode_handle *parent;
>>> +
>>> +	if (!strcmp(swnode->parent->node->name, "ports"))
>>> +		parent = &swnode->parent->parent->fwnode;
>>> +	else
>>> +		parent = &swnode->parent->fwnode;
>>> +
>>> +	return software_node_get(parent);
>>> +}
>>> +
>>> +static int
>>> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>>> +				   struct fwnode_endpoint *endpoint)
>>> +{
>>> +	struct swnode *swnode = to_swnode(fwnode);
>>> +	int ret;
>>> +
>>> +	ret = kstrtou32(swnode->parent->node->name + 4, 10, &endpoint->port);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	endpoint->id = swnode->id;
>>> +	endpoint->local_fwnode = fwnode;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static const struct fwnode_operations software_node_ops = {
>>>  	.get = software_node_get,
>>>  	.put = software_node_put,
>>> +	.device_is_available = software_node_device_is_available,
>>>  	.property_present = software_node_property_present,
>>>  	.property_read_int_array = software_node_read_int_array,
>>>  	.property_read_string_array = software_node_read_string_array,
>>> @@ -547,7 +668,11 @@ static const struct fwnode_operations software_node_ops = {
>>>  	.get_parent = software_node_get_parent,
>>>  	.get_next_child_node = software_node_get_next_child,
>>>  	.get_named_child_node = software_node_get_named_child_node,
>>> -	.get_reference_args = software_node_get_reference_args
>>> +	.get_reference_args = software_node_get_reference_args,
>>> +	.graph_get_next_endpoint = software_node_graph_get_next_endpoint,
>>> +	.graph_get_remote_endpoint = software_node_graph_get_remote_endpoint,
>>> +	.graph_get_port_parent = software_node_graph_get_port_parent,
>>> +	.graph_parse_endpoint = software_node_graph_parse_endpoint,
>>>  };
>>>  
>>>  /* -------------------------------------------------------------------------- */

Re: [PATCH v2] software_node: Add support for fwnode_graph*() family of functions

[Andy Shevchenko]

On Wed, Sep 16, 2020 at 04:06:25PM +0100, Kieran Bingham wrote:
> On 16/09/2020 14:22, Dan Scally wrote:
> > On 16/09/2020 10:17, Sakari Ailus wrote:
> >> On Wed, Sep 16, 2020 at 12:28:27AM +0100, Daniel Scally wrote:

Thank you, Kieran, for detailed explanation, one small correction below though.

...

> >> This looks like a bugfix that probably should or could be backported. Could
> >> you make it a separate patch, with a Fixes: tag?
> > Yes, sure. That does change how some of the other code would need to
> > work though if this patch were applied but not the separated one. Sorry;
> > not sure what's the best way to proceed in that case. Should I just note
> > that this patch depends on the prior application of the separated one?
> 
> I think the assumption is that this individual change to
> software_node_property_present() should be in a patch on it's own
> preceeding 'this' one.
> 
> Running git-blame on drivers/base/swnode.c identifies this line as
> previously being added by: 59abd83672f70, so you would add the tag:

> Fixes: 59abd83672f7 ("drivers: base: Introducing software nodes to the
> firmware node framework")

Just to point out that this must be on one line.

> to the 'fixing' patch, and that can be backported accordingly.
> 
> When patches are sent in a series, the dependency becomes implicit.
> You can do this by specifying a range when you do `git format-patch`
> 
> If you want to save off the last '2' patches, you can use a range
> shorthand of '-2':
> 
> for example:
> 
>   git format-patch -2 -v3 --cover-letter -o patches/swnode
> 
> As it's a 'series' we add a cover letter to group them, and that gives a
> location to add some free-form text as you wish too.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] software_node: Add support for fwnode_graph*() family of functions

[Sakari Ailus]

Hi Dan,

On Wed, Sep 16, 2020 at 02:22:10PM +0100, Dan Scally wrote:
> Hi Sakari - thanks for the comments
> 
> On 16/09/2020 10:17, Sakari Ailus wrote:
> > Moi Daniel and Heikki,
> >
> > On Wed, Sep 16, 2020 at 12:28:27AM +0100, Daniel Scally wrote:
> >> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >>
> >> This implements the remaining .graph_* callbacks in the
> >> fwnode operations vector for the software nodes. That makes
> >> the fwnode_graph*() functions available in the drivers also
> >> when software nodes are used.
> >>
> >> The implementation tries to mimic the "OF graph" as much as
> >> possible, but there is no support for the "reg" device
> >> property. The ports will need to have the index in their
> >> name which starts with "port" (for example "port0", "port1",
> >> ...) and endpoints will use the index of the software node
> >> that is given to them during creation. The port nodes can
> >> also be grouped under a specially named "ports" subnode,
> >> just like in DT, if necessary.
> >>
> >> The remote-endpoints are reference properties under the
> >> endpoint nodes that are named "remote-endpoint". 
> >>
> >> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >> Co-developed-by: Daniel Scally <djrscally@gmail.com>
> >> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> >> ---
> >> changes in v2:
> >> 	- added software_node_device_is_available
> >> 	- altered software_node_get_next_child to get references
> >> 	- altered software_node_get_next_endpoint to release references
> >> 	to ports and avoid passing invalid combinations of swnodes to
> >> 	software_node_get_next_child
> >> 	- altered swnode_graph_find_next_port to release port rather than
> >> 	old
> >> 	
> >>  drivers/base/swnode.c | 129 +++++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 127 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> >> index 010828fc785b..d69034b807e3 100644
> >> --- a/drivers/base/swnode.c
> >> +++ b/drivers/base/swnode.c
> >> @@ -363,6 +363,11 @@ static void software_node_put(struct fwnode_handle *fwnode)
> >>  	kobject_put(&swnode->kobj);
> >>  }
> >>  
> >> +static bool software_node_device_is_available(const struct fwnode_handle *fwnode)
> >> +{
> >> +	return is_software_node(fwnode);
> > This basically tells whether the device is there. Are there software node
> > based devices, i.e. do you need this?
> >
> > If you do really need this, then I guess this could just return true for
> > now as if you somehow get here, the node is a software node anyway.
> 
> I do think its better to include it; I'm targeting using this with
> ipu3-cio2; the cio2_parse_firmware() call there doesn't pass
> FWNODE_GRAPH_DEVICE_DISABLED to fwnode_graph_get_endpoint_by_id() so

I wonder if this has something to do with replacing the device's fwnode
in the cio2-bridge patch.

It's the device that needs to be enabled, and it's not a software node.

-- 
Regards,

Sakari Ailus

Re: [PATCH v2] software_node: Add support for fwnode_graph*() family of functions

[Dan Scally]

Good morning

On 18/09/2020 07:22, Sakari Ailus wrote:
> Hi Dan,
>
> On Wed, Sep 16, 2020 at 02:22:10PM +0100, Dan Scally wrote:
>> Hi Sakari - thanks for the comments
>>
>> On 16/09/2020 10:17, Sakari Ailus wrote:
>>> Moi Daniel and Heikki,
>>>
>>> On Wed, Sep 16, 2020 at 12:28:27AM +0100, Daniel Scally wrote:
>>>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>>
>>>> This implements the remaining .graph_* callbacks in the
>>>> fwnode operations vector for the software nodes. That makes
>>>> the fwnode_graph*() functions available in the drivers also
>>>> when software nodes are used.
>>>>
>>>> The implementation tries to mimic the "OF graph" as much as
>>>> possible, but there is no support for the "reg" device
>>>> property. The ports will need to have the index in their
>>>> name which starts with "port" (for example "port0", "port1",
>>>> ...) and endpoints will use the index of the software node
>>>> that is given to them during creation. The port nodes can
>>>> also be grouped under a specially named "ports" subnode,
>>>> just like in DT, if necessary.
>>>>
>>>> The remote-endpoints are reference properties under the
>>>> endpoint nodes that are named "remote-endpoint". 
>>>>
>>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>> Co-developed-by: Daniel Scally <djrscally@gmail.com>
>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>>> ---
>>>> changes in v2:
>>>> 	- added software_node_device_is_available
>>>> 	- altered software_node_get_next_child to get references
>>>> 	- altered software_node_get_next_endpoint to release references
>>>> 	to ports and avoid passing invalid combinations of swnodes to
>>>> 	software_node_get_next_child
>>>> 	- altered swnode_graph_find_next_port to release port rather than
>>>> 	old
>>>> 	
>>>>  drivers/base/swnode.c | 129 +++++++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 127 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
>>>> index 010828fc785b..d69034b807e3 100644
>>>> --- a/drivers/base/swnode.c
>>>> +++ b/drivers/base/swnode.c
>>>> @@ -363,6 +363,11 @@ static void software_node_put(struct fwnode_handle *fwnode)
>>>>  	kobject_put(&swnode->kobj);
>>>>  }
>>>>  
>>>> +static bool software_node_device_is_available(const struct fwnode_handle *fwnode)
>>>> +{
>>>> +	return is_software_node(fwnode);
>>> This basically tells whether the device is there. Are there software node
>>> based devices, i.e. do you need this?
>>>
>>> If you do really need this, then I guess this could just return true for
>>> now as if you somehow get here, the node is a software node anyway.
>> I do think its better to include it; I'm targeting using this with
>> ipu3-cio2; the cio2_parse_firmware() call there doesn't pass
>> FWNODE_GRAPH_DEVICE_DISABLED to fwnode_graph_get_endpoint_by_id() so
> I wonder if this has something to do with replacing the device's fwnode
> in the cio2-bridge patch.
>
> It's the device that needs to be enabled, and it's not a software node.
>
I think it is because of that yes, but I don't see a way around it at
the moment - unless there's a way to attach the software_node port and
endpoints that cio2-bridge creates to the device's existing firmware
instead.

Re: [PATCH v2] software_node: Add support for fwnode_graph*() family of functions

[Sakari Ailus]

On Fri, Sep 18, 2020 at 07:49:31AM +0100, Dan Scally wrote:
> Good morning
> 
> On 18/09/2020 07:22, Sakari Ailus wrote:
> > Hi Dan,
> >
> > On Wed, Sep 16, 2020 at 02:22:10PM +0100, Dan Scally wrote:
> >> Hi Sakari - thanks for the comments
> >>
> >> On 16/09/2020 10:17, Sakari Ailus wrote:
> >>> Moi Daniel and Heikki,
> >>>
> >>> On Wed, Sep 16, 2020 at 12:28:27AM +0100, Daniel Scally wrote:
> >>>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >>>>
> >>>> This implements the remaining .graph_* callbacks in the
> >>>> fwnode operations vector for the software nodes. That makes
> >>>> the fwnode_graph*() functions available in the drivers also
> >>>> when software nodes are used.
> >>>>
> >>>> The implementation tries to mimic the "OF graph" as much as
> >>>> possible, but there is no support for the "reg" device
> >>>> property. The ports will need to have the index in their
> >>>> name which starts with "port" (for example "port0", "port1",
> >>>> ...) and endpoints will use the index of the software node
> >>>> that is given to them during creation. The port nodes can
> >>>> also be grouped under a specially named "ports" subnode,
> >>>> just like in DT, if necessary.
> >>>>
> >>>> The remote-endpoints are reference properties under the
> >>>> endpoint nodes that are named "remote-endpoint". 
> >>>>
> >>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >>>> Co-developed-by: Daniel Scally <djrscally@gmail.com>
> >>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> >>>> ---
> >>>> changes in v2:
> >>>> 	- added software_node_device_is_available
> >>>> 	- altered software_node_get_next_child to get references
> >>>> 	- altered software_node_get_next_endpoint to release references
> >>>> 	to ports and avoid passing invalid combinations of swnodes to
> >>>> 	software_node_get_next_child
> >>>> 	- altered swnode_graph_find_next_port to release port rather than
> >>>> 	old
> >>>> 	
> >>>>  drivers/base/swnode.c | 129 +++++++++++++++++++++++++++++++++++++++++-
> >>>>  1 file changed, 127 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> >>>> index 010828fc785b..d69034b807e3 100644
> >>>> --- a/drivers/base/swnode.c
> >>>> +++ b/drivers/base/swnode.c
> >>>> @@ -363,6 +363,11 @@ static void software_node_put(struct fwnode_handle *fwnode)
> >>>>  	kobject_put(&swnode->kobj);
> >>>>  }
> >>>>  
> >>>> +static bool software_node_device_is_available(const struct fwnode_handle *fwnode)
> >>>> +{
> >>>> +	return is_software_node(fwnode);
> >>> This basically tells whether the device is there. Are there software node
> >>> based devices, i.e. do you need this?
> >>>
> >>> If you do really need this, then I guess this could just return true for
> >>> now as if you somehow get here, the node is a software node anyway.
> >> I do think its better to include it; I'm targeting using this with
> >> ipu3-cio2; the cio2_parse_firmware() call there doesn't pass
> >> FWNODE_GRAPH_DEVICE_DISABLED to fwnode_graph_get_endpoint_by_id() so
> > I wonder if this has something to do with replacing the device's fwnode
> > in the cio2-bridge patch.
> >
> > It's the device that needs to be enabled, and it's not a software node.
> >
> I think it is because of that yes, but I don't see a way around it at
> the moment - unless there's a way to attach the software_node port and
> endpoints that cio2-bridge creates to the device's existing firmware
> instead.

I thought this was how it was meant to be used?

The secondary field is there for this purpose. But it may be not all fwnode
interface functions operate on fwnode->secondary?

-- 
Regards,

Sakari Ailus

Re: [PATCH v2] software_node: Add support for fwnode_graph*() family of functions

[Dan Scally]

On 18/09/2020 08:34, Sakari Ailus wrote:
> On Fri, Sep 18, 2020 at 07:49:31AM +0100, Dan Scally wrote:
>> Good morning
>>
>> On 18/09/2020 07:22, Sakari Ailus wrote:
>>> Hi Dan,
>>>
>>> On Wed, Sep 16, 2020 at 02:22:10PM +0100, Dan Scally wrote:
>>>> Hi Sakari - thanks for the comments
>>>>
>>>> On 16/09/2020 10:17, Sakari Ailus wrote:
>>>>> Moi Daniel and Heikki,
>>>>>
>>>>> On Wed, Sep 16, 2020 at 12:28:27AM +0100, Daniel Scally wrote:
>>>>>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>>>>
>>>>>> This implements the remaining .graph_* callbacks in the
>>>>>> fwnode operations vector for the software nodes. That makes
>>>>>> the fwnode_graph*() functions available in the drivers also
>>>>>> when software nodes are used.
>>>>>>
>>>>>> The implementation tries to mimic the "OF graph" as much as
>>>>>> possible, but there is no support for the "reg" device
>>>>>> property. The ports will need to have the index in their
>>>>>> name which starts with "port" (for example "port0", "port1",
>>>>>> ...) and endpoints will use the index of the software node
>>>>>> that is given to them during creation. The port nodes can
>>>>>> also be grouped under a specially named "ports" subnode,
>>>>>> just like in DT, if necessary.
>>>>>>
>>>>>> The remote-endpoints are reference properties under the
>>>>>> endpoint nodes that are named "remote-endpoint". 
>>>>>>
>>>>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>>>> Co-developed-by: Daniel Scally <djrscally@gmail.com>
>>>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>>>>> ---
>>>>>> changes in v2:
>>>>>> 	- added software_node_device_is_available
>>>>>> 	- altered software_node_get_next_child to get references
>>>>>> 	- altered software_node_get_next_endpoint to release references
>>>>>> 	to ports and avoid passing invalid combinations of swnodes to
>>>>>> 	software_node_get_next_child
>>>>>> 	- altered swnode_graph_find_next_port to release port rather than
>>>>>> 	old
>>>>>> 	
>>>>>>  drivers/base/swnode.c | 129 +++++++++++++++++++++++++++++++++++++++++-
>>>>>>  1 file changed, 127 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
>>>>>> index 010828fc785b..d69034b807e3 100644
>>>>>> --- a/drivers/base/swnode.c
>>>>>> +++ b/drivers/base/swnode.c
>>>>>> @@ -363,6 +363,11 @@ static void software_node_put(struct fwnode_handle *fwnode)
>>>>>>  	kobject_put(&swnode->kobj);
>>>>>>  }
>>>>>>  
>>>>>> +static bool software_node_device_is_available(const struct fwnode_handle *fwnode)
>>>>>> +{
>>>>>> +	return is_software_node(fwnode);
>>>>> This basically tells whether the device is there. Are there software node
>>>>> based devices, i.e. do you need this?
>>>>>
>>>>> If you do really need this, then I guess this could just return true for
>>>>> now as if you somehow get here, the node is a software node anyway.
>>>> I do think its better to include it; I'm targeting using this with
>>>> ipu3-cio2; the cio2_parse_firmware() call there doesn't pass
>>>> FWNODE_GRAPH_DEVICE_DISABLED to fwnode_graph_get_endpoint_by_id() so
>>> I wonder if this has something to do with replacing the device's fwnode
>>> in the cio2-bridge patch.
>>>
>>> It's the device that needs to be enabled, and it's not a software node.
>>>
>> I think it is because of that yes, but I don't see a way around it at
>> the moment - unless there's a way to attach the software_node port and
>> endpoints that cio2-bridge creates to the device's existing firmware
>> instead.
> I thought this was how it was meant to be used?
>
> The secondary field is there for this purpose. But it may be not all fwnode
> interface functions operate on fwnode->secondary?
Let me test it; it might just require some changes to
software_node_graph_get_port_parent() to check if the parent fwnode is a
secondary, and if it is to return the primary instead.

Re: [PATCH v2] software_node: Add support for fwnode_graph*() family of functions

[Sakari Ailus]

On Fri, Sep 18, 2020 at 08:46:52AM +0100, Dan Scally wrote:
> On 18/09/2020 08:34, Sakari Ailus wrote:
> > On Fri, Sep 18, 2020 at 07:49:31AM +0100, Dan Scally wrote:
> >> Good morning
> >>
> >> On 18/09/2020 07:22, Sakari Ailus wrote:
> >>> Hi Dan,
> >>>
> >>> On Wed, Sep 16, 2020 at 02:22:10PM +0100, Dan Scally wrote:
> >>>> Hi Sakari - thanks for the comments
> >>>>
> >>>> On 16/09/2020 10:17, Sakari Ailus wrote:
> >>>>> Moi Daniel and Heikki,
> >>>>>
> >>>>> On Wed, Sep 16, 2020 at 12:28:27AM +0100, Daniel Scally wrote:
> >>>>>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >>>>>>
> >>>>>> This implements the remaining .graph_* callbacks in the
> >>>>>> fwnode operations vector for the software nodes. That makes
> >>>>>> the fwnode_graph*() functions available in the drivers also
> >>>>>> when software nodes are used.
> >>>>>>
> >>>>>> The implementation tries to mimic the "OF graph" as much as
> >>>>>> possible, but there is no support for the "reg" device
> >>>>>> property. The ports will need to have the index in their
> >>>>>> name which starts with "port" (for example "port0", "port1",
> >>>>>> ...) and endpoints will use the index of the software node
> >>>>>> that is given to them during creation. The port nodes can
> >>>>>> also be grouped under a specially named "ports" subnode,
> >>>>>> just like in DT, if necessary.
> >>>>>>
> >>>>>> The remote-endpoints are reference properties under the
> >>>>>> endpoint nodes that are named "remote-endpoint". 
> >>>>>>
> >>>>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >>>>>> Co-developed-by: Daniel Scally <djrscally@gmail.com>
> >>>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> >>>>>> ---
> >>>>>> changes in v2:
> >>>>>> 	- added software_node_device_is_available
> >>>>>> 	- altered software_node_get_next_child to get references
> >>>>>> 	- altered software_node_get_next_endpoint to release references
> >>>>>> 	to ports and avoid passing invalid combinations of swnodes to
> >>>>>> 	software_node_get_next_child
> >>>>>> 	- altered swnode_graph_find_next_port to release port rather than
> >>>>>> 	old
> >>>>>> 	
> >>>>>>  drivers/base/swnode.c | 129 +++++++++++++++++++++++++++++++++++++++++-
> >>>>>>  1 file changed, 127 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> >>>>>> index 010828fc785b..d69034b807e3 100644
> >>>>>> --- a/drivers/base/swnode.c
> >>>>>> +++ b/drivers/base/swnode.c
> >>>>>> @@ -363,6 +363,11 @@ static void software_node_put(struct fwnode_handle *fwnode)
> >>>>>>  	kobject_put(&swnode->kobj);
> >>>>>>  }
> >>>>>>  
> >>>>>> +static bool software_node_device_is_available(const struct fwnode_handle *fwnode)
> >>>>>> +{
> >>>>>> +	return is_software_node(fwnode);
> >>>>> This basically tells whether the device is there. Are there software node
> >>>>> based devices, i.e. do you need this?
> >>>>>
> >>>>> If you do really need this, then I guess this could just return true for
> >>>>> now as if you somehow get here, the node is a software node anyway.
> >>>> I do think its better to include it; I'm targeting using this with
> >>>> ipu3-cio2; the cio2_parse_firmware() call there doesn't pass
> >>>> FWNODE_GRAPH_DEVICE_DISABLED to fwnode_graph_get_endpoint_by_id() so
> >>> I wonder if this has something to do with replacing the device's fwnode
> >>> in the cio2-bridge patch.
> >>>
> >>> It's the device that needs to be enabled, and it's not a software node.
> >>>
> >> I think it is because of that yes, but I don't see a way around it at
> >> the moment - unless there's a way to attach the software_node port and
> >> endpoints that cio2-bridge creates to the device's existing firmware
> >> instead.
> > I thought this was how it was meant to be used?
> >
> > The secondary field is there for this purpose. But it may be not all fwnode
> > interface functions operate on fwnode->secondary?
> Let me test it; it might just require some changes to
> software_node_graph_get_port_parent() to check if the parent fwnode is a
> secondary, and if it is to return the primary instead.

Ah, indeed. I forgot this part. I wonder if it'd cause issues to return the
primary if you've got the secondary swnode.

Heikki, any idea?

Code elsewhere (e.g. V4L2 fwnode framework + drivers) assume a device is
identified by a single fwnode, not two --- currently the swnode graph
function returning port parent returns the secondary so there's no match
with the primary fwnode.

-- 
Sakari Ailus

Re: [PATCH v2] software_node: Add support for fwnode_graph*() family of functions

[Dan Scally]

On 18/09/2020 09:57, Heikki Krogerus wrote:
> On Fri, Sep 18, 2020 at 10:57:41AM +0300, Sakari Ailus wrote:
>> On Fri, Sep 18, 2020 at 08:46:52AM +0100, Dan Scally wrote:
>>> On 18/09/2020 08:34, Sakari Ailus wrote:
>>>> On Fri, Sep 18, 2020 at 07:49:31AM +0100, Dan Scally wrote:
>>>>> Good morning
>>>>>
>>>>> On 18/09/2020 07:22, Sakari Ailus wrote:
>>>>>> I wonder if this has something to do with replacing the device's fwnode
>>>>>> in the cio2-bridge patch.
>>>>>>
>>>>>> It's the device that needs to be enabled, and it's not a software node.
>>>>>>
>>>>> I think it is because of that yes, but I don't see a way around it at
>>>>> the moment - unless there's a way to attach the software_node port and
>>>>> endpoints that cio2-bridge creates to the device's existing firmware
>>>>> instead.
>>>> I thought this was how it was meant to be used?
>>>>
>>>> The secondary field is there for this purpose. But it may be not all fwnode
>>>> interface functions operate on fwnode->secondary?
>>> Let me test it; it might just require some changes to
>>> software_node_graph_get_port_parent() to check if the parent fwnode is a
>>> secondary, and if it is to return the primary instead.
>> Ah, indeed. I forgot this part. I wonder if it'd cause issues to return the
>> primary if you've got the secondary swnode.
>>
>> Heikki, any idea?
>>
>> Code elsewhere (e.g. V4L2 fwnode framework + drivers) assume a device is
>> identified by a single fwnode, not two --- currently the swnode graph
>> function returning port parent returns the secondary so there's no match
>> with the primary fwnode.
> Sorry I don't think I understand the scenario here, but never return
> the primary node when the software node is the secondary from the
> software node API! The software node functions deal and return
> software nodes, and nothing else, just like ACPI deals with ACPI nodes
> only and DT deals with OF nodes only. We must never jump between the
> fwnode types at this level. That also means that if you want to
> describe the device graph with software nodes, then every node in the
> graph, starting from the port parents, must be a software node.
> Whether or not the node is secondary is irrelevant. But I guess this
> is not a problem here (or is it?).
>
> Considering the secondary node will unfortunately need to be done by
> the callers of fwnode API when the fwnode API can't take care of that.
>
Alright, so if we want to attach software nodes as secondaries to a
devices existing fwnode we'd need to modify things like
fwnode_graph_get_next_endpoint_by_id() [1] to consider whether the
returned node was a software_node secondary when they try to get the
device's node to run *is_available()


I did sort of wonder whether this was the right approach before, but
there's other comments [2] in the source that reassured me, for example
device_add_properties():

>  * WARNING: The callers should not use this function if it is known that there
>  * is no real firmware node associated with @dev! In that case the callers
>  * should create a software node and assign it to @dev directly.


[1]
https://elixir.bootlin.com/linux/latest/source/drivers/base/property.c#L1126

[2]
https://elixir.bootlin.com/linux/latest/source/drivers/base/property.c#L541

Re: [PATCH v2] software_node: Add support for fwnode_graph*() family of functions

[Sakari Ailus]

On Fri, Sep 18, 2020 at 11:57:09AM +0300, Heikki Krogerus wrote:
> On Fri, Sep 18, 2020 at 10:57:41AM +0300, Sakari Ailus wrote:
> > On Fri, Sep 18, 2020 at 08:46:52AM +0100, Dan Scally wrote:
> > > On 18/09/2020 08:34, Sakari Ailus wrote:
> > > > On Fri, Sep 18, 2020 at 07:49:31AM +0100, Dan Scally wrote:
> > > >> Good morning
> > > >>
> > > >> On 18/09/2020 07:22, Sakari Ailus wrote:
> > > >>> Hi Dan,
> > > >>>
> > > >>> On Wed, Sep 16, 2020 at 02:22:10PM +0100, Dan Scally wrote:
> > > >>>> Hi Sakari - thanks for the comments
> > > >>>>
> > > >>>> On 16/09/2020 10:17, Sakari Ailus wrote:
> > > >>>>> Moi Daniel and Heikki,
> > > >>>>>
> > > >>>>> On Wed, Sep 16, 2020 at 12:28:27AM +0100, Daniel Scally wrote:
> > > >>>>>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > >>>>>>
> > > >>>>>> This implements the remaining .graph_* callbacks in the
> > > >>>>>> fwnode operations vector for the software nodes. That makes
> > > >>>>>> the fwnode_graph*() functions available in the drivers also
> > > >>>>>> when software nodes are used.
> > > >>>>>>
> > > >>>>>> The implementation tries to mimic the "OF graph" as much as
> > > >>>>>> possible, but there is no support for the "reg" device
> > > >>>>>> property. The ports will need to have the index in their
> > > >>>>>> name which starts with "port" (for example "port0", "port1",
> > > >>>>>> ...) and endpoints will use the index of the software node
> > > >>>>>> that is given to them during creation. The port nodes can
> > > >>>>>> also be grouped under a specially named "ports" subnode,
> > > >>>>>> just like in DT, if necessary.
> > > >>>>>>
> > > >>>>>> The remote-endpoints are reference properties under the
> > > >>>>>> endpoint nodes that are named "remote-endpoint". 
> > > >>>>>>
> > > >>>>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > >>>>>> Co-developed-by: Daniel Scally <djrscally@gmail.com>
> > > >>>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> > > >>>>>> ---
> > > >>>>>> changes in v2:
> > > >>>>>> 	- added software_node_device_is_available
> > > >>>>>> 	- altered software_node_get_next_child to get references
> > > >>>>>> 	- altered software_node_get_next_endpoint to release references
> > > >>>>>> 	to ports and avoid passing invalid combinations of swnodes to
> > > >>>>>> 	software_node_get_next_child
> > > >>>>>> 	- altered swnode_graph_find_next_port to release port rather than
> > > >>>>>> 	old
> > > >>>>>> 	
> > > >>>>>>  drivers/base/swnode.c | 129 +++++++++++++++++++++++++++++++++++++++++-
> > > >>>>>>  1 file changed, 127 insertions(+), 2 deletions(-)
> > > >>>>>>
> > > >>>>>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> > > >>>>>> index 010828fc785b..d69034b807e3 100644
> > > >>>>>> --- a/drivers/base/swnode.c
> > > >>>>>> +++ b/drivers/base/swnode.c
> > > >>>>>> @@ -363,6 +363,11 @@ static void software_node_put(struct fwnode_handle *fwnode)
> > > >>>>>>  	kobject_put(&swnode->kobj);
> > > >>>>>>  }
> > > >>>>>>  
> > > >>>>>> +static bool software_node_device_is_available(const struct fwnode_handle *fwnode)
> > > >>>>>> +{
> > > >>>>>> +	return is_software_node(fwnode);
> > > >>>>> This basically tells whether the device is there. Are there software node
> > > >>>>> based devices, i.e. do you need this?
> > > >>>>>
> > > >>>>> If you do really need this, then I guess this could just return true for
> > > >>>>> now as if you somehow get here, the node is a software node anyway.
> > > >>>> I do think its better to include it; I'm targeting using this with
> > > >>>> ipu3-cio2; the cio2_parse_firmware() call there doesn't pass
> > > >>>> FWNODE_GRAPH_DEVICE_DISABLED to fwnode_graph_get_endpoint_by_id() so
> > > >>> I wonder if this has something to do with replacing the device's fwnode
> > > >>> in the cio2-bridge patch.
> > > >>>
> > > >>> It's the device that needs to be enabled, and it's not a software node.
> > > >>>
> > > >> I think it is because of that yes, but I don't see a way around it at
> > > >> the moment - unless there's a way to attach the software_node port and
> > > >> endpoints that cio2-bridge creates to the device's existing firmware
> > > >> instead.
> > > > I thought this was how it was meant to be used?
> > > >
> > > > The secondary field is there for this purpose. But it may be not all fwnode
> > > > interface functions operate on fwnode->secondary?
> > > Let me test it; it might just require some changes to
> > > software_node_graph_get_port_parent() to check if the parent fwnode is a
> > > secondary, and if it is to return the primary instead.
> > 
> > Ah, indeed. I forgot this part. I wonder if it'd cause issues to return the
> > primary if you've got the secondary swnode.
> > 
> > Heikki, any idea?
> > 
> > Code elsewhere (e.g. V4L2 fwnode framework + drivers) assume a device is
> > identified by a single fwnode, not two --- currently the swnode graph
> > function returning port parent returns the secondary so there's no match
> > with the primary fwnode.
> 
> Sorry I don't think I understand the scenario here, but never return
> the primary node when the software node is the secondary from the
> software node API! The software node functions deal and return
> software nodes, and nothing else, just like ACPI deals with ACPI nodes
> only and DT deals with OF nodes only. We must never jump between the
> fwnode types at this level. That also means that if you want to
> describe the device graph with software nodes, then every node in the
> graph, starting from the port parents, must be a software node.
> Whether or not the node is secondary is irrelevant. But I guess this
> is not a problem here (or is it?).

The way software nodes work (as in this patch) is not consistent with DT or
ACPI. For instance, the parent of the port node, returned by
software_node_graph_get_port_parent() is fwnode->secondary of the device,
not device's fwnode.

This is not expected by the users of the fwnode property API.

Also, it leads to drivers only seeing the software nodes while DT and ACPI
nodes as well as properties would be hidden.

> 
> Considering the secondary node will unfortunately need to be done by
> the callers of fwnode API when the fwnode API can't take care of that.

What problems would there be in returning the primary fwnode?

-- 
Regards,

Sakari Ailus

Re: [PATCH v2] software_node: Add support for fwnode_graph*() family of functions

[Dan Scally]

On 18/09/2020 10:15, Sakari Ailus wrote:
> On Fri, Sep 18, 2020 at 11:57:09AM +0300, Heikki Krogerus wrote:
>> On Fri, Sep 18, 2020 at 10:57:41AM +0300, Sakari Ailus wrote:
>>> On Fri, Sep 18, 2020 at 08:46:52AM +0100, Dan Scally wrote:
>>>> On 18/09/2020 08:34, Sakari Ailus wrote:
>>>>> On Fri, Sep 18, 2020 at 07:49:31AM +0100, Dan Scally wrote:
>>>>>> Good morning
>>>>>>
>>>>>> On 18/09/2020 07:22, Sakari Ailus wrote:
>>>>>>> Hi Dan,
>>>>>>>
>>>>>>> On Wed, Sep 16, 2020 at 02:22:10PM +0100, Dan Scally wrote:
>>>>>>>> Hi Sakari - thanks for the comments
>>>>>>>>
>>>>>>>> On 16/09/2020 10:17, Sakari Ailus wrote:
>>>>>>>>> Moi Daniel and Heikki,
>>>>>>>>>
>>>>>>>>> On Wed, Sep 16, 2020 at 12:28:27AM +0100, Daniel Scally wrote:
>>>>>>>>>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>>>>>>>>
>>>>>>>>>> This implements the remaining .graph_* callbacks in the
>>>>>>>>>> fwnode operations vector for the software nodes. That makes
>>>>>>>>>> the fwnode_graph*() functions available in the drivers also
>>>>>>>>>> when software nodes are used.
>>>>>>>>>>
>>>>>>>>>> The implementation tries to mimic the "OF graph" as much as
>>>>>>>>>> possible, but there is no support for the "reg" device
>>>>>>>>>> property. The ports will need to have the index in their
>>>>>>>>>> name which starts with "port" (for example "port0", "port1",
>>>>>>>>>> ...) and endpoints will use the index of the software node
>>>>>>>>>> that is given to them during creation. The port nodes can
>>>>>>>>>> also be grouped under a specially named "ports" subnode,
>>>>>>>>>> just like in DT, if necessary.
>>>>>>>>>>
>>>>>>>>>> The remote-endpoints are reference properties under the
>>>>>>>>>> endpoint nodes that are named "remote-endpoint". 
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>>>>>>>> Co-developed-by: Daniel Scally <djrscally@gmail.com>
>>>>>>>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>>>>>>>>> ---
>>>>>>>>>> changes in v2:
>>>>>>>>>> 	- added software_node_device_is_available
>>>>>>>>>> 	- altered software_node_get_next_child to get references
>>>>>>>>>> 	- altered software_node_get_next_endpoint to release references
>>>>>>>>>> 	to ports and avoid passing invalid combinations of swnodes to
>>>>>>>>>> 	software_node_get_next_child
>>>>>>>>>> 	- altered swnode_graph_find_next_port to release port rather than
>>>>>>>>>> 	old
>>>>>>>>>> 	
>>>>>>>>>>  drivers/base/swnode.c | 129 +++++++++++++++++++++++++++++++++++++++++-
>>>>>>>>>>  1 file changed, 127 insertions(+), 2 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
>>>>>>>>>> index 010828fc785b..d69034b807e3 100644
>>>>>>>>>> --- a/drivers/base/swnode.c
>>>>>>>>>> +++ b/drivers/base/swnode.c
>>>>>>>>>> @@ -363,6 +363,11 @@ static void software_node_put(struct fwnode_handle *fwnode)
>>>>>>>>>>  	kobject_put(&swnode->kobj);
>>>>>>>>>>  }
>>>>>>>>>>  
>>>>>>>>>> +static bool software_node_device_is_available(const struct fwnode_handle *fwnode)
>>>>>>>>>> +{
>>>>>>>>>> +	return is_software_node(fwnode);
>>>>>>>>> This basically tells whether the device is there. Are there software node
>>>>>>>>> based devices, i.e. do you need this?
>>>>>>>>>
>>>>>>>>> If you do really need this, then I guess this could just return true for
>>>>>>>>> now as if you somehow get here, the node is a software node anyway.
>>>>>>>> I do think its better to include it; I'm targeting using this with
>>>>>>>> ipu3-cio2; the cio2_parse_firmware() call there doesn't pass
>>>>>>>> FWNODE_GRAPH_DEVICE_DISABLED to fwnode_graph_get_endpoint_by_id() so
>>>>>>> I wonder if this has something to do with replacing the device's fwnode
>>>>>>> in the cio2-bridge patch.
>>>>>>>
>>>>>>> It's the device that needs to be enabled, and it's not a software node.
>>>>>>>
>>>>>> I think it is because of that yes, but I don't see a way around it at
>>>>>> the moment - unless there's a way to attach the software_node port and
>>>>>> endpoints that cio2-bridge creates to the device's existing firmware
>>>>>> instead.
>>>>> I thought this was how it was meant to be used?
>>>>>
>>>>> The secondary field is there for this purpose. But it may be not all fwnode
>>>>> interface functions operate on fwnode->secondary?
>>>> Let me test it; it might just require some changes to
>>>> software_node_graph_get_port_parent() to check if the parent fwnode is a
>>>> secondary, and if it is to return the primary instead.
>>> Ah, indeed. I forgot this part. I wonder if it'd cause issues to return the
>>> primary if you've got the secondary swnode.
>>>
>>> Heikki, any idea?
>>>
>>> Code elsewhere (e.g. V4L2 fwnode framework + drivers) assume a device is
>>> identified by a single fwnode, not two --- currently the swnode graph
>>> function returning port parent returns the secondary so there's no match
>>> with the primary fwnode.
>> Sorry I don't think I understand the scenario here, but never return
>> the primary node when the software node is the secondary from the
>> software node API! The software node functions deal and return
>> software nodes, and nothing else, just like ACPI deals with ACPI nodes
>> only and DT deals with OF nodes only. We must never jump between the
>> fwnode types at this level. That also means that if you want to
>> describe the device graph with software nodes, then every node in the
>> graph, starting from the port parents, must be a software node.
>> Whether or not the node is secondary is irrelevant. But I guess this
>> is not a problem here (or is it?).
> The way software nodes work (as in this patch) is not consistent with DT or
> ACPI. For instance, the parent of the port node, returned by
> software_node_graph_get_port_parent() is fwnode->secondary of the device,
> not device's fwnode.
At the moment this isn't the case; at least in the cio2-bridge, I've
been setting the device's _primary_ fwnode to the software_node that the
driver creates. Sorry to confuse things; I thought you were suggesting I
set the software node as fwnode->secondary of the device instead, and
arrange it so that when other bits of code fetch the "device node" via
software_node_get_port_parent() it returns the primary rather than the
software_node secondary, meaning we wouldn't need
software_node_device_is_available() because when something calls
fwnode_device_is_available() it would be using the existing device
firmware node instead of the software node.
> This is not expected by the users of the fwnode property API.
>
> Also, it leads to drivers only seeing the software nodes while DT and ACPI
> nodes as well as properties would be hidden.
>
>> Considering the secondary node will unfortunately need to be done by
>> the callers of fwnode API when the fwnode API can't take care of that.
> What problems would there be in returning the primary fwnode?
>

Re: [PATCH v2] software_node: Add support for fwnode_graph*() family of functions

[Andy Shevchenko]

On Fri, Sep 18, 2020 at 10:57:41AM +0300, Sakari Ailus wrote:
> On Fri, Sep 18, 2020 at 08:46:52AM +0100, Dan Scally wrote:
> > On 18/09/2020 08:34, Sakari Ailus wrote:
> > > On Fri, Sep 18, 2020 at 07:49:31AM +0100, Dan Scally wrote:

...

> > > The secondary field is there for this purpose. But it may be not all fwnode
> > > interface functions operate on fwnode->secondary?
> > Let me test it; it might just require some changes to
> > software_node_graph_get_port_parent() to check if the parent fwnode is a
> > secondary, and if it is to return the primary instead.
> 
> Ah, indeed. I forgot this part. I wonder if it'd cause issues to return the
> primary if you've got the secondary swnode.
> 
> Heikki, any idea?
> 
> Code elsewhere (e.g. V4L2 fwnode framework + drivers) assume a device is
> identified by a single fwnode, not two --- currently the swnode graph
> function returning port parent returns the secondary so there's no match
> with the primary fwnode.

At least recently I made a patch (114dbb4fa7c4) to support secondary in
device_get_next_child_node(). I'm not sure how we can do it on fwnode level.

And now I'm thinking that above mentioned change is just particular case, but
doesn't really move entire device property API to that.

-- 
With Best Regards,
Andy Shevchenko