[PATCH 0/2] rcar-csi2: make use V4L2_ASYNC_MATCH_CUSTOM to do fwnode matching

[PATCH 0/2] rcar-csi2: make use V4L2_ASYNC_MATCH_CUSTOM to do fwnode matching

[Lad Prabhakar]

Hi All,

This patch series adds support for fwnode matching to be handled by rcar-csi2 driver.

Thanks,
Prabhakar

Lad Prabhakar (2):
  media: v4l2-async: Pass pointer to struct v4l2_subdev in match_custom
    callback
  media: rcar-csi2: Let the driver handle fwnode matching using
    match_custom callback

 drivers/media/platform/rcar-vin/rcar-csi2.c | 46 +++++++++++++++++++--
 drivers/media/v4l2-core/v4l2-async.c        |  2 +-
 include/media/v4l2-async.h                  |  4 +-
 3 files changed, 46 insertions(+), 6 deletions(-)

-- 
2.20.1

[PATCH 1/2] media: v4l2-async: Pass pointer to struct v4l2_subdev in match_custom callback

[Lad Prabhakar]

Passing a pointer to struct device for the match_custom callback is of no
use as in the bridge driver to match the fwnode, so instead pass the
struct v4l2_subdev pointer so that the bridge driver has enough
information to match against the subdevices.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/v4l2-core/v4l2-async.c | 2 +-
 include/media/v4l2-async.h           | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 8bde33c21ce4..f897d4025f97 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -80,7 +80,7 @@ static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 		/* Match always */
 		return true;
 
-	return asd->match.custom.match(sd->dev, asd);
+	return asd->match.custom.match(sd, asd);
 }
 
 static LIST_HEAD(subdev_list);
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index 8319284c93cb..8c014e3bbd6c 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -86,8 +86,8 @@ struct v4l2_async_subdev {
 			unsigned short address;
 		} i2c;
 		struct {
-			bool (*match)(struct device *dev,
-				      struct v4l2_async_subdev *sd);
+			bool (*match)(struct v4l2_subdev *sd,
+				      struct v4l2_async_subdev *asd);
 			void *priv;
 		} custom;
 	} match;
-- 
2.20.1

[PATCH 2/2] media: rcar-csi2: Let the driver handle fwnode matching using match_custom callback

[Lad Prabhakar]

The rcar-csi2 driver uses the v4l2-async framework to do endpoint matching
instead of node matching. This is needed as it needs to work with the
adv748x driver which register it self in v4l2-async using endpoints
instead of nodes. The reason for this is that from a single DT node it
creates multiple subdevices, one for each endpoint.

But when using subdevs which register itself in v4l2-async using nodes,
the rcar-csi2 driver failed to find the matching endpoint because the
match.fwnode was pointing to remote endpoint instead of remote parent
port.

This commit adds support in rcar-csi2 driver to handle both the cases
where subdev registers in v4l2-async using endpoints/nodes, by using
match_type as V4L2_ASYNC_MATCH_CUSTOM and implementing the match()
callback to compare the fwnode of either remote/parent.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 46 +++++++++++++++++++--
 1 file changed, 43 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index faa9fb23a2e9..1bbf05e9f025 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -808,6 +808,46 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
 	return 0;
 }
 
+static bool rcsi2_asd_match(struct v4l2_subdev *sd,
+			    struct v4l2_async_subdev *asd)
+{
+	struct rcar_csi2 *priv = (struct rcar_csi2 *)asd->match.custom.priv;
+	struct fwnode_handle *remote_endpoint;
+	struct fwnode_handle *subdev_endpoint;
+	struct device_node *np;
+	bool matched = false;
+
+	np = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
+	if (!np) {
+		dev_err(priv->dev, "Not connected to subdevice\n");
+		return matched;
+	}
+
+	remote_endpoint =
+		fwnode_graph_get_remote_endpoint(of_fwnode_handle(np));
+	if (!remote_endpoint) {
+		dev_err(priv->dev, "Failed to get remote endpoint\n");
+		of_node_put(np);
+		return matched;
+	}
+	of_node_put(np);
+
+	if (sd->fwnode != dev_fwnode(sd->dev)) {
+		if (remote_endpoint == sd->fwnode)
+			matched = true;
+	} else {
+		subdev_endpoint =
+		      fwnode_graph_get_next_endpoint(dev_fwnode(sd->dev), NULL);
+		if (remote_endpoint == subdev_endpoint)
+			matched = true;
+		fwnode_handle_put(subdev_endpoint);
+	}
+
+	fwnode_handle_put(remote_endpoint);
+
+	return matched;
+}
+
 static int rcsi2_parse_dt(struct rcar_csi2 *priv)
 {
 	struct device_node *ep;
@@ -833,9 +873,9 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
 		return ret;
 	}
 
-	priv->asd.match.fwnode =
-		fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
-	priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
+	priv->asd.match.custom.match = &rcsi2_asd_match;
+	priv->asd.match.custom.priv = priv;
+	priv->asd.match_type = V4L2_ASYNC_MATCH_CUSTOM;
 
 	of_node_put(ep);
 
-- 
2.20.1

Re: [PATCH 2/2] media: rcar-csi2: Let the driver handle fwnode matching using match_custom callback

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Sun, Mar 15, 2020 at 10:27:24AM +0000, Lad Prabhakar wrote:
> The rcar-csi2 driver uses the v4l2-async framework to do endpoint matching
> instead of node matching. This is needed as it needs to work with the
> adv748x driver which register it self in v4l2-async using endpoints
> instead of nodes. The reason for this is that from a single DT node it
> creates multiple subdevices, one for each endpoint.
> 
> But when using subdevs which register itself in v4l2-async using nodes,
> the rcar-csi2 driver failed to find the matching endpoint because the
> match.fwnode was pointing to remote endpoint instead of remote parent
> port.
> 
> This commit adds support in rcar-csi2 driver to handle both the cases
> where subdev registers in v4l2-async using endpoints/nodes, by using
> match_type as V4L2_ASYNC_MATCH_CUSTOM and implementing the match()
> callback to compare the fwnode of either remote/parent.

This is not the way to go. The v4l2-async framework needs to be fixed
instead, so that fwnode match will do the right thing automatically
regardless of whether the node is a device node or and endpoint node.

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 46 +++++++++++++++++++--
>  1 file changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index faa9fb23a2e9..1bbf05e9f025 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -808,6 +808,46 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
>  	return 0;
>  }
>  
> +static bool rcsi2_asd_match(struct v4l2_subdev *sd,
> +			    struct v4l2_async_subdev *asd)
> +{
> +	struct rcar_csi2 *priv = (struct rcar_csi2 *)asd->match.custom.priv;
> +	struct fwnode_handle *remote_endpoint;
> +	struct fwnode_handle *subdev_endpoint;
> +	struct device_node *np;
> +	bool matched = false;
> +
> +	np = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> +	if (!np) {
> +		dev_err(priv->dev, "Not connected to subdevice\n");
> +		return matched;
> +	}
> +
> +	remote_endpoint =
> +		fwnode_graph_get_remote_endpoint(of_fwnode_handle(np));
> +	if (!remote_endpoint) {
> +		dev_err(priv->dev, "Failed to get remote endpoint\n");
> +		of_node_put(np);
> +		return matched;
> +	}
> +	of_node_put(np);
> +
> +	if (sd->fwnode != dev_fwnode(sd->dev)) {
> +		if (remote_endpoint == sd->fwnode)
> +			matched = true;
> +	} else {
> +		subdev_endpoint =
> +		      fwnode_graph_get_next_endpoint(dev_fwnode(sd->dev), NULL);
> +		if (remote_endpoint == subdev_endpoint)
> +			matched = true;
> +		fwnode_handle_put(subdev_endpoint);
> +	}
> +
> +	fwnode_handle_put(remote_endpoint);
> +
> +	return matched;
> +}
> +
>  static int rcsi2_parse_dt(struct rcar_csi2 *priv)
>  {
>  	struct device_node *ep;
> @@ -833,9 +873,9 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
>  		return ret;
>  	}
>  
> -	priv->asd.match.fwnode =
> -		fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> -	priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> +	priv->asd.match.custom.match = &rcsi2_asd_match;
> +	priv->asd.match.custom.priv = priv;
> +	priv->asd.match_type = V4L2_ASYNC_MATCH_CUSTOM;
>  
>  	of_node_put(ep);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] media: v4l2-async: Pass pointer to struct v4l2_subdev in match_custom callback

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Sun, Mar 15, 2020 at 10:27:23AM +0000, Lad Prabhakar wrote:
> Passing a pointer to struct device for the match_custom callback is of no
> use as in the bridge driver to match the fwnode, so instead pass the
> struct v4l2_subdev pointer so that the bridge driver has enough
> information to match against the subdevices.

I'm not sure I like this. Conceptually speaking, the driver that
registers the notifier wants to get v4l2_subdev instances corresponding
to devices. A struct device is thus all it should need. Giving the match
function access to the subdev opens the door to all kind of nasty hacks.

In any case, I don't think is is required, see my reply to patch 2/2.

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 2 +-
>  include/media/v4l2-async.h           | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 8bde33c21ce4..f897d4025f97 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -80,7 +80,7 @@ static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
>  		/* Match always */
>  		return true;
>  
> -	return asd->match.custom.match(sd->dev, asd);
> +	return asd->match.custom.match(sd, asd);
>  }
>  
>  static LIST_HEAD(subdev_list);
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 8319284c93cb..8c014e3bbd6c 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -86,8 +86,8 @@ struct v4l2_async_subdev {
>  			unsigned short address;
>  		} i2c;
>  		struct {
> -			bool (*match)(struct device *dev,
> -				      struct v4l2_async_subdev *sd);
> +			bool (*match)(struct v4l2_subdev *sd,
> +				      struct v4l2_async_subdev *asd);
>  			void *priv;
>  		} custom;
>  	} match;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: rcar-csi2: Let the driver handle fwnode matching using match_custom callback

[Lad, Prabhakar]

Hi Laurent,

Thank you for the quick review.

