diff mbox series

[v3,07/14] software_node: Add support for fwnode_graph*() family of functions

Message ID 20201224010907.263125-8-djrscally@gmail.com
State Superseded
Headers show
Series None | expand

Commit Message

Daniel Scally Dec. 24, 2020, 1:09 a.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".

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(-)

Comments

Laurent Pinchart Dec. 24, 2020, 12:53 p.m. UTC | #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,
>  };
>  
>  /* -------------------------------------------------------------------------- */
Laurent Pinchart Dec. 24, 2020, 12:55 p.m. UTC | #2
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,
> >  };
> >
> >  /* -------------------------------------------------------------------------- */
Andy Shevchenko Dec. 24, 2020, 1:03 p.m. UTC | #3
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;
Daniel Scally Dec. 24, 2020, 2:24 p.m. UTC | #4
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!
Daniel Scally Dec. 26, 2020, 11:47 p.m. UTC | #5
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.
Sakari Ailus Dec. 28, 2020, 4:41 p.m. UTC | #6
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.

-- 
Kind regards,

Sakari Ailus
Daniel Scally Dec. 28, 2020, 9:37 p.m. UTC | #7
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 mbox series

Patch

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