diff mbox series

[v4,08/15] software_node: Add support for fwnode_graph*() family of functions

Message ID 20210103231235.792999-9-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 Jan. 3, 2021, 11:12 p.m. UTC
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

This implements the remaining .graph_*() callbacks in the fwnode
operations structure 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
"port@0", "port@1", ...) 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".

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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 v4:

	- Replaced the FWNODE_GRAPH_PORT_NAME_PREFIX_LEN macro with
	  strlen("port@") throughout
	- Added a check to software_node_graph_parse_endpoint() to ensure
	  the name of the endpoint's parent matches the expected port@n
	  format

 drivers/base/swnode.c | 116 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 115 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko Jan. 4, 2021, 10:22 a.m. UTC | #1
On Sun, Jan 03, 2021 at 11:12:28PM +0000, Daniel Scally wrote:
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> 

> This implements the remaining .graph_*() callbacks in the fwnode

> operations structure 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

> "port@0", "port@1", ...) 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".


Couple of nitpicks below (can be considered as follow up, depends on yours and
maintainer wishes).

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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 v4:

> 

> 	- Replaced the FWNODE_GRAPH_PORT_NAME_PREFIX_LEN macro with

> 	  strlen("port@") throughout

> 	- Added a check to software_node_graph_parse_endpoint() to ensure

> 	  the name of the endpoint's parent matches the expected port@n

> 	  format

> 

>  drivers/base/swnode.c | 116 +++++++++++++++++++++++++++++++++++++++++-

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

> 

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

> index 1f43c51b431e..82f9d6326110 100644

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

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

> @@ -540,6 +540,116 @@ 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))) {

> +		/*

> +		 * fwnode ports have naming style "port@", so we search for any

> +		 * children that follow that convention.

> +		 */

> +		if (!strncmp(to_swnode(port)->node->name, "port@",

> +			     strlen("port@")))

> +			return 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 *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)) {

> +		endpoint = software_node_get_next_child(port, endpoint);

> +		if (endpoint) {

> +			fwnode_handle_put(port);

> +			break;

> +		}

> +	}

> +

> +	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);

> +

> +	swnode = swnode->parent;

> +	if (swnode && !strcmp(swnode->node->name, "ports"))

> +		swnode = swnode->parent;

> +

> +	return swnode ? software_node_get(&swnode->fwnode) : NULL;

> +}

> +

> +static int

> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,

> +				   struct fwnode_endpoint *endpoint)

> +{

> +	struct swnode *swnode = to_swnode(fwnode);

> +	const char *parent_name = swnode->parent->node->name;

> +	int ret;

> +

> +	if (!(strlen(parent_name) > strlen("port@")) ||


A nit:

	if (strlen("port@") >= strlen(parent_name) ||

better to read

> +	    strncmp(parent_name, "port@", strlen("port@")))

> +		return -EINVAL;

> +

> +	/* Ports have naming style "port@n", we need to select the n */

> +	ret = kstrtou32(parent_name + strlen("port@"),

> +			10, &endpoint->port);


A nit:

	ret = kstrtou32(parent_name + strlen("port@"), 10, &endpoint->port);

(perhaps you need to adjust your editor settings, this still fits 80)

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

> @@ -551,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.25.1

> 


-- 
With Best Regards,
Andy Shevchenko
Daniel Scally Jan. 4, 2021, 10:35 a.m. UTC | #2
Hi Andy

On 04/01/2021 10:22, Andy Shevchenko wrote:
> On Sun, Jan 03, 2021 at 11:12:28PM +0000, Daniel Scally wrote:
>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>
>> This implements the remaining .graph_*() callbacks in the fwnode
>> operations structure 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
>> "port@0", "port@1", ...) 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".
> Couple of nitpicks below (can be considered as follow up, depends on yours and
> maintainer wishes).
>
Thanks
> +static int
> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
> +				   struct fwnode_endpoint *endpoint)
> +{
> +	struct swnode *swnode = to_swnode(fwnode);
> +	const char *parent_name = swnode->parent->node->name;
> +	int ret;
> +
> +	if (!(strlen(parent_name) > strlen("port@")) ||
> A nit:
>
> 	if (strlen("port@") >= strlen(parent_name) ||
>
> better to read

yeah agreed

>
>> +	    strncmp(parent_name, "port@", strlen("port@")))
>> +		return -EINVAL;
>> +
>> +	/* Ports have naming style "port@n", we need to select the n */
>> +	ret = kstrtou32(parent_name + strlen("port@"),
>> +			10, &endpoint->port);
> A nit:
>
> 	ret = kstrtou32(parent_name + strlen("port@"), 10, &endpoint->port);
>
> (perhaps you need to adjust your editor settings, this still fits 80)
Ah - my bad. Originally instead of parent_name there was
swnode->parent->node->name, which didn't fit. When I added the temp
variable I forgot to fix the line break - thanks.
diff mbox series

Patch

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 1f43c51b431e..82f9d6326110 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -540,6 +540,116 @@  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))) {
+		/*
+		 * fwnode ports have naming style "port@", so we search for any
+		 * children that follow that convention.
+		 */
+		if (!strncmp(to_swnode(port)->node->name, "port@",
+			     strlen("port@")))
+			return 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 *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)) {
+		endpoint = software_node_get_next_child(port, endpoint);
+		if (endpoint) {
+			fwnode_handle_put(port);
+			break;
+		}
+	}
+
+	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);
+
+	swnode = swnode->parent;
+	if (swnode && !strcmp(swnode->node->name, "ports"))
+		swnode = swnode->parent;
+
+	return swnode ? software_node_get(&swnode->fwnode) : NULL;
+}
+
+static int
+software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode,
+				   struct fwnode_endpoint *endpoint)
+{
+	struct swnode *swnode = to_swnode(fwnode);
+	const char *parent_name = swnode->parent->node->name;
+	int ret;
+
+	if (!(strlen(parent_name) > strlen("port@")) ||
+	    strncmp(parent_name, "port@", strlen("port@")))
+		return -EINVAL;
+
+	/* Ports have naming style "port@n", we need to select the n */
+	ret = kstrtou32(parent_name + strlen("port@"),
+			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,
@@ -551,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,
 };
 
 /* -------------------------------------------------------------------------- */