On Sun, Mar 15, 2020 at 10:30 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Sun, Mar 15, 2020 at 10:27:24AM +0000, Lad Prabhakar wrote:
> > The rcar-csi2 driver uses the v4l2-async framework to do endpoint matching
> > instead of node matching. This is needed as it needs to work with the
> > adv748x driver which register it self in v4l2-async using endpoints
> > instead of nodes. The reason for this is that from a single DT node it
> > creates multiple subdevices, one for each endpoint.
> >
> > But when using subdevs which register itself in v4l2-async using nodes,
> > the rcar-csi2 driver failed to find the matching endpoint because the
> > match.fwnode was pointing to remote endpoint instead of remote parent
> > port.
> >
> > This commit adds support in rcar-csi2 driver to handle both the cases
> > where subdev registers in v4l2-async using endpoints/nodes, by using
> > match_type as V4L2_ASYNC_MATCH_CUSTOM and implementing the match()
> > callback to compare the fwnode of either remote/parent.
>
> This is not the way to go. The v4l2-async framework needs to be fixed
> instead, so that fwnode match will do the right thing automatically
> regardless of whether the node is a device node or and endpoint node.
>
OK, so moving forward should the v4l2-async do strictly endpoint
matching only or
both nodes/endpoints. fwnode in all the bridge drivers be replaced to
remote endpoints ?

Looking at the adv7604 its registered as node to v4l2-async which can
have upto 3 endpoints,
adv748x is the single driver which registers itself as endpoint to
v4l2-async, and rest of the
other subdevices have single endpoint and are registered as node to
v4l2-async. How would you
suggest to handle these cases.

Cheers,
--Prabhakar Lad

> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 46 +++++++++++++++++++--
> >  1 file changed, 43 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index faa9fb23a2e9..1bbf05e9f025 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -808,6 +808,46 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
> >       return 0;
> >  }
> >
> > +static bool rcsi2_asd_match(struct v4l2_subdev *sd,
> > +                         struct v4l2_async_subdev *asd)
> > +{
> > +     struct rcar_csi2 *priv = (struct rcar_csi2 *)asd->match.custom.priv;
> > +     struct fwnode_handle *remote_endpoint;
> > +     struct fwnode_handle *subdev_endpoint;
> > +     struct device_node *np;
> > +     bool matched = false;
> > +
> > +     np = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> > +     if (!np) {
> > +             dev_err(priv->dev, "Not connected to subdevice\n");
> > +             return matched;
> > +     }
> > +
> > +     remote_endpoint =
> > +             fwnode_graph_get_remote_endpoint(of_fwnode_handle(np));
> > +     if (!remote_endpoint) {
> > +             dev_err(priv->dev, "Failed to get remote endpoint\n");
> > +             of_node_put(np);
> > +             return matched;
> > +     }
> > +     of_node_put(np);
> > +
> > +     if (sd->fwnode != dev_fwnode(sd->dev)) {
> > +             if (remote_endpoint == sd->fwnode)
> > +                     matched = true;
> > +     } else {
> > +             subdev_endpoint =
> > +                   fwnode_graph_get_next_endpoint(dev_fwnode(sd->dev), NULL);
> > +             if (remote_endpoint == subdev_endpoint)
> > +                     matched = true;
> > +             fwnode_handle_put(subdev_endpoint);
> > +     }
> > +
> > +     fwnode_handle_put(remote_endpoint);
> > +
> > +     return matched;
> > +}
> > +
> >  static int rcsi2_parse_dt(struct rcar_csi2 *priv)
> >  {
> >       struct device_node *ep;
> > @@ -833,9 +873,9 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
> >               return ret;
> >       }
> >
> > -     priv->asd.match.fwnode =
> > -             fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> > -     priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > +     priv->asd.match.custom.match = &rcsi2_asd_match;
> > +     priv->asd.match.custom.priv = priv;
> > +     priv->asd.match_type = V4L2_ASYNC_MATCH_CUSTOM;
> >
> >       of_node_put(ep);
> >
>
> --
> Regards,
>
> Laurent Pinchart

[PATCH] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Laurent Pinchart]

fwnode matching was designed to match on nodes corresponding to a
device. Some drivers, however, needed to match on endpoints, and have
passed endpoint fwnodes to v4l2-async. This works when both the subdev
and the notifier use the same fwnode types (endpoint or device), but
makes drivers that use different types incompatible.

Fix this by extending the fwnode match to handle fwnodes of different
types. When the types (deduced from the node name) are different,
retrieve the device fwnode for the side that provides an endpoint
fwnode, and compare it with the device fwnode provided by the other
side. This allows interoperability between all drivers, regardless of
which type of fwnode they use for matching.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
This has been compile-tested only. Prabhakar, could you check if it
fixes your issue ?

 drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 8bde33c21ce4..995e5464cba7 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd,
 
 static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
-	return sd->fwnode == asd->match.fwnode;
+	struct fwnode_handle *other_fwnode;
+	struct fwnode_handle *dev_fwnode;
+	bool asd_fwnode_is_ep;
+	bool sd_fwnode_is_ep;
+	const char *name;
+
+	/*
+	 * Both the subdev and the async subdev can provide either an endpoint
+	 * fwnode or a device fwnode. Start with the simple case of direct
+	 * fwnode matching.
+	 */
+	if (sd->fwnode == asd->match.fwnode)
+		return true;
+
+	/*
+	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
+	 * endpoint or a device. If they're of the same type, there's no match.
+	 */
+	name = fwnode_get_name(sd->fwnode);
+	sd_fwnode_is_ep = name && strstarts(name, "endpoint");
+	name = fwnode_get_name(asd->match.fwnode);
+	asd_fwnode_is_ep = name && strstarts(name, "endpoint");
+
+	if (sd_fwnode_is_ep == asd_fwnode_is_ep)
+		return false;
+
+	/*
+	 * The sd and asd fwnodes are of different types. Get the device fwnode
+	 * parent of the endpoint fwnode, and compare it with the other fwnode.
+	 */
+	if (sd_fwnode_is_ep) {
+		dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode);
+		other_fwnode = asd->match.fwnode;
+	} else {
+		dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode);
+		other_fwnode = sd->fwnode;
+	}
+
+	fwnode_handle_put(dev_fwnode);
+
+	return dev_fwnode == other_fwnode;
 }
 
 static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: rcar-csi2: Let the driver handle fwnode matching using match_custom callback

[Laurent Pinchart]

Hi Prabhakar,

On Sun, Mar 15, 2020 at 12:10:14PM +0000, Lad, Prabhakar wrote:
> On Sun, Mar 15, 2020 at 10:30 AM Laurent Pinchart wrote:
> > On Sun, Mar 15, 2020 at 10:27:24AM +0000, Lad Prabhakar wrote:
> > > The rcar-csi2 driver uses the v4l2-async framework to do endpoint matching
> > > instead of node matching. This is needed as it needs to work with the
> > > adv748x driver which register it self in v4l2-async using endpoints
> > > instead of nodes. The reason for this is that from a single DT node it
> > > creates multiple subdevices, one for each endpoint.
> > >
> > > But when using subdevs which register itself in v4l2-async using nodes,
> > > the rcar-csi2 driver failed to find the matching endpoint because the
> > > match.fwnode was pointing to remote endpoint instead of remote parent
> > > port.
> > >
> > > This commit adds support in rcar-csi2 driver to handle both the cases
> > > where subdev registers in v4l2-async using endpoints/nodes, by using
> > > match_type as V4L2_ASYNC_MATCH_CUSTOM and implementing the match()
> > > callback to compare the fwnode of either remote/parent.
> >
> > This is not the way to go. The v4l2-async framework needs to be fixed
> > instead, so that fwnode match will do the right thing automatically
> > regardless of whether the node is a device node or and endpoint node.
>
> OK, so moving forward should the v4l2-async do strictly endpoint
> matching only or both nodes/endpoints. fwnode in all the bridge
> drivers be replaced to remote endpoints ?

Long term I think everything should use endpoint matching, but to get
there we shouldn't transition all drivers in one go. I've submitted a
patch to v4l2-async that I believe will fix your problem and allow for a
smooth transition. Could you give it a try ?

> Looking at the adv7604 its registered as node to v4l2-async which can
> have upto 3 endpoints, adv748x is the single driver which registers
> itself as endpoint to v4l2-async, and rest of the other subdevices
> have single endpoint and are registered as node to v4l2-async. How
> would you suggest to handle these cases.
> 
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-csi2.c | 46 +++++++++++++++++++--
> > >  1 file changed, 43 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > index faa9fb23a2e9..1bbf05e9f025 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > @@ -808,6 +808,46 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
> > >       return 0;
> > >  }
> > >
> > > +static bool rcsi2_asd_match(struct v4l2_subdev *sd,
> > > +                         struct v4l2_async_subdev *asd)
> > > +{
> > > +     struct rcar_csi2 *priv = (struct rcar_csi2 *)asd->match.custom.priv;
> > > +     struct fwnode_handle *remote_endpoint;
> > > +     struct fwnode_handle *subdev_endpoint;
> > > +     struct device_node *np;
> > > +     bool matched = false;
> > > +
> > > +     np = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> > > +     if (!np) {
> > > +             dev_err(priv->dev, "Not connected to subdevice\n");
> > > +             return matched;
> > > +     }
> > > +
> > > +     remote_endpoint =
> > > +             fwnode_graph_get_remote_endpoint(of_fwnode_handle(np));
> > > +     if (!remote_endpoint) {
> > > +             dev_err(priv->dev, "Failed to get remote endpoint\n");
> > > +             of_node_put(np);
> > > +             return matched;
> > > +     }
> > > +     of_node_put(np);
> > > +
> > > +     if (sd->fwnode != dev_fwnode(sd->dev)) {
> > > +             if (remote_endpoint == sd->fwnode)
> > > +                     matched = true;
> > > +     } else {
> > > +             subdev_endpoint =
> > > +                   fwnode_graph_get_next_endpoint(dev_fwnode(sd->dev), NULL);
> > > +             if (remote_endpoint == subdev_endpoint)
> > > +                     matched = true;
> > > +             fwnode_handle_put(subdev_endpoint);
> > > +     }
> > > +
> > > +     fwnode_handle_put(remote_endpoint);
> > > +
> > > +     return matched;
> > > +}
> > > +
> > >  static int rcsi2_parse_dt(struct rcar_csi2 *priv)
> > >  {
> > >       struct device_node *ep;
> > > @@ -833,9 +873,9 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
> > >               return ret;
> > >       }
> > >
> > > -     priv->asd.match.fwnode =
> > > -             fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> > > -     priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > > +     priv->asd.match.custom.match = &rcsi2_asd_match;
> > > +     priv->asd.match.custom.priv = priv;
> > > +     priv->asd.match_type = V4L2_ASYNC_MATCH_CUSTOM;
> > >
> > >       of_node_put(ep);
> > >

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Lad, Prabhakar]

