Hi Andy On 24/12/2020 12:24, Andy Shevchenko wrote: > On Thu, Dec 24, 2020 at 3:14 AM Daniel Scally <djrscally@xxxxxxxxx> wrote: >> >> From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> >> >> 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@xxxxxxxxx> 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.