| 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 |
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
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 --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, }; /* -------------------------------------------------------------------------- */