Hi Laurent,

Thank you for the patch.

On Sun, Mar 15, 2020 at 12:55 PM Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
>
> fwnode matching was designed to match on nodes corresponding to a
> device. Some drivers, however, needed to match on endpoints, and have
> passed endpoint fwnodes to v4l2-async. This works when both the subdev
> and the notifier use the same fwnode types (endpoint or device), but
> makes drivers that use different types incompatible.
>
> Fix this by extending the fwnode match to handle fwnodes of different
> types. When the types (deduced from the node name) are different,
> retrieve the device fwnode for the side that provides an endpoint
> fwnode, and compare it with the device fwnode provided by the other
> side. This allows interoperability between all drivers, regardless of
> which type of fwnode they use for matching.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> This has been compile-tested only. Prabhakar, could you check if it
> fixes your issue ?
>
Yes it does handle all the case.

Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Cheers,
--Prabhakar

>  drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 8bde33c21ce4..995e5464cba7 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd,
>
>  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
>  {
> -       return sd->fwnode == asd->match.fwnode;
> +       struct fwnode_handle *other_fwnode;
> +       struct fwnode_handle *dev_fwnode;
> +       bool asd_fwnode_is_ep;
> +       bool sd_fwnode_is_ep;
> +       const char *name;
> +
> +       /*
> +        * Both the subdev and the async subdev can provide either an endpoint
> +        * fwnode or a device fwnode. Start with the simple case of direct
> +        * fwnode matching.
> +        */
> +       if (sd->fwnode == asd->match.fwnode)
> +               return true;
> +
> +       /*
> +        * Otherwise, check if the sd fwnode and the asd fwnode refer to an
> +        * endpoint or a device. If they're of the same type, there's no match.
> +        */
> +       name = fwnode_get_name(sd->fwnode);
> +       sd_fwnode_is_ep = name && strstarts(name, "endpoint");
> +       name = fwnode_get_name(asd->match.fwnode);
> +       asd_fwnode_is_ep = name && strstarts(name, "endpoint");
> +
> +       if (sd_fwnode_is_ep == asd_fwnode_is_ep)
> +               return false;
> +
> +       /*
> +        * The sd and asd fwnodes are of different types. Get the device fwnode
> +        * parent of the endpoint fwnode, and compare it with the other fwnode.
> +        */
> +       if (sd_fwnode_is_ep) {
> +               dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode);
> +               other_fwnode = asd->match.fwnode;
> +       } else {
> +               dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode);
> +               other_fwnode = sd->fwnode;
> +       }
> +
> +       fwnode_handle_put(dev_fwnode);
> +
> +       return dev_fwnode == other_fwnode;
>  }
>
>  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Jacopo Mondi]

Hi Laurent,

On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote:
> fwnode matching was designed to match on nodes corresponding to a
> device. Some drivers, however, needed to match on endpoints, and have
> passed endpoint fwnodes to v4l2-async. This works when both the subdev
> and the notifier use the same fwnode types (endpoint or device), but
> makes drivers that use different types incompatible.
>
> Fix this by extending the fwnode match to handle fwnodes of different
> types. When the types (deduced from the node name) are different,
> retrieve the device fwnode for the side that provides an endpoint
> fwnode, and compare it with the device fwnode provided by the other
> side. This allows interoperability between all drivers, regardless of
> which type of fwnode they use for matching.
>

I'm sorry but I'm not sure why would make a difference compared to
what Kieran's patch did.
https://lore.kernel.org/patchwork/patch/788637/

If the bridge matches on device node, and the remote registers more
than one endpoints it is possible to get a false match.

I'm not sure how that would happen in practice, as the bridge would be
registering the fwnode of the remote device twice, but I think that
was the reason kieran's patch has not been collected or am I
mistaken ?

> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> This has been compile-tested only. Prabhakar, could you check if it
> fixes your issue ?
>
>  drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 8bde33c21ce4..995e5464cba7 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd,
>
>  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
>  {
> -	return sd->fwnode == asd->match.fwnode;
> +	struct fwnode_handle *other_fwnode;
> +	struct fwnode_handle *dev_fwnode;
> +	bool asd_fwnode_is_ep;
> +	bool sd_fwnode_is_ep;
> +	const char *name;
> +
> +	/*
> +	 * Both the subdev and the async subdev can provide either an endpoint
> +	 * fwnode or a device fwnode. Start with the simple case of direct
> +	 * fwnode matching.
> +	 */
> +	if (sd->fwnode == asd->match.fwnode)
> +		return true;
> +
> +	/*
> +	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
> +	 * endpoint or a device. If they're of the same type, there's no match.
> +	 */
> +	name = fwnode_get_name(sd->fwnode);
> +	sd_fwnode_is_ep = name && strstarts(name, "endpoint");
> +	name = fwnode_get_name(asd->match.fwnode);
> +	asd_fwnode_is_ep = name && strstarts(name, "endpoint");
> +
> +	if (sd_fwnode_is_ep == asd_fwnode_is_ep)
> +		return false;
> +
> +	/*
> +	 * The sd and asd fwnodes are of different types. Get the device fwnode
> +	 * parent of the endpoint fwnode, and compare it with the other fwnode.
> +	 */
> +	if (sd_fwnode_is_ep) {
> +		dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode);
> +		other_fwnode = asd->match.fwnode;
> +	} else {
> +		dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode);
> +		other_fwnode = sd->fwnode;
> +	}
> +
> +	fwnode_handle_put(dev_fwnode);
> +
> +	return dev_fwnode == other_fwnode;
>  }
>
>  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Laurent Pinchart]

Hi Jacopo,

On Sun, Mar 15, 2020 at 10:47:07PM +0100, Jacopo Mondi wrote:
> On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote:
> > fwnode matching was designed to match on nodes corresponding to a
> > device. Some drivers, however, needed to match on endpoints, and have
> > passed endpoint fwnodes to v4l2-async. This works when both the subdev
> > and the notifier use the same fwnode types (endpoint or device), but
> > makes drivers that use different types incompatible.
> >
> > Fix this by extending the fwnode match to handle fwnodes of different
> > types. When the types (deduced from the node name) are different,
> > retrieve the device fwnode for the side that provides an endpoint
> > fwnode, and compare it with the device fwnode provided by the other
> > side. This allows interoperability between all drivers, regardless of
> > which type of fwnode they use for matching.
> 
> I'm sorry but I'm not sure why would make a difference compared to
> what Kieran's patch did.
> https://lore.kernel.org/patchwork/patch/788637/
> 
> If the bridge matches on device node, and the remote registers more
> than one endpoints it is possible to get a false match.

How so ? If a notifier entry points to a device node, and two subdevs
are registered with different endpoint nodes that are both part of the
same device node, the notifier will get either of them. Which subdev
match the notifier won't be guaranteed, but that's what the notifier
asked for if it contains a device node and not an endpoint node: any
subdev corresponding to the device node.

In practice notifiers will need to move to endpoint matching if they
want to get a particular subdev of a device, and this change allows
doing so without mass-patching every driver. It allows a notifier to
switch to endpoint nodes, while subdevs still use device nodes and are
gradually ported.

> I'm not sure how that would happen in practice, as the bridge would be
> registering the fwnode of the remote device twice, but I think that
> was the reason kieran's patch has not been collected or am I
> mistaken ?
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > This has been compile-tested only. Prabhakar, could you check if it
> > fixes your issue ?
> >
> >  drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++-
> >  1 file changed, 41 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 8bde33c21ce4..995e5464cba7 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd,
> >
> >  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> >  {
> > -	return sd->fwnode == asd->match.fwnode;
> > +	struct fwnode_handle *other_fwnode;
> > +	struct fwnode_handle *dev_fwnode;
> > +	bool asd_fwnode_is_ep;
> > +	bool sd_fwnode_is_ep;
> > +	const char *name;
> > +
> > +	/*
> > +	 * Both the subdev and the async subdev can provide either an endpoint
> > +	 * fwnode or a device fwnode. Start with the simple case of direct
> > +	 * fwnode matching.
> > +	 */
> > +	if (sd->fwnode == asd->match.fwnode)
> > +		return true;
> > +
> > +	/*
> > +	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
> > +	 * endpoint or a device. If they're of the same type, there's no match.
> > +	 */
> > +	name = fwnode_get_name(sd->fwnode);
> > +	sd_fwnode_is_ep = name && strstarts(name, "endpoint");
> > +	name = fwnode_get_name(asd->match.fwnode);
> > +	asd_fwnode_is_ep = name && strstarts(name, "endpoint");
> > +
> > +	if (sd_fwnode_is_ep == asd_fwnode_is_ep)
> > +		return false;
> > +
> > +	/*
> > +	 * The sd and asd fwnodes are of different types. Get the device fwnode
> > +	 * parent of the endpoint fwnode, and compare it with the other fwnode.
> > +	 */
> > +	if (sd_fwnode_is_ep) {
> > +		dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode);
> > +		other_fwnode = asd->match.fwnode;
> > +	} else {
> > +		dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode);
> > +		other_fwnode = sd->fwnode;
> > +	}
> > +
> > +	fwnode_handle_put(dev_fwnode);
> > +
> > +	return dev_fwnode == other_fwnode;
> >  }
> >
> >  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Jacopo Mondi]

Hi Laurent,

