diff mbox series

[RFC,v3,4/9] software_node: Add support for fwnode_graph*() family of functions

Message ID 20201019225903.14276-5-djrscally@gmail.com
State Superseded
Headers show
Series Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows | expand

Commit Message

Daniel Scally Oct. 19, 2020, 10:58 p.m. UTC
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 v3:
	- removed software_node_device_is_available
	- moved the change to software_node_get_next_child to a separate
	patch
	- switched to use fwnode_handle_put() in graph_get_next_endpoint()
	instead of software_node_put()

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 | 120 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 119 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko Oct. 20, 2020, 9:17 a.m. UTC | #1
On Mon, Oct 19, 2020 at 11:58:58PM +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". 


FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


> 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 v3:

> 	- removed software_node_device_is_available

> 	- moved the change to software_node_get_next_child to a separate

> 	patch

> 	- switched to use fwnode_handle_put() in graph_get_next_endpoint()

> 	instead of software_node_put()

> 

> 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 | 120 +++++++++++++++++++++++++++++++++++++++++-

>  1 file changed, 119 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c

> index 741149b90..3732530ce 100644

> --- a/drivers/base/swnode.c

> +++ b/drivers/base/swnode.c

> @@ -536,6 +536,120 @@ 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);

> +	fwnode_handle_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,

> @@ -547,7 +661,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

> 


-- 
With Best Regards,
Andy Shevchenko
Sakari Ailus Oct. 20, 2020, 12:35 p.m. UTC | #2
Hi Daniel, Heikki,

Thanks for the patch.

On Mon, Oct 19, 2020 at 11:58:58PM +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 v3:

> 	- removed software_node_device_is_available

> 	- moved the change to software_node_get_next_child to a separate

> 	patch

> 	- switched to use fwnode_handle_put() in graph_get_next_endpoint()

> 	instead of software_node_put()

> 

> 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 | 120 +++++++++++++++++++++++++++++++++++++++++-

>  1 file changed, 119 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c

> index 741149b90..3732530ce 100644

> --- a/drivers/base/swnode.c

> +++ b/drivers/base/swnode.c

> @@ -536,6 +536,120 @@ 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))) {


software_node_get_next_child() doesn't drop the reference of the old child
nor gets a reference to the returned node. Should it?

The function to get a named child node does.

It'd be nice if this was aligned. Or am I missing something?

This isn't really a comment on this patch though.

-- 
Kind regards,

Sakari Ailus
Sakari Ailus Oct. 20, 2020, 1:32 p.m. UTC | #3
On Tue, Oct 20, 2020 at 03:35:56PM +0300, Sakari Ailus wrote:
> Hi Daniel, Heikki,

> 

> Thanks for the patch.

> 

> On Mon, Oct 19, 2020 at 11:58:58PM +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 v3:

> > 	- removed software_node_device_is_available

> > 	- moved the change to software_node_get_next_child to a separate

> > 	patch

> > 	- switched to use fwnode_handle_put() in graph_get_next_endpoint()

> > 	instead of software_node_put()

> > 

> > 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 | 120 +++++++++++++++++++++++++++++++++++++++++-

> >  1 file changed, 119 insertions(+), 1 deletion(-)

> > 

> > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c

> > index 741149b90..3732530ce 100644

> > --- a/drivers/base/swnode.c

> > +++ b/drivers/base/swnode.c

> > @@ -536,6 +536,120 @@ 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))) {

> 

> software_node_get_next_child() doesn't drop the reference of the old child

> nor gets a reference to the returned node. Should it?

> 

> The function to get a named child node does.

> 

> It'd be nice if this was aligned. Or am I missing something?

> 

> This isn't really a comment on this patch though.


I didn't get this patch to my @linux.intel.com account so I guess it's our
mail servers again...

Anyway, please see my comments on that and ignore this one.

-- 
Sakari Ailus
diff mbox series

Patch

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 741149b90..3732530ce 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -536,6 +536,120 @@  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);
+	fwnode_handle_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,
@@ -547,7 +661,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,
 };
 
 /* -------------------------------------------------------------------------- */