| Message ID | 20201224010907.263125-8-djrscally@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows | expand |
On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally <djrscally@gmail.com> wrote: > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > This implements the remaining .graph_* callbacks in the .graph_* ==> ->graph_*() ? > fwnode operations structure for the software nodes. That makes > the fwnode_graph*() functions available in the drivers also graph*() -> graph_*() ? > 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", > ...) Looks not good, perhaps move to the previous line, or move port@1 example here? > 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". Few nitpicks here and there, after addressing them, 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 v3 > - Changed software_node_get_next_endpoint() to drop the variable > named "old" > - Used the macros defined in 06/14 instead of magic numbers > - Added some comments to explain behaviour a little where it's unclear > > drivers/base/swnode.c | 112 +++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 111 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c > index 2d07eb04c6c8..ff690029060d 100644 > --- a/drivers/base/swnode.c > +++ b/drivers/base/swnode.c > @@ -540,6 +540,112 @@ 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))) { > + /* > + * ports have naming style "port@n", so we search for children > + * that follow that convention (but without assuming anything > + * about the index number) > + */ > + if (!strncmp(to_swnode(port)->node->name, "port@", You may use here corresponding _FMT macro. > + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN)) > + 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); > + int ret; > + > + /* Ports have naming style "port@n", we need to select the n */ > + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN, Maybe a temporary variable? unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN; ... ret = kstrtou32(swnode->parent->node->name + prefix_len, > + 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 +657,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 >
Hi Daniel, Thank you for the patch. On Thu, Dec 24, 2020 at 01:09:00AM +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". > > 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 > - Changed software_node_get_next_endpoint() to drop the variable > named "old" > - Used the macros defined in 06/14 instead of magic numbers > - Added some comments to explain behaviour a little where it's unclear > > drivers/base/swnode.c | 112 +++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 111 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c > index 2d07eb04c6c8..ff690029060d 100644 > --- a/drivers/base/swnode.c > +++ b/drivers/base/swnode.c > @@ -540,6 +540,112 @@ 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))) { > + /* > + * ports have naming style "port@n", so we search for children > + * that follow that convention (but without assuming anything > + * about the index number) > + */ > + if (!strncmp(to_swnode(port)->node->name, "port@", > + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN)) I would either add a macro to replace the prefix ("port@"), or drop FWNODE_GRAPH_PORT_NAME_PREFIX_LEN. I think this is the worst of both worlds, the string and its length are defined in two different places :-) I would personally drop the macro, but I don't mind either way as long as the string and its length are defined in the same place. > + 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); > + int ret; > + > + /* Ports have naming style "port@n", we need to select the n */ > + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN, > + 10, &endpoint->port); Same here. I wonder if we should add a check to ensure parent->node->name is long enough (and possibly even start with the right prefix), as otherwise the pointer passed to kstrtou32() may be past the end of the string. Maybe this is overkill, if we can rely on the fact that software nodes have correct names. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + 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 +657,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, > }; > > /* -------------------------------------------------------------------------- */
Hi Andy, On Thu, Dec 24, 2020 at 02:24:12PM +0200, Andy Shevchenko wrote: > On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally wrote: > > > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > This implements the remaining .graph_* callbacks in the > > .graph_* ==> ->graph_*() ? > > > fwnode operations structure for the software nodes. That makes > > the fwnode_graph*() functions available in the drivers also > > graph*() -> graph_*() ? > > > 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", > > > ...) > > Looks not good, perhaps move to the previous line, or move port@1 example here? > > > 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". > > Few nitpicks here and there, after addressing them, > 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 v3 > > - Changed software_node_get_next_endpoint() to drop the variable > > named "old" > > - Used the macros defined in 06/14 instead of magic numbers > > - Added some comments to explain behaviour a little where it's unclear > > > > drivers/base/swnode.c | 112 +++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 111 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c > > index 2d07eb04c6c8..ff690029060d 100644 > > --- a/drivers/base/swnode.c > > +++ b/drivers/base/swnode.c > > @@ -540,6 +540,112 @@ 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))) { > > + /* > > + * ports have naming style "port@n", so we search for children > > + * that follow that convention (but without assuming anything > > + * about the index number) > > + */ > > > + if (!strncmp(to_swnode(port)->node->name, "port@", > > You may use here corresponding _FMT macro. > > > + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN)) > > + 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); > > + int ret; > > + > > + /* Ports have naming style "port@n", we need to select the n */ > > > + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN, > > Maybe a temporary variable? > > unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN; > ... > ret = kstrtou32(swnode->parent->node->name + prefix_len, Honestly I'm wondering if those macros don't hinder readability. I'd rather write + strlen("port@") and let the compiler optimize this to a compile-time constant. > > + 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 +657,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, > > }; > > > > /* -------------------------------------------------------------------------- */
On Thu, Dec 24, 2020 at 2:55 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Thu, Dec 24, 2020 at 02:24:12PM +0200, Andy Shevchenko wrote: > > On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally wrote: ... > > > + if (!strncmp(to_swnode(port)->node->name, "port@", > > > > You may use here corresponding _FMT macro. > > > > > + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN)) > > > + return port; ... > > > + /* Ports have naming style "port@n", we need to select the n */ > > > > > + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN, > > > > Maybe a temporary variable? > > > > unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN; > > ... > > ret = kstrtou32(swnode->parent->node->name + prefix_len, > > Honestly I'm wondering if those macros don't hinder readability. I'd > rather write > > + strlen("port@") Works for me, since the compiler optimizes this away to be a plain constant. > and let the compiler optimize this to a compile-time constant. > > > > + 10, &endpoint->port); > > > + if (ret) > > > + return ret;
Hi Andy, Laurent > On Thu, Dec 24, 2020 at 2:55 PM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: >> On Thu, Dec 24, 2020 at 02:24:12PM +0200, Andy Shevchenko wrote: >>> On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally wrote: > > ... > >>>> + if (!strncmp(to_swnode(port)->node->name, "port@", >>> >>> You may use here corresponding _FMT macro. >>> >>>> + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN)) >>>> + return port; > > ... > >>>> + /* Ports have naming style "port@n", we need to select the n */ >>> >>>> + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN, >>> >>> Maybe a temporary variable? >>> >>> unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN; >>> ... >>> ret = kstrtou32(swnode->parent->node->name + prefix_len, >> >> Honestly I'm wondering if those macros don't hinder readability. I'd >> rather write >> >> + strlen("port@") > > Works for me, since the compiler optimizes this away to be a plain constant. Well, how about instead of the LEN macro, we have: #define FWNODE_GRAPH_PORT_NAME_PREFIX "port@" #define FWNODE_GRAPH_PORT_NAME_FMT FWNODE_GRAPH_PORT_NAME_PREFIX "%u" And then it's still maintainable in one place but (I think) slightly less clunky, since we can do strlen(FWNODE_GRAPH_PORT_NAME_PREFIX) Or we can do strlen("port@"), I'm good either way :)
On 24/12/2020 12:53, Laurent Pinchart wrote: >> + while ((port = software_node_get_next_child(parent, old))) { >> + /* >> + * ports have naming style "port@n", so we search for children >> + * that follow that convention (but without assuming anything >> + * about the index number) >> + */ >> + if (!strncmp(to_swnode(port)->node->name, "port@", >> + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN)) > > I would either add a macro to replace the prefix ("port@"), or drop > FWNODE_GRAPH_PORT_NAME_PREFIX_LEN. I think this is the worst of both > worlds, the string and its length are defined in two different places > :-) > > I would personally drop the macro, but I don't mind either way as long > as the string and its length are defined in the same place. OK, pending outcome of the discussion in the other thread I'll do both things the same way - whatever the decision there is. >> +static int >> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode, >> + struct fwnode_endpoint *endpoint) >> +{ >> + struct swnode *swnode = to_swnode(fwnode); >> + int ret; >> + >> + /* Ports have naming style "port@n", we need to select the n */ >> + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN, >> + 10, &endpoint->port); > > Same here. > > I wonder if we should add a check to ensure parent->node->name is long > enough (and possibly even start with the right prefix), as otherwise the > pointer passed to kstrtou32() may be past the end of the string. Maybe > this is overkill, if we can rely on the fact that software nodes have > correct names. Not necessarily actually; ports yes but endpoints no, so I think the danger is there. I'll add the check. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks!
Hi Andy On 24/12/2020 12:24, Andy Shevchenko wrote: > On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally <djrscally@gmail.com> wrote: >> >> From: Heikki Krogerus <heikki.krogerus@linux.intel.com> >> >> This implements the remaining .graph_* callbacks in the > > .graph_* ==> ->graph_*() ? > >> fwnode operations structure for the software nodes. That makes >> the fwnode_graph*() functions available in the drivers also > > graph*() -> graph_*() ? Both done. >> 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", > >> ...) > > Looks not good, perhaps move to the previous line, or move port@1 example here? Fixed - by expanding the lines to ~75 chars > Few nitpicks here and there, after addressing them, > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Thank you! >> +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))) { >> + /* >> + * ports have naming style "port@n", so we search for children >> + * that follow that convention (but without assuming anything >> + * about the index number) >> + */ > >> + if (!strncmp(to_swnode(port)->node->name, "port@", > > You may use here corresponding _FMT macro. >> +static int >> +software_node_graph_parse_endpoint(const struct fwnode_handle *fwnode, >> + struct fwnode_endpoint *endpoint) >> +{ >> + struct swnode *swnode = to_swnode(fwnode); >> + int ret; >> + >> + /* Ports have naming style "port@n", we need to select the n */ > >> + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN, > > Maybe a temporary variable? > > unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN; > ... > ret = kstrtou32(swnode->parent->node->name + prefix_len, Both discussed after Laurent's reply I think.
Hi Daniel, On Thu, Dec 24, 2020 at 02:21:15PM +0000, Daniel Scally wrote: > Hi Andy, Laurent > > > On Thu, Dec 24, 2020 at 2:55 PM Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > >> On Thu, Dec 24, 2020 at 02:24:12PM +0200, Andy Shevchenko wrote: > >>> On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally wrote: > > > > ... > > > >>>> + if (!strncmp(to_swnode(port)->node->name, "port@", > >>> > >>> You may use here corresponding _FMT macro. > >>> > >>>> + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN)) > >>>> + return port; > > > > ... > > > >>>> + /* Ports have naming style "port@n", we need to select the n */ > >>> > >>>> + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN, > >>> > >>> Maybe a temporary variable? > >>> > >>> unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN; > >>> ... > >>> ret = kstrtou32(swnode->parent->node->name + prefix_len, > >> > >> Honestly I'm wondering if those macros don't hinder readability. I'd > >> rather write > >> > >> + strlen("port@") > > > > Works for me, since the compiler optimizes this away to be a plain constant. > > Well, how about instead of the LEN macro, we have: > > #define FWNODE_GRAPH_PORT_NAME_PREFIX "port@" > #define FWNODE_GRAPH_PORT_NAME_FMT FWNODE_GRAPH_PORT_NAME_PREFIX "%u" > > And then it's still maintainable in one place but (I think) slightly > less clunky, since we can do strlen(FWNODE_GRAPH_PORT_NAME_PREFIX) > > Or we can do strlen("port@"), I'm good either way :) I'd be in favour of using strlen("port@") here. At least for now. I think refactoring the use of such strings could be a separate set at another time, if that's seen as the way to go.
Hi Sakari On 28/12/2020 16:41, Sakari Ailus wrote: > Hi Daniel, > > On Thu, Dec 24, 2020 at 02:21:15PM +0000, Daniel Scally wrote: >> Hi Andy, Laurent >> >>> On Thu, Dec 24, 2020 at 2:55 PM Laurent Pinchart >>> <laurent.pinchart@ideasonboard.com> wrote: >>>> On Thu, Dec 24, 2020 at 02:24:12PM +0200, Andy Shevchenko wrote: >>>>> On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally wrote: >>> >>> ... >>> >>>>>> + if (!strncmp(to_swnode(port)->node->name, "port@", >>>>> >>>>> You may use here corresponding _FMT macro. >>>>> >>>>>> + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN)) >>>>>> + return port; >>> >>> ... >>> >>>>>> + /* Ports have naming style "port@n", we need to select the n */ >>>>> >>>>>> + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN, >>>>> >>>>> Maybe a temporary variable? >>>>> >>>>> unsigned int prefix_len = FWNODE_GRAPH_PORT_NAME_PREFIX_LEN; >>>>> ... >>>>> ret = kstrtou32(swnode->parent->node->name + prefix_len, >>>> >>>> Honestly I'm wondering if those macros don't hinder readability. I'd >>>> rather write >>>> >>>> + strlen("port@") >>> >>> Works for me, since the compiler optimizes this away to be a plain constant. >> >> Well, how about instead of the LEN macro, we have: >> >> #define FWNODE_GRAPH_PORT_NAME_PREFIX "port@" >> #define FWNODE_GRAPH_PORT_NAME_FMT FWNODE_GRAPH_PORT_NAME_PREFIX "%u" >> >> And then it's still maintainable in one place but (I think) slightly >> less clunky, since we can do strlen(FWNODE_GRAPH_PORT_NAME_PREFIX) >> >> Or we can do strlen("port@"), I'm good either way :) > > I'd be in favour of using strlen("port@") here. > > At least for now. I think refactoring the use of such strings could be a > separate set at another time, if that's seen as the way to go. Alright - thanks. In that case I'll switch to strlen("port@0"), and FWNODE_GRAPH_PORT_NAME_LEN can be dropped then I think.
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index 2d07eb04c6c8..ff690029060d 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -540,6 +540,112 @@ 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))) { + /* + * ports have naming style "port@n", so we search for children + * that follow that convention (but without assuming anything + * about the index number) + */ + if (!strncmp(to_swnode(port)->node->name, "port@", + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN)) + 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); + int ret; + + /* Ports have naming style "port@n", we need to select the n */ + ret = kstrtou32(swnode->parent->node->name + FWNODE_GRAPH_PORT_NAME_PREFIX_LEN, + 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 +657,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, }; /* -------------------------------------------------------------------------- */