On Mon, Mar 16, 2020 at 08:34:44AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Sun, Mar 15, 2020 at 10:47:07PM +0100, Jacopo Mondi wrote:
> > On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote:
> > > fwnode matching was designed to match on nodes corresponding to a
> > > device. Some drivers, however, needed to match on endpoints, and have
> > > passed endpoint fwnodes to v4l2-async. This works when both the subdev
> > > and the notifier use the same fwnode types (endpoint or device), but
> > > makes drivers that use different types incompatible.
> > >
> > > Fix this by extending the fwnode match to handle fwnodes of different
> > > types. When the types (deduced from the node name) are different,
> > > retrieve the device fwnode for the side that provides an endpoint
> > > fwnode, and compare it with the device fwnode provided by the other
> > > side. This allows interoperability between all drivers, regardless of
> > > which type of fwnode they use for matching.
> >
> > I'm sorry but I'm not sure why would make a difference compared to
> > what Kieran's patch did.
> > https://lore.kernel.org/patchwork/patch/788637/
> >
> > If the bridge matches on device node, and the remote registers more
> > than one endpoints it is possible to get a false match.
>
> How so ? If a notifier entry points to a device node, and two subdevs
> are registered with different endpoint nodes that are both part of the
> same device node, the notifier will get either of them. Which subdev
> match the notifier won't be guaranteed, but that's what the notifier
> asked for if it contains a device node and not an endpoint node: any
> subdev corresponding to the device node.
>
> In practice notifiers will need to move to endpoint matching if they
> want to get a particular subdev of a device, and this change allows
> doing so without mass-patching every driver. It allows a notifier to
> switch to endpoint nodes, while subdevs still use device nodes and are
> gradually ported.
>

In case a device has two CSI-2 receivers, different IPs, different
drivers which register different notifiers, and they are connected in
DTS to a device like adv748x which registers two async
devices for its two CSI-2 transmitter on endpoints, depending on which
CSI-2 receiver gets probed first, it matches any of the two CSI-2 Tx.
The media graph would complete but it won't be what's described in
dts.

I agree it's unlikely, and having something like this or what kieran
did in is better than the current situation, so I'm not pushing this
back, at all. Just pointing possible reasons why we still don't have
any solution to this issue in mainline.

Thanks
   j

> > I'm not sure how that would happen in practice, as the bridge would be
> > registering the fwnode of the remote device twice, but I think that
> > was the reason kieran's patch has not been collected or am I
> > mistaken ?
> >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > ---
> > > This has been compile-tested only. Prabhakar, could you check if it
> > > fixes your issue ?
> > >
> > >  drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++-
> > >  1 file changed, 41 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > index 8bde33c21ce4..995e5464cba7 100644
> > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd,
> > >
> > >  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> > >  {
> > > -	return sd->fwnode == asd->match.fwnode;
> > > +	struct fwnode_handle *other_fwnode;
> > > +	struct fwnode_handle *dev_fwnode;
> > > +	bool asd_fwnode_is_ep;
> > > +	bool sd_fwnode_is_ep;
> > > +	const char *name;
> > > +
> > > +	/*
> > > +	 * Both the subdev and the async subdev can provide either an endpoint
> > > +	 * fwnode or a device fwnode. Start with the simple case of direct
> > > +	 * fwnode matching.
> > > +	 */
> > > +	if (sd->fwnode == asd->match.fwnode)
> > > +		return true;
> > > +
> > > +	/*
> > > +	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
> > > +	 * endpoint or a device. If they're of the same type, there's no match.
> > > +	 */
> > > +	name = fwnode_get_name(sd->fwnode);
> > > +	sd_fwnode_is_ep = name && strstarts(name, "endpoint");
> > > +	name = fwnode_get_name(asd->match.fwnode);
> > > +	asd_fwnode_is_ep = name && strstarts(name, "endpoint");
> > > +
> > > +	if (sd_fwnode_is_ep == asd_fwnode_is_ep)
> > > +		return false;
> > > +
> > > +	/*
> > > +	 * The sd and asd fwnodes are of different types. Get the device fwnode
> > > +	 * parent of the endpoint fwnode, and compare it with the other fwnode.
> > > +	 */
> > > +	if (sd_fwnode_is_ep) {
> > > +		dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode);
> > > +		other_fwnode = asd->match.fwnode;
> > > +	} else {
> > > +		dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode);
> > > +		other_fwnode = sd->fwnode;
> > > +	}
> > > +
> > > +	fwnode_handle_put(dev_fwnode);
> > > +
> > > +	return dev_fwnode == other_fwnode;
> > >  }
> > >
> > >  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Laurent Pinchart]

Hi Jacopo,

On Mon, Mar 16, 2020 at 09:59:34AM +0100, Jacopo Mondi wrote:
> On Mon, Mar 16, 2020 at 08:34:44AM +0200, Laurent Pinchart wrote:
> > On Sun, Mar 15, 2020 at 10:47:07PM +0100, Jacopo Mondi wrote:
> >> On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote:
> >>> fwnode matching was designed to match on nodes corresponding to a
> >>> device. Some drivers, however, needed to match on endpoints, and have
> >>> passed endpoint fwnodes to v4l2-async. This works when both the subdev
> >>> and the notifier use the same fwnode types (endpoint or device), but
> >>> makes drivers that use different types incompatible.
> >>>
> >>> Fix this by extending the fwnode match to handle fwnodes of different
> >>> types. When the types (deduced from the node name) are different,
> >>> retrieve the device fwnode for the side that provides an endpoint
> >>> fwnode, and compare it with the device fwnode provided by the other
> >>> side. This allows interoperability between all drivers, regardless of
> >>> which type of fwnode they use for matching.
> >>
> >> I'm sorry but I'm not sure why would make a difference compared to
> >> what Kieran's patch did.
> >> https://lore.kernel.org/patchwork/patch/788637/
> >>
> >> If the bridge matches on device node, and the remote registers more
> >> than one endpoints it is possible to get a false match.
> >
> > How so ? If a notifier entry points to a device node, and two subdevs
> > are registered with different endpoint nodes that are both part of the
> > same device node, the notifier will get either of them. Which subdev
> > match the notifier won't be guaranteed, but that's what the notifier
> > asked for if it contains a device node and not an endpoint node: any
> > subdev corresponding to the device node.
> >
> > In practice notifiers will need to move to endpoint matching if they
> > want to get a particular subdev of a device, and this change allows
> > doing so without mass-patching every driver. It allows a notifier to
> > switch to endpoint nodes, while subdevs still use device nodes and are
> > gradually ported.
> 
> In case a device has two CSI-2 receivers, different IPs, different
> drivers which register different notifiers, and they are connected in
> DTS to a device like adv748x which registers two async
> devices for its two CSI-2 transmitter on endpoints, depending on which
> CSI-2 receiver gets probed first, it matches any of the two CSI-2 Tx.
> The media graph would complete but it won't be what's described in
> dts.
> 
> I agree it's unlikely, and having something like this or what kieran
> did in is better than the current situation, so I'm not pushing this
> back, at all. Just pointing possible reasons why we still don't have
> any solution to this issue in mainline.

Regardless of whether it's likely or not, to support this correctly,
endpoint matching is required, there's no way around that. This change
doesn't introduce any regression, and allows migrating subdevs and
subdevs users independently from each other, so I see no drawback :-)

> >> I'm not sure how that would happen in practice, as the bridge would be
> >> registering the fwnode of the remote device twice, but I think that
> >> was the reason kieran's patch has not been collected or am I
> >> mistaken ?
> >>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>> ---
> >>> This has been compile-tested only. Prabhakar, could you check if it
> >>> fixes your issue ?
> >>>
> >>>  drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++-
> >>>  1 file changed, 41 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> >>> index 8bde33c21ce4..995e5464cba7 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-async.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-async.c
> >>> @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd,
> >>>
> >>>  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> >>>  {
> >>> -	return sd->fwnode == asd->match.fwnode;
> >>> +	struct fwnode_handle *other_fwnode;
> >>> +	struct fwnode_handle *dev_fwnode;
> >>> +	bool asd_fwnode_is_ep;
> >>> +	bool sd_fwnode_is_ep;
> >>> +	const char *name;
> >>> +
> >>> +	/*
> >>> +	 * Both the subdev and the async subdev can provide either an endpoint
> >>> +	 * fwnode or a device fwnode. Start with the simple case of direct
> >>> +	 * fwnode matching.
> >>> +	 */
> >>> +	if (sd->fwnode == asd->match.fwnode)
> >>> +		return true;
> >>> +
> >>> +	/*
> >>> +	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
> >>> +	 * endpoint or a device. If they're of the same type, there's no match.
> >>> +	 */
> >>> +	name = fwnode_get_name(sd->fwnode);
> >>> +	sd_fwnode_is_ep = name && strstarts(name, "endpoint");
> >>> +	name = fwnode_get_name(asd->match.fwnode);
> >>> +	asd_fwnode_is_ep = name && strstarts(name, "endpoint");
> >>> +
> >>> +	if (sd_fwnode_is_ep == asd_fwnode_is_ep)
> >>> +		return false;
> >>> +
> >>> +	/*
> >>> +	 * The sd and asd fwnodes are of different types. Get the device fwnode
> >>> +	 * parent of the endpoint fwnode, and compare it with the other fwnode.
> >>> +	 */
> >>> +	if (sd_fwnode_is_ep) {
> >>> +		dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode);
> >>> +		other_fwnode = asd->match.fwnode;
> >>> +	} else {
> >>> +		dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode);
> >>> +		other_fwnode = sd->fwnode;
> >>> +	}
> >>> +
> >>> +	fwnode_handle_put(dev_fwnode);
> >>> +
> >>> +	return dev_fwnode == other_fwnode;
> >>>  }
> >>>
> >>>  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Niklas Söderlund]

Hi Laurent,

Thanks for your work.

On 2020-03-15 14:55:11 +0200, Laurent Pinchart wrote:
> fwnode matching was designed to match on nodes corresponding to a
> device. Some drivers, however, needed to match on endpoints, and have
> passed endpoint fwnodes to v4l2-async. This works when both the subdev
> and the notifier use the same fwnode types (endpoint or device), but
> makes drivers that use different types incompatible.
> 
> Fix this by extending the fwnode match to handle fwnodes of different
> types. When the types (deduced from the node name) are different,
> retrieve the device fwnode for the side that provides an endpoint
> fwnode, and compare it with the device fwnode provided by the other
> side. This allows interoperability between all drivers, regardless of
> which type of fwnode they use for matching.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

