Re: [PATCH v12 04/19] media: v4l2-fwnode: add endpoint id field to v4l2_fwnode_link

[Marco Felsch <m.felsch@pengutronix.de>]

Hi Sakari,

On 20-03-11 10:15, Sakari Ailus wrote:
> Hi Marco,
> 
> On Mon, Mar 09, 2020 at 11:14:13AM +0100, Marco Felsch wrote:
> > A link is between two endpoints not between two ports to be more
> > precise. Add the local_id/remote_id field which stores the endpoint
> > reg/port property to the link. Now the link holds all necessary
> > information about a link.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > 
> > v12:
> > - New in this serie
> > 
> >  drivers/media/v4l2-core/v4l2-fwnode.c | 2 ++
> >  include/media/v4l2-fwnode.h           | 4 ++++
> >  2 files changed, 6 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index 6ece4320e1d2..78c32aebbe03 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -565,6 +565,7 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *__fwnode,
> >  
> >  	memset(link, 0, sizeof(*link));
> >  
> > +	fwnode_property_read_u32(__fwnode, "reg", &link->local_id);
> >  	fwnode = fwnode_get_parent(__fwnode);
> >  	fwnode_property_read_u32(fwnode, port_prop, &link->local_port);
> >  	fwnode = fwnode_get_next_parent(fwnode);
> > @@ -578,6 +579,7 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *__fwnode,
> >  		return -ENOLINK;
> >  	}
> >  
> > +	fwnode_property_read_u32(fwnode, "reg", &link->remote_id);
> 
> This code really should use fwnode_graph_parse_endpoint(), and not
> implement the parsing locally.
> 
> I do think the end result would be better if the code using this function
> would be calling the fwnode graph APIs directly.

I think doing it manually was used due to the
"port" (ACPI) <-> "reg" (DT) property difference. I'm completely with
you to change it to fwnode_graph_parse_endpoint() since ACPI also uses
"reg" as port numbering (as you explained it to me).

Regards,
  Marco

> >  	fwnode = fwnode_get_parent(fwnode);
> >  	fwnode_property_read_u32(fwnode, port_prop, &link->remote_port);
> >  	fwnode = fwnode_get_next_parent(fwnode);
> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > index f6a7bcd13197..7bba6dfa3fd6 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -113,14 +113,18 @@ struct v4l2_fwnode_endpoint {
> >   * struct v4l2_fwnode_link - a link between two endpoints
> >   * @local_node: pointer to device_node of this endpoint
> >   * @local_port: identifier of the port this endpoint belongs to
> > + * @local_id: identifier of the id this endpoint belongs to
> >   * @remote_node: pointer to device_node of the remote endpoint
> >   * @remote_port: identifier of the port the remote endpoint belongs to
> > + * @remote_id: identifier of the id the remote endpoint belongs to
> >   */
> >  struct v4l2_fwnode_link {
> >  	struct fwnode_handle *local_node;
> >  	unsigned int local_port;
> > +	unsigned int local_id;
> >  	struct fwnode_handle *remote_node;
> >  	unsigned int remote_port;
> > +	unsigned int remote_id;
> >  };
> >  
> >  /**
> 
> -- 
> Sakari Ailus
>