I tested this on R-Car CSI-2 and ADV748x without any regressions. As 
Jacopo already pointed out it's similar to what have been tried before 
and have the potential problem for new transmitters registering multiple 
endpoints (like ADV748x) being used together with older receivers who 
register a single device node in v4l-async. But this do not introduce 
any regressions and is a good first step to move everything to endpoint 
matching.

Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Maybe a info message should be logged if a match is made between 
endpoint and node? It would make it easy to spot if one needs to debug a 
miss match and would be a clear message one driver should be moved to 
endpoint matching. Maybe adding such a log message would count as a 
regression for some.

> ---
> This has been compile-tested only. Prabhakar, could you check if it
> fixes your issue ?
> 
>  drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 8bde33c21ce4..995e5464cba7 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd,
>  
>  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
>  {
> -	return sd->fwnode == asd->match.fwnode;
> +	struct fwnode_handle *other_fwnode;
> +	struct fwnode_handle *dev_fwnode;
> +	bool asd_fwnode_is_ep;
> +	bool sd_fwnode_is_ep;
> +	const char *name;
> +
> +	/*
> +	 * Both the subdev and the async subdev can provide either an endpoint
> +	 * fwnode or a device fwnode. Start with the simple case of direct
> +	 * fwnode matching.
> +	 */
> +	if (sd->fwnode == asd->match.fwnode)
> +		return true;
> +
> +	/*
> +	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
> +	 * endpoint or a device. If they're of the same type, there's no match.
> +	 */
> +	name = fwnode_get_name(sd->fwnode);
> +	sd_fwnode_is_ep = name && strstarts(name, "endpoint");
> +	name = fwnode_get_name(asd->match.fwnode);
> +	asd_fwnode_is_ep = name && strstarts(name, "endpoint");
> +
> +	if (sd_fwnode_is_ep == asd_fwnode_is_ep)
> +		return false;
> +
> +	/*
> +	 * The sd and asd fwnodes are of different types. Get the device fwnode
> +	 * parent of the endpoint fwnode, and compare it with the other fwnode.
> +	 */
> +	if (sd_fwnode_is_ep) {
> +		dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode);
> +		other_fwnode = asd->match.fwnode;
> +	} else {
> +		dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode);
> +		other_fwnode = sd->fwnode;
> +	}
> +
> +	fwnode_handle_put(dev_fwnode);
> +
> +	return dev_fwnode == other_fwnode;
>  }
>  
>  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Laurent Pinchart]

Hi Niklas,

On Mon, Mar 16, 2020 at 10:40:12PM +0100, Niklas Söderlund wrote:
> On 2020-03-15 14:55:11 +0200, Laurent Pinchart wrote:
> > fwnode matching was designed to match on nodes corresponding to a
> > device. Some drivers, however, needed to match on endpoints, and have
> > passed endpoint fwnodes to v4l2-async. This works when both the subdev
> > and the notifier use the same fwnode types (endpoint or device), but
> > makes drivers that use different types incompatible.
> > 
> > Fix this by extending the fwnode match to handle fwnodes of different
> > types. When the types (deduced from the node name) are different,
> > retrieve the device fwnode for the side that provides an endpoint
> > fwnode, and compare it with the device fwnode provided by the other
> > side. This allows interoperability between all drivers, regardless of
> > which type of fwnode they use for matching.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> I tested this on R-Car CSI-2 and ADV748x without any regressions. As 
> Jacopo already pointed out it's similar to what have been tried before 
> and have the potential problem for new transmitters registering multiple 
> endpoints (like ADV748x) being used together with older receivers who 
> register a single device node in v4l-async. But this do not introduce 
> any regressions and is a good first step to move everything to endpoint 
> matching.
> 
> Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Maybe a info message should be logged if a match is made between 
> endpoint and node? It would make it easy to spot if one needs to debug a 
> miss match and would be a clear message one driver should be moved to 
> endpoint matching. Maybe adding such a log message would count as a 
> regression for some.

WARN("FIX YOUR DRIVER TO USE ENDPOINT MATCHING") ? :-)

Jokes aside, something a bit less harsh such as "Matching endpoint with
device node, consider fixing driver %s to use endpoints" wouldn't be a
bad idea.

> > ---
> > This has been compile-tested only. Prabhakar, could you check if it
> > fixes your issue ?
> > 
> >  drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++-
> >  1 file changed, 41 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 8bde33c21ce4..995e5464cba7 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd,
> >  
> >  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> >  {
> > -	return sd->fwnode == asd->match.fwnode;
> > +	struct fwnode_handle *other_fwnode;
> > +	struct fwnode_handle *dev_fwnode;
> > +	bool asd_fwnode_is_ep;
> > +	bool sd_fwnode_is_ep;
> > +	const char *name;
> > +
> > +	/*
> > +	 * Both the subdev and the async subdev can provide either an endpoint
> > +	 * fwnode or a device fwnode. Start with the simple case of direct
> > +	 * fwnode matching.
> > +	 */
> > +	if (sd->fwnode == asd->match.fwnode)
> > +		return true;
> > +
> > +	/*
> > +	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
> > +	 * endpoint or a device. If they're of the same type, there's no match.
> > +	 */
> > +	name = fwnode_get_name(sd->fwnode);
> > +	sd_fwnode_is_ep = name && strstarts(name, "endpoint");
> > +	name = fwnode_get_name(asd->match.fwnode);
> > +	asd_fwnode_is_ep = name && strstarts(name, "endpoint");
> > +
> > +	if (sd_fwnode_is_ep == asd_fwnode_is_ep)
> > +		return false;
> > +
> > +	/*
> > +	 * The sd and asd fwnodes are of different types. Get the device fwnode
> > +	 * parent of the endpoint fwnode, and compare it with the other fwnode.
> > +	 */
> > +	if (sd_fwnode_is_ep) {
> > +		dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode);
> > +		other_fwnode = asd->match.fwnode;
> > +	} else {
> > +		dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode);
> > +		other_fwnode = sd->fwnode;
> > +	}
> > +
> > +	fwnode_handle_put(dev_fwnode);
> > +
> > +	return dev_fwnode == other_fwnode;
> >  }
> >  
> >  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Kieran Bingham]

Hi Laurent,

On 16/03/2020 21:47, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Mon, Mar 16, 2020 at 10:40:12PM +0100, Niklas Söderlund wrote:
>> On 2020-03-15 14:55:11 +0200, Laurent Pinchart wrote:
>>> fwnode matching was designed to match on nodes corresponding to a
>>> device. Some drivers, however, needed to match on endpoints, and have
>>> passed endpoint fwnodes to v4l2-async. This works when both the subdev
>>> and the notifier use the same fwnode types (endpoint or device), but
>>> makes drivers that use different types incompatible.
>>>
>>> Fix this by extending the fwnode match to handle fwnodes of different
>>> types. When the types (deduced from the node name) are different,
>>> retrieve the device fwnode for the side that provides an endpoint
>>> fwnode, and compare it with the device fwnode provided by the other
>>> side. This allows interoperability between all drivers, regardless of
>>> which type of fwnode they use for matching.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>
>> I tested this on R-Car CSI-2 and ADV748x without any regressions. As 
>> Jacopo already pointed out it's similar to what have been tried before 

This was the patch that I had believed was accepted, but ended up stuck
in Sakari's tree:

https://git.linuxtv.org/sailus/media_tree.git/commit/?h=fwnode-const&id=35c32d99b2c3f5086b911ec817926de9b7bc3b41

(it's already a little bit-rotted though)


>> and have the potential problem for new transmitters registering multiple 
>> endpoints (like ADV748x) being used together with older receivers who 
>> register a single device node in v4l-async. But this do not introduce 

So if an 'old' receiver wants to use the 'new' features, it must upgrade
to endpoint matching.

I think that's fine.

>> any regressions and is a good first step to move everything to endpoint 
>> matching.
>>
>> Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>
>> Maybe a info message should be logged if a match is made between 
>> endpoint and node? It would make it easy to spot if one needs to debug a 
>> miss match and would be a clear message one driver should be moved to 
>> endpoint matching. Maybe adding such a log message would count as a 
>> regression for some.
> 
> WARN("FIX YOUR DRIVER TO USE ENDPOINT MATCHING") ? :-)

Indeed, a notification that they need to update their matching would be
useful in that scenario.

I believe we need to move forward with this somehow, as we have Xilinx
trying to use MAX9286 with Xilinx drivers, (endpoint matching subdev
with dev node matching receiver) and Renesas trying to use non-endpoint
subdevices against an endpoint matched RCar-VIN ...?


> Jokes aside, something a bit less harsh such as "Matching endpoint with
> device node, consider fixing driver %s to use endpoints" wouldn't be a
> bad idea.

Yes, Is there anything else we can do? Even if we 'started' converting
other receivers to match on endpoints, it would take time - so I think
an intermediate stage like this is still very useful.

Of course this patch also lets us push the updates back to those who
care about those drivers too ...

>>> ---
>>> This has been compile-tested only. Prabhakar, could you check if it
>>> fixes your issue ?
>>>
>>>  drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++-
>>>  1 file changed, 41 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>>> index 8bde33c21ce4..995e5464cba7 100644
>>> --- a/drivers/media/v4l2-core/v4l2-async.c
>>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>>> @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd,
>>>  
>>>  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
>>>  {
>>> -	return sd->fwnode == asd->match.fwnode;
>>> +	struct fwnode_handle *other_fwnode;
>>> +	struct fwnode_handle *dev_fwnode;
>>> +	bool asd_fwnode_is_ep;
>>> +	bool sd_fwnode_is_ep;
>>> +	const char *name;
>>> +
>>> +	/*
>>> +	 * Both the subdev and the async subdev can provide either an endpoint
>>> +	 * fwnode or a device fwnode. Start with the simple case of direct
>>> +	 * fwnode matching.
>>> +	 */
>>> +	if (sd->fwnode == asd->match.fwnode)
>>> +		return true;
>>> +
>>> +	/*
>>> +	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
>>> +	 * endpoint or a device. If they're of the same type, there's no match.
>>> +	 */
>>> +	name = fwnode_get_name(sd->fwnode);
>>> +	sd_fwnode_is_ep = name && strstarts(name, "endpoint");
>>> +	name = fwnode_get_name(asd->match.fwnode);
>>> +	asd_fwnode_is_ep = name && strstarts(name, "endpoint");
>>> +
>>> +	if (sd_fwnode_is_ep == asd_fwnode_is_ep)
>>> +		return false;

Ok, so this looks like a good safety check for edge cases which would
potentially have got through in my version.

>>> +
>>> +	/*
>>> +	 * The sd and asd fwnodes are of different types. Get the device fwnode
>>> +	 * parent of the endpoint fwnode, and compare it with the other fwnode.
>>> +	 */
>>> +	if (sd_fwnode_is_ep) {
>>> +		dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode);
>>> +		other_fwnode = asd->match.fwnode;
>>> +	} else {
>>> +		dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode);
>>> +		other_fwnode = sd->fwnode;
>>> +	}
>>> +
>>> +	fwnode_handle_put(dev_fwnode);

It seems in my implementation these got leaked too :-)

I'm sold. This one is better than the old version I had.

Hopefully we can get this moving so that we can progress towards
endpoint matching throughout next.

(Ideally with a warning to convert non-endpoint matching drivers...)

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


>>> +
>>> +	return dev_fwnode == other_fwnode;
>>>  }
>>>  
>>>  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> 

Re: [PATCH] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Sakari Ailus]

Hi Laurent,

Thanks for the patch.

On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote:
> fwnode matching was designed to match on nodes corresponding to a
> device. Some drivers, however, needed to match on endpoints, and have
> passed endpoint fwnodes to v4l2-async. This works when both the subdev
> and the notifier use the same fwnode types (endpoint or device), but
> makes drivers that use different types incompatible.
> 
> Fix this by extending the fwnode match to handle fwnodes of different
> types. When the types (deduced from the node name) are different,
> retrieve the device fwnode for the side that provides an endpoint
> fwnode, and compare it with the device fwnode provided by the other
> side. This allows interoperability between all drivers, regardless of
> which type of fwnode they use for matching.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> This has been compile-tested only. Prabhakar, could you check if it
> fixes your issue ?
> 
>  drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 8bde33c21ce4..995e5464cba7 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd,
>  
>  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
>  {
> -	return sd->fwnode == asd->match.fwnode;
> +	struct fwnode_handle *other_fwnode;
> +	struct fwnode_handle *dev_fwnode;
> +	bool asd_fwnode_is_ep;
> +	bool sd_fwnode_is_ep;
> +	const char *name;
> +
> +	/*
> +	 * Both the subdev and the async subdev can provide either an endpoint
> +	 * fwnode or a device fwnode. Start with the simple case of direct
> +	 * fwnode matching.
> +	 */
> +	if (sd->fwnode == asd->match.fwnode)
> +		return true;
> +
> +	/*
> +	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
> +	 * endpoint or a device. If they're of the same type, there's no match.
> +	 */
> +	name = fwnode_get_name(sd->fwnode);
> +	sd_fwnode_is_ep = name && strstarts(name, "endpoint");
> +	name = fwnode_get_name(asd->match.fwnode);
> +	asd_fwnode_is_ep = name && strstarts(name, "endpoint");

Apart from the fact that you're parsing graph node names here, this looks
good.

How about checking instead that calling
fwnode_graph_get_remote_endpoint(fwnode_graph_get_remote_endpoint()) yields
the same node? That would ensure you're dealing with endpoint nodes without
explicitly parsing the graph in any way.

Just remember to drop the references.

> +
> +	if (sd_fwnode_is_ep == asd_fwnode_is_ep)
> +		return false;
> +
> +	/*
> +	 * The sd and asd fwnodes are of different types. Get the device fwnode
> +	 * parent of the endpoint fwnode, and compare it with the other fwnode.
> +	 */
> +	if (sd_fwnode_is_ep) {
> +		dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode);
> +		other_fwnode = asd->match.fwnode;
> +	} else {
> +		dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode);
> +		other_fwnode = sd->fwnode;
> +	}
> +
> +	fwnode_handle_put(dev_fwnode);
> +
> +	return dev_fwnode == other_fwnode;
>  }
>  
>  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)

-- 
Kind regards,

Sakari Ailus

Re: [PATCH] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Laurent Pinchart]

Hi Sakari,

On Tue, Mar 17, 2020 at 02:44:56PM +0200, Sakari Ailus wrote:
> On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote:
> > fwnode matching was designed to match on nodes corresponding to a
> > device. Some drivers, however, needed to match on endpoints, and have
> > passed endpoint fwnodes to v4l2-async. This works when both the subdev
> > and the notifier use the same fwnode types (endpoint or device), but
> > makes drivers that use different types incompatible.
> > 
> > Fix this by extending the fwnode match to handle fwnodes of different
> > types. When the types (deduced from the node name) are different,
> > retrieve the device fwnode for the side that provides an endpoint
> > fwnode, and compare it with the device fwnode provided by the other
> > side. This allows interoperability between all drivers, regardless of
> > which type of fwnode they use for matching.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > This has been compile-tested only. Prabhakar, could you check if it
> > fixes your issue ?
> > 
> >  drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++-
> >  1 file changed, 41 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 8bde33c21ce4..995e5464cba7 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd,
> >  
> >  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> >  {
> > -	return sd->fwnode == asd->match.fwnode;
> > +	struct fwnode_handle *other_fwnode;
> > +	struct fwnode_handle *dev_fwnode;
> > +	bool asd_fwnode_is_ep;
> > +	bool sd_fwnode_is_ep;
> > +	const char *name;
> > +
> > +	/*
> > +	 * Both the subdev and the async subdev can provide either an endpoint
> > +	 * fwnode or a device fwnode. Start with the simple case of direct
> > +	 * fwnode matching.
> > +	 */
> > +	if (sd->fwnode == asd->match.fwnode)
> > +		return true;
> > +
> > +	/*
> > +	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
> > +	 * endpoint or a device. If they're of the same type, there's no match.
> > +	 */
> > +	name = fwnode_get_name(sd->fwnode);
> > +	sd_fwnode_is_ep = name && strstarts(name, "endpoint");
> > +	name = fwnode_get_name(asd->match.fwnode);
> > +	asd_fwnode_is_ep = name && strstarts(name, "endpoint");
> 
> Apart from the fact that you're parsing graph node names here, this looks
> good.
> 
> How about checking instead that calling
> fwnode_graph_get_remote_endpoint(fwnode_graph_get_remote_endpoint()) yields
> the same node? That would ensure you're dealing with endpoint nodes without
> explicitly parsing the graph in any way.

Would it be simpler to check for the presence of an endpoint property ?

> Just remember to drop the references.
> 
> > +
> > +	if (sd_fwnode_is_ep == asd_fwnode_is_ep)
> > +		return false;
> > +
> > +	/*
> > +	 * The sd and asd fwnodes are of different types. Get the device fwnode
> > +	 * parent of the endpoint fwnode, and compare it with the other fwnode.
> > +	 */
> > +	if (sd_fwnode_is_ep) {
> > +		dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode);
> > +		other_fwnode = asd->match.fwnode;
> > +	} else {
> > +		dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode);
> > +		other_fwnode = sd->fwnode;
> > +	}
> > +
> > +	fwnode_handle_put(dev_fwnode);
> > +
> > +	return dev_fwnode == other_fwnode;
> >  }
> >  
> >  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Laurent Pinchart]

Hi Kieran,

On Tue, Mar 17, 2020 at 12:33:07PM +0000, Kieran Bingham wrote:
> On 16/03/2020 21:47, Laurent Pinchart wrote:
> > On Mon, Mar 16, 2020 at 10:40:12PM +0100, Niklas Söderlund wrote:
> >> On 2020-03-15 14:55:11 +0200, Laurent Pinchart wrote:
> >>> fwnode matching was designed to match on nodes corresponding to a
> >>> device. Some drivers, however, needed to match on endpoints, and have
> >>> passed endpoint fwnodes to v4l2-async. This works when both the subdev
> >>> and the notifier use the same fwnode types (endpoint or device), but
> >>> makes drivers that use different types incompatible.
> >>>
> >>> Fix this by extending the fwnode match to handle fwnodes of different
> >>> types. When the types (deduced from the node name) are different,
> >>> retrieve the device fwnode for the side that provides an endpoint
> >>> fwnode, and compare it with the device fwnode provided by the other
> >>> side. This allows interoperability between all drivers, regardless of
> >>> which type of fwnode they use for matching.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>
> >> I tested this on R-Car CSI-2 and ADV748x without any regressions. As 
> >> Jacopo already pointed out it's similar to what have been tried before 
> 
> This was the patch that I had believed was accepted, but ended up stuck
> in Sakari's tree:
> 
> https://git.linuxtv.org/sailus/media_tree.git/commit/?h=fwnode-const&id=35c32d99b2c3f5086b911ec817926de9b7bc3b41
> 
> (it's already a little bit-rotted though)

Yes, I noticed that. I don't mind dropping this patch if you rebase
yours, as long as we merge a fix :-)

> >> and have the potential problem for new transmitters registering multiple 
> >> endpoints (like ADV748x) being used together with older receivers who 
> >> register a single device node in v4l-async. But this do not introduce 
> 
> So if an 'old' receiver wants to use the 'new' features, it must upgrade
> to endpoint matching.
> 
> I think that's fine.

Yes that's the idea. It will however not have to upgrade all the subdev
drivers it uses at the same time.

> >> any regressions and is a good first step to move everything to endpoint 
> >> matching.
> >>
> >> Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>
> >> Maybe a info message should be logged if a match is made between 
> >> endpoint and node? It would make it easy to spot if one needs to debug a 
> >> miss match and would be a clear message one driver should be moved to 
> >> endpoint matching. Maybe adding such a log message would count as a 
> >> regression for some.
> > 
> > WARN("FIX YOUR DRIVER TO USE ENDPOINT MATCHING") ? :-)
> 
> Indeed, a notification that they need to update their matching would be
> useful in that scenario.
> 
> I believe we need to move forward with this somehow, as we have Xilinx
> trying to use MAX9286 with Xilinx drivers, (endpoint matching subdev
> with dev node matching receiver) and Renesas trying to use non-endpoint
> subdevices against an endpoint matched RCar-VIN ...?
> 
> > Jokes aside, something a bit less harsh such as "Matching endpoint with
> > device node, consider fixing driver %s to use endpoints" wouldn't be a
> > bad idea.
> 
> Yes, Is there anything else we can do? Even if we 'started' converting
> other receivers to match on endpoints, it would take time - so I think
> an intermediate stage like this is still very useful.
> 
> Of course this patch also lets us push the updates back to those who
> care about those drivers too ...

Exactly :-)

> >>> ---
> >>> This has been compile-tested only. Prabhakar, could you check if it
> >>> fixes your issue ?
> >>>
> >>>  drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++-
> >>>  1 file changed, 41 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> >>> index 8bde33c21ce4..995e5464cba7 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-async.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-async.c
> >>> @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd,
> >>>  
> >>>  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> >>>  {
> >>> -	return sd->fwnode == asd->match.fwnode;
> >>> +	struct fwnode_handle *other_fwnode;
> >>> +	struct fwnode_handle *dev_fwnode;
> >>> +	bool asd_fwnode_is_ep;
> >>> +	bool sd_fwnode_is_ep;
> >>> +	const char *name;
> >>> +
> >>> +	/*
> >>> +	 * Both the subdev and the async subdev can provide either an endpoint
> >>> +	 * fwnode or a device fwnode. Start with the simple case of direct
> >>> +	 * fwnode matching.
> >>> +	 */
> >>> +	if (sd->fwnode == asd->match.fwnode)
> >>> +		return true;
> >>> +
> >>> +	/*
> >>> +	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
> >>> +	 * endpoint or a device. If they're of the same type, there's no match.
> >>> +	 */
> >>> +	name = fwnode_get_name(sd->fwnode);
> >>> +	sd_fwnode_is_ep = name && strstarts(name, "endpoint");
> >>> +	name = fwnode_get_name(asd->match.fwnode);
> >>> +	asd_fwnode_is_ep = name && strstarts(name, "endpoint");
> >>> +
> >>> +	if (sd_fwnode_is_ep == asd_fwnode_is_ep)
> >>> +		return false;
> 
> Ok, so this looks like a good safety check for edge cases which would
> potentially have got through in my version.
> 
> >>> +
> >>> +	/*
> >>> +	 * The sd and asd fwnodes are of different types. Get the device fwnode
> >>> +	 * parent of the endpoint fwnode, and compare it with the other fwnode.
> >>> +	 */
> >>> +	if (sd_fwnode_is_ep) {
> >>> +		dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode);
> >>> +		other_fwnode = asd->match.fwnode;
> >>> +	} else {
> >>> +		dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode);
> >>> +		other_fwnode = sd->fwnode;
> >>> +	}
> >>> +
> >>> +	fwnode_handle_put(dev_fwnode);
> 
> It seems in my implementation these got leaked too :-)
> 
> I'm sold. This one is better than the old version I had.
> 
> Hopefully we can get this moving so that we can progress towards
> endpoint matching throughout next.
> 
> (Ideally with a warning to convert non-endpoint matching drivers...)
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Thank you. I'll add a warning and agree with Sakari on the best method
to check if a node is an endpoint node, and will then resubmit.

> >>> +
> >>> +	return dev_fwnode == other_fwnode;
> >>>  }
> >>>  
> >>>  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Laurent Pinchart]

Hi Sakari,

On Wed, Mar 18, 2020 at 01:04:32AM +0200, Laurent Pinchart wrote:
> On Tue, Mar 17, 2020 at 02:44:56PM +0200, Sakari Ailus wrote:
> > On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote:
> > > fwnode matching was designed to match on nodes corresponding to a
> > > device. Some drivers, however, needed to match on endpoints, and have
> > > passed endpoint fwnodes to v4l2-async. This works when both the subdev
> > > and the notifier use the same fwnode types (endpoint or device), but
> > > makes drivers that use different types incompatible.
> > > 
> > > Fix this by extending the fwnode match to handle fwnodes of different
> > > types. When the types (deduced from the node name) are different,
> > > retrieve the device fwnode for the side that provides an endpoint
> > > fwnode, and compare it with the device fwnode provided by the other
> > > side. This allows interoperability between all drivers, regardless of
> > > which type of fwnode they use for matching.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > ---
> > > This has been compile-tested only. Prabhakar, could you check if it
> > > fixes your issue ?
> > > 
> > >  drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++-
> > >  1 file changed, 41 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > index 8bde33c21ce4..995e5464cba7 100644
> > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd,
> > >  
> > >  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> > >  {
> > > -	return sd->fwnode == asd->match.fwnode;
> > > +	struct fwnode_handle *other_fwnode;
> > > +	struct fwnode_handle *dev_fwnode;
> > > +	bool asd_fwnode_is_ep;
> > > +	bool sd_fwnode_is_ep;
> > > +	const char *name;
> > > +
> > > +	/*
> > > +	 * Both the subdev and the async subdev can provide either an endpoint
> > > +	 * fwnode or a device fwnode. Start with the simple case of direct
> > > +	 * fwnode matching.
> > > +	 */
> > > +	if (sd->fwnode == asd->match.fwnode)
> > > +		return true;
> > > +
> > > +	/*
> > > +	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
> > > +	 * endpoint or a device. If they're of the same type, there's no match.
> > > +	 */
> > > +	name = fwnode_get_name(sd->fwnode);
> > > +	sd_fwnode_is_ep = name && strstarts(name, "endpoint");
> > > +	name = fwnode_get_name(asd->match.fwnode);
> > > +	asd_fwnode_is_ep = name && strstarts(name, "endpoint");
> > 
> > Apart from the fact that you're parsing graph node names here, this looks
> > good.

And why is that an issue btw ? the ACPI fwnode ops seem to provide a
.get_name() operation, would it return the ACPI bus ID here ?

> > How about checking instead that calling
> > fwnode_graph_get_remote_endpoint(fwnode_graph_get_remote_endpoint()) yields
> > the same node? That would ensure you're dealing with endpoint nodes without
> > explicitly parsing the graph in any way.
> 
> Would it be simpler to check for the presence of an endpoint property ?
> 
> > Just remember to drop the references.
> > 
> > > +
> > > +	if (sd_fwnode_is_ep == asd_fwnode_is_ep)
> > > +		return false;
> > > +
> > > +	/*
> > > +	 * The sd and asd fwnodes are of different types. Get the device fwnode
> > > +	 * parent of the endpoint fwnode, and compare it with the other fwnode.
> > > +	 */
> > > +	if (sd_fwnode_is_ep) {
> > > +		dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode);
> > > +		other_fwnode = asd->match.fwnode;
> > > +	} else {
> > > +		dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode);
> > > +		other_fwnode = sd->fwnode;
> > > +	}
> > > +
> > > +	fwnode_handle_put(dev_fwnode);
> > > +
> > > +	return dev_fwnode == other_fwnode;
> > >  }
> > >  
> > >  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Sakari Ailus]

Hi Laurent,

On Wed, Mar 18, 2020 at 02:17:26AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wed, Mar 18, 2020 at 01:04:32AM +0200, Laurent Pinchart wrote:
> > On Tue, Mar 17, 2020 at 02:44:56PM +0200, Sakari Ailus wrote:
> > > On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote:
> > > > fwnode matching was designed to match on nodes corresponding to a
> > > > device. Some drivers, however, needed to match on endpoints, and have
> > > > passed endpoint fwnodes to v4l2-async. This works when both the subdev
> > > > and the notifier use the same fwnode types (endpoint or device), but
> > > > makes drivers that use different types incompatible.
> > > > 
> > > > Fix this by extending the fwnode match to handle fwnodes of different
> > > > types. When the types (deduced from the node name) are different,
> > > > retrieve the device fwnode for the side that provides an endpoint
> > > > fwnode, and compare it with the device fwnode provided by the other
> > > > side. This allows interoperability between all drivers, regardless of
> > > > which type of fwnode they use for matching.
> > > > 
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > ---
> > > > This has been compile-tested only. Prabhakar, could you check if it
> > > > fixes your issue ?
> > > > 
> > > >  drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++-
> > > >  1 file changed, 41 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > > index 8bde33c21ce4..995e5464cba7 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > > @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd,
> > > >  
> > > >  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> > > >  {
> > > > -	return sd->fwnode == asd->match.fwnode;
> > > > +	struct fwnode_handle *other_fwnode;
> > > > +	struct fwnode_handle *dev_fwnode;
> > > > +	bool asd_fwnode_is_ep;
> > > > +	bool sd_fwnode_is_ep;
> > > > +	const char *name;
> > > > +
> > > > +	/*
> > > > +	 * Both the subdev and the async subdev can provide either an endpoint
> > > > +	 * fwnode or a device fwnode. Start with the simple case of direct
> > > > +	 * fwnode matching.
> > > > +	 */
> > > > +	if (sd->fwnode == asd->match.fwnode)
> > > > +		return true;
> > > > +
> > > > +	/*
> > > > +	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
> > > > +	 * endpoint or a device. If they're of the same type, there's no match.
> > > > +	 */
> > > > +	name = fwnode_get_name(sd->fwnode);
> > > > +	sd_fwnode_is_ep = name && strstarts(name, "endpoint");
> > > > +	name = fwnode_get_name(asd->match.fwnode);
> > > > +	asd_fwnode_is_ep = name && strstarts(name, "endpoint");
> > > 
> > > Apart from the fact that you're parsing graph node names here, this looks
> > > good.
> 
> And why is that an issue btw ? the ACPI fwnode ops seem to provide a
> .get_name() operation, would it return the ACPI bus ID here ?

I'd really prefer not to do graph parsing outside the main parser(s), OF,
ACPI and property frameworks.

Just for an example, the v4l2_fwnode_link_parse() was broken for ACPI for a
long time just because it did not use the graph parsing functions, but
implemented graph parsing on its own.

> 
> > > How about checking instead that calling
> > > fwnode_graph_get_remote_endpoint(fwnode_graph_get_remote_endpoint()) yields
> > > the same node? That would ensure you're dealing with endpoint nodes without
> > > explicitly parsing the graph in any way.
> > 
> > Would it be simpler to check for the presence of an endpoint property ?

There's no endpoint property, apart from an old ACPI definition.

There are differences in the implementations and this is not the best place
to try to take them all into account.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Laurent Pinchart]

Hi Sakari,

On Wed, Mar 18, 2020 at 09:52:25AM +0200, Sakari Ailus wrote:
> On Wed, Mar 18, 2020 at 02:17:26AM +0200, Laurent Pinchart wrote:
> > On Wed, Mar 18, 2020 at 01:04:32AM +0200, Laurent Pinchart wrote:
> >> On Tue, Mar 17, 2020 at 02:44:56PM +0200, Sakari Ailus wrote:
> >>> On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote:
> >>>> fwnode matching was designed to match on nodes corresponding to a
> >>>> device. Some drivers, however, needed to match on endpoints, and have
> >>>> passed endpoint fwnodes to v4l2-async. This works when both the subdev
> >>>> and the notifier use the same fwnode types (endpoint or device), but
> >>>> makes drivers that use different types incompatible.
> >>>> 
> >>>> Fix this by extending the fwnode match to handle fwnodes of different
> >>>> types. When the types (deduced from the node name) are different,
> >>>> retrieve the device fwnode for the side that provides an endpoint
> >>>> fwnode, and compare it with the device fwnode provided by the other
> >>>> side. This allows interoperability between all drivers, regardless of
> >>>> which type of fwnode they use for matching.
> >>>> 
> >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>>> ---
> >>>> This has been compile-tested only. Prabhakar, could you check if it
> >>>> fixes your issue ?
> >>>> 
> >>>>  drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++-
> >>>>  1 file changed, 41 insertions(+), 1 deletion(-)
> >>>> 
> >>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> >>>> index 8bde33c21ce4..995e5464cba7 100644
> >>>> --- a/drivers/media/v4l2-core/v4l2-async.c
> >>>> +++ b/drivers/media/v4l2-core/v4l2-async.c
> >>>> @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd,
> >>>>  
> >>>>  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> >>>>  {
> >>>> -	return sd->fwnode == asd->match.fwnode;
> >>>> +	struct fwnode_handle *other_fwnode;
> >>>> +	struct fwnode_handle *dev_fwnode;
> >>>> +	bool asd_fwnode_is_ep;
> >>>> +	bool sd_fwnode_is_ep;
> >>>> +	const char *name;
> >>>> +
> >>>> +	/*
> >>>> +	 * Both the subdev and the async subdev can provide either an endpoint
> >>>> +	 * fwnode or a device fwnode. Start with the simple case of direct
> >>>> +	 * fwnode matching.
> >>>> +	 */
> >>>> +	if (sd->fwnode == asd->match.fwnode)
> >>>> +		return true;
> >>>> +
> >>>> +	/*
> >>>> +	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
> >>>> +	 * endpoint or a device. If they're of the same type, there's no match.
> >>>> +	 */
> >>>> +	name = fwnode_get_name(sd->fwnode);
> >>>> +	sd_fwnode_is_ep = name && strstarts(name, "endpoint");
> >>>> +	name = fwnode_get_name(asd->match.fwnode);
> >>>> +	asd_fwnode_is_ep = name && strstarts(name, "endpoint");
> >>> 
> >>> Apart from the fact that you're parsing graph node names here, this looks
> >>> good.
> > 
> > And why is that an issue btw ? the ACPI fwnode ops seem to provide a
> > .get_name() operation, would it return the ACPI bus ID here ?
> 
> I'd really prefer not to do graph parsing outside the main parser(s), OF,
> ACPI and property frameworks.
> 
> Just for an example, the v4l2_fwnode_link_parse() was broken for ACPI for a
> long time just because it did not use the graph parsing functions, but
> implemented graph parsing on its own.
> 
> >>> How about checking instead that calling
> >>> fwnode_graph_get_remote_endpoint(fwnode_graph_get_remote_endpoint()) yields
> >>> the same node? That would ensure you're dealing with endpoint nodes without
> >>> explicitly parsing the graph in any way.
> >> 
> >> Would it be simpler to check for the presence of an endpoint property ?
> 
> There's no endpoint property, apart from an old ACPI definition.

There isn't ? How does it work on ACPI then ?
acpi_graph_get_remote_endpoint() starts with

	ret = acpi_node_get_property_reference(__fwnode, "remote-endpoint", 0,
					       &args);

> There are differences in the implementations and this is not the best place
> to try to take them all into account.

OK, but in that case I think we need an fwnode_graph_is_endpoint().

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Sakari Ailus]

Hi Laurent,

On Wed, Mar 18, 2020 at 01:22:16PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wed, Mar 18, 2020 at 09:52:25AM +0200, Sakari Ailus wrote:
> > On Wed, Mar 18, 2020 at 02:17:26AM +0200, Laurent Pinchart wrote:
> > > On Wed, Mar 18, 2020 at 01:04:32AM +0200, Laurent Pinchart wrote:
> > >> On Tue, Mar 17, 2020 at 02:44:56PM +0200, Sakari Ailus wrote:
> > >>> On Sun, Mar 15, 2020 at 02:55:11PM +0200, Laurent Pinchart wrote:
> > >>>> fwnode matching was designed to match on nodes corresponding to a
> > >>>> device. Some drivers, however, needed to match on endpoints, and have
> > >>>> passed endpoint fwnodes to v4l2-async. This works when both the subdev
> > >>>> and the notifier use the same fwnode types (endpoint or device), but
> > >>>> makes drivers that use different types incompatible.
> > >>>> 
> > >>>> Fix this by extending the fwnode match to handle fwnodes of different
> > >>>> types. When the types (deduced from the node name) are different,
> > >>>> retrieve the device fwnode for the side that provides an endpoint
> > >>>> fwnode, and compare it with the device fwnode provided by the other
> > >>>> side. This allows interoperability between all drivers, regardless of
> > >>>> which type of fwnode they use for matching.
> > >>>> 
> > >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > >>>> ---
> > >>>> This has been compile-tested only. Prabhakar, could you check if it
> > >>>> fixes your issue ?
> > >>>> 
> > >>>>  drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++-
> > >>>>  1 file changed, 41 insertions(+), 1 deletion(-)
> > >>>> 
> > >>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > >>>> index 8bde33c21ce4..995e5464cba7 100644
> > >>>> --- a/drivers/media/v4l2-core/v4l2-async.c
> > >>>> +++ b/drivers/media/v4l2-core/v4l2-async.c
> > >>>> @@ -71,7 +71,47 @@ static bool match_devname(struct v4l2_subdev *sd,
> > >>>>  
> > >>>>  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> > >>>>  {
> > >>>> -	return sd->fwnode == asd->match.fwnode;
> > >>>> +	struct fwnode_handle *other_fwnode;
> > >>>> +	struct fwnode_handle *dev_fwnode;
> > >>>> +	bool asd_fwnode_is_ep;
> > >>>> +	bool sd_fwnode_is_ep;
> > >>>> +	const char *name;
> > >>>> +
> > >>>> +	/*
> > >>>> +	 * Both the subdev and the async subdev can provide either an endpoint
> > >>>> +	 * fwnode or a device fwnode. Start with the simple case of direct
> > >>>> +	 * fwnode matching.
> > >>>> +	 */
> > >>>> +	if (sd->fwnode == asd->match.fwnode)
> > >>>> +		return true;
> > >>>> +
> > >>>> +	/*
> > >>>> +	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
> > >>>> +	 * endpoint or a device. If they're of the same type, there's no match.
> > >>>> +	 */
> > >>>> +	name = fwnode_get_name(sd->fwnode);
> > >>>> +	sd_fwnode_is_ep = name && strstarts(name, "endpoint");
> > >>>> +	name = fwnode_get_name(asd->match.fwnode);
> > >>>> +	asd_fwnode_is_ep = name && strstarts(name, "endpoint");
> > >>> 
> > >>> Apart from the fact that you're parsing graph node names here, this looks
> > >>> good.
> > > 
> > > And why is that an issue btw ? the ACPI fwnode ops seem to provide a
> > > .get_name() operation, would it return the ACPI bus ID here ?
> > 
> > I'd really prefer not to do graph parsing outside the main parser(s), OF,
> > ACPI and property frameworks.
> > 
> > Just for an example, the v4l2_fwnode_link_parse() was broken for ACPI for a
> > long time just because it did not use the graph parsing functions, but
> > implemented graph parsing on its own.
> > 
> > >>> How about checking instead that calling
> > >>> fwnode_graph_get_remote_endpoint(fwnode_graph_get_remote_endpoint()) yields
> > >>> the same node? That would ensure you're dealing with endpoint nodes without
> > >>> explicitly parsing the graph in any way.
> > >> 
> > >> Would it be simpler to check for the presence of an endpoint property ?
> > 
> > There's no endpoint property, apart from an old ACPI definition.
> 
> There isn't ? How does it work on ACPI then ?
> acpi_graph_get_remote_endpoint() starts with
> 
> 	ret = acpi_node_get_property_reference(__fwnode, "remote-endpoint", 0,
> 					       &args);

The remote-endpoint property is used in ACPI, too, yes. But the question
was about a property named "endpoint".

> 
> > There are differences in the implementations and this is not the best place
> > to try to take them all into account.
> 
> OK, but in that case I think we need an fwnode_graph_is_endpoint().

Thinking about this --- could you check fwnode_graph_get_remote_endpoint()
returns non-NULL, and then put the remote? That would be more simple as you
are only interested in knowing you're dealing with an endpoint. I don't
object having a little helper for this though.

-- 
Regards,

Sakari Ailus