[PATCH v3 0/3] media: v4l2-async: Accept endpoints and devices for fwnode matching

[PATCH v3 0/3] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Laurent Pinchart]

Hello,

This patch series is the third version of fwnode endpoint matching
support in v4l2-async. The secon version can be found at [1].

Compared to v2, review comments have been incorporated, patch 4/4 got
squashed in 1/4 (now 1/3), and the code has been rebased on top of
v5.8-rc1.

Sakari, is they way that patch 1/3 determines if a fwnode is an endpoint
acceptable for you (and for ACPI) ?

[1] https://lore.kernel.org/linux-media/20200318002507.30336-1-laurent.pinchart+renesas@ideasonboard.com

Laurent Pinchart (3):
  media: v4l2-async: Accept endpoints and devices for fwnode matching
  media: v4l2-async: Pass notifier pointer to match functions
  media: v4l2-async: Log message in case of heterogeneous fwnode match

 drivers/media/v4l2-core/v4l2-async.c | 85 +++++++++++++++++++++++++---
 1 file changed, 77 insertions(+), 8 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v3 1/3] 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 presence of remote endpoints)
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>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
Changes since v2:

- Add comment to explain that we're matching connecting endpoints
- Don't check fwnode name to detect endpoint
---
 drivers/media/v4l2-core/v4l2-async.c | 45 +++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 8bde33c21ce4..f82e0a32647d 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -71,7 +71,50 @@ 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;
+
+	/*
+	 * 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.
+	 * Technically speaking this checks if the nodes refer to a connected
+	 * endpoint, which is the simplest check that works for both OF and
+	 * ACPI. This won't make a difference, as drivers should not try to
+	 * match unconnected endpoints.
+	 */
+	sd_fwnode_is_ep = fwnode_property_present(sd->fwnode,
+						  "remote-endpoint");
+	asd_fwnode_is_ep = fwnode_property_present(asd->match.fwnode,
+						   "remote-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

[PATCH v3 2/3] media: v4l2-async: Pass notifier pointer to match functions

[Laurent Pinchart]

The notifier is useful to match functions to access information about
the device matching a subdev. This will be used to print messages using
the correct struct device and driver name.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/v4l2-core/v4l2-async.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index f82e0a32647d..1599f89c2436 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -50,7 +50,8 @@ static int v4l2_async_notifier_call_complete(struct v4l2_async_notifier *n)
 	return n->ops->complete(n);
 }
 
-static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
+static bool match_i2c(struct v4l2_async_notifier *notifier,
+		      struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
 #if IS_ENABLED(CONFIG_I2C)
 	struct i2c_client *client = i2c_verify_client(sd->dev);
@@ -63,13 +64,14 @@ static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 #endif
 }
 
-static bool match_devname(struct v4l2_subdev *sd,
-			  struct v4l2_async_subdev *asd)
+static bool match_devname(struct v4l2_async_notifier *notifier,
+			  struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
 	return !strcmp(asd->match.device_name, dev_name(sd->dev));
 }
 
-static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
+static bool match_fwnode(struct v4l2_async_notifier *notifier,
+			 struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
 	struct fwnode_handle *other_fwnode;
 	struct fwnode_handle *dev_fwnode;
@@ -117,7 +119,8 @@ static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 	return dev_fwnode == other_fwnode;
 }
 
-static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
+static bool match_custom(struct v4l2_async_notifier *notifier,
+			 struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
 {
 	if (!asd->match.custom.match)
 		/* Match always */
@@ -134,7 +137,8 @@ static struct v4l2_async_subdev *
 v4l2_async_find_match(struct v4l2_async_notifier *notifier,
 		      struct v4l2_subdev *sd)
 {
-	bool (*match)(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd);
+	bool (*match)(struct v4l2_async_notifier *notifier,
+		      struct v4l2_subdev *sd, struct v4l2_async_subdev *asd);
 	struct v4l2_async_subdev *asd;
 
 	list_for_each_entry(asd, &notifier->waiting, list) {
@@ -159,7 +163,7 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier,
 		}
 
 		/* match cannot be NULL here */
-		if (match(sd, asd))
+		if (match(notifier, sd, asd))
 			return asd;
 	}
 
-- 
Regards,

Laurent Pinchart

[PATCH v3 3/3] media: v4l2-async: Log message in case of heterogeneous fwnode match

[Laurent Pinchart]

When a notifier supplies a device fwnode and a subdev supplies an
endpoint fwnode, incorrect matches may occur if multiple subdevs
correspond to the same device fwnode. This can't be handled
transparently in the framework, and requires the notifier to switch to
endpoint fwnodes. Log a message to notify of this problem. A second
message is added to help accelerating the transition to endpoint
matching.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
Changes since v2:

- Use dev_warn() and dev_notice()
- Fix typo
---
 drivers/media/v4l2-core/v4l2-async.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 1599f89c2436..242e0d93e4f0 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -77,6 +77,7 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
 	struct fwnode_handle *dev_fwnode;
 	bool asd_fwnode_is_ep;
 	bool sd_fwnode_is_ep;
+	struct device *dev;
 
 	/*
 	 * Both the subdev and the async subdev can provide either an endpoint
@@ -116,7 +117,28 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
 
 	fwnode_handle_put(dev_fwnode);
 
-	return dev_fwnode == other_fwnode;
+	if (dev_fwnode != other_fwnode)
+		return false;
+
+	/*
+	 * We have a heterogeneous match. Retrieve the struct device of the side
+	 * that matched on a device fwnode to print its driver name.
+	 */
+	if (sd_fwnode_is_ep)
+		dev = notifier->v4l2_dev ? notifier->v4l2_dev->dev
+		    : notifier->sd->dev;
+	else
+		dev = sd->dev;
+
+	if (dev && dev->driver) {
+		if (sd_fwnode_is_ep)
+			dev_warn(dev, "Driver %s uses device fwnode, incorrect match may occur\n",
+				 dev->driver->name);
+		dev_notice(dev, "Consider updating driver %s to match on endpoints\n",
+			   dev->driver->name);
+	}
+
+	return true;
 }
 
 static bool match_custom(struct v4l2_async_notifier *notifier,
-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

On Sun, Jun 21, 2020 at 03:00:26AM +0300, 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 presence of remote endpoints)
> 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>
> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> Changes since v2:
> 
> - Add comment to explain that we're matching connecting endpoints
> - Don't check fwnode name to detect endpoint
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 45 +++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 8bde33c21ce4..f82e0a32647d 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -71,7 +71,50 @@ 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;
> +
> +	/*
> +	 * 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.
> +	 * Technically speaking this checks if the nodes refer to a connected
> +	 * endpoint, which is the simplest check that works for both OF and
> +	 * ACPI. This won't make a difference, as drivers should not try to
> +	 * match unconnected endpoints.
> +	 */
> +	sd_fwnode_is_ep = fwnode_property_present(sd->fwnode,
> +						  "remote-endpoint");
> +	asd_fwnode_is_ep = fwnode_property_present(asd->match.fwnode,
> +						   "remote-endpoint");

Please don't try parsing graph bindings outside the main parsers.

There's no API function to do just this, but you could go and check for the
port parent right away. The code might be even more simple that way.

Alternatively, I guess we could add fwnode_graph_is_endpoint() or something
but I wonder if it'd be worth it just for this.

> +
> +	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 v3 1/3] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Laurent Pinchart]

Hi Sakari,

On Wed, Jun 24, 2020 at 12:13:06AM +0300, Sakari Ailus wrote:
> On Sun, Jun 21, 2020 at 03:00:26AM +0300, 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 presence of remote endpoints)
> > 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>
> > Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> > Changes since v2:
> > 
> > - Add comment to explain that we're matching connecting endpoints
> > - Don't check fwnode name to detect endpoint
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 45 +++++++++++++++++++++++++++-
> >  1 file changed, 44 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 8bde33c21ce4..f82e0a32647d 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -71,7 +71,50 @@ 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;
> > +
> > +	/*
> > +	 * 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.
> > +	 * Technically speaking this checks if the nodes refer to a connected
> > +	 * endpoint, which is the simplest check that works for both OF and
> > +	 * ACPI. This won't make a difference, as drivers should not try to
> > +	 * match unconnected endpoints.
> > +	 */
> > +	sd_fwnode_is_ep = fwnode_property_present(sd->fwnode,
> > +						  "remote-endpoint");
> > +	asd_fwnode_is_ep = fwnode_property_present(asd->match.fwnode,
> > +						   "remote-endpoint");
> 
> Please don't try parsing graph bindings outside the main parsers.

Why is that ? On the DT side, bindings are considered to be stable, so
isolating their parsing in helpers would not help with ABI compatibility
anyway. Maybe it would be useful if you could explain how graph parsing
works in ACPI ? The fact that there's a remote-endpoint property without
endpoints is a the minimum quite puzzling.

> There's no API function to do just this, but you could go and check for the
> port parent right away. The code might be even more simple that way.

How will that help ? With OF at least, fwnode_graph_get_port_parent()
will return the grand-parent if the passed node isn't an endpoint, not
much can be deduced from that.

> Alternatively, I guess we could add fwnode_graph_is_endpoint() or something
> but I wonder if it'd be worth it just for this.
> 
> > +
> > +	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 v3 1/3] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Sakari Ailus]

Hi Laurent,

On Wed, Jun 24, 2020 at 12:22:41AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wed, Jun 24, 2020 at 12:13:06AM +0300, Sakari Ailus wrote:
> > On Sun, Jun 21, 2020 at 03:00:26AM +0300, 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 presence of remote endpoints)
> > > 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>
> > > Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > > Changes since v2:
> > > 
> > > - Add comment to explain that we're matching connecting endpoints
> > > - Don't check fwnode name to detect endpoint
> > > ---
> > >  drivers/media/v4l2-core/v4l2-async.c | 45 +++++++++++++++++++++++++++-
> > >  1 file changed, 44 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > index 8bde33c21ce4..f82e0a32647d 100644
> > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > @@ -71,7 +71,50 @@ 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;
> > > +
> > > +	/*
> > > +	 * 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.
> > > +	 * Technically speaking this checks if the nodes refer to a connected
> > > +	 * endpoint, which is the simplest check that works for both OF and
> > > +	 * ACPI. This won't make a difference, as drivers should not try to
> > > +	 * match unconnected endpoints.
> > > +	 */
> > > +	sd_fwnode_is_ep = fwnode_property_present(sd->fwnode,
> > > +						  "remote-endpoint");
> > > +	asd_fwnode_is_ep = fwnode_property_present(asd->match.fwnode,
> > > +						   "remote-endpoint");
> > 
> > Please don't try parsing graph bindings outside the main parsers.
> 
> Why is that ? On the DT side, bindings are considered to be stable, so
> isolating their parsing in helpers would not help with ABI compatibility
> anyway. Maybe it would be useful if you could explain how graph parsing
> works in ACPI ? The fact that there's a remote-endpoint property without
> endpoints is a the minimum quite puzzling.

No other drivers (or frameworks to my knowledge) work with the graphs
directly anymore. There was a staging driver (IMX) that did but that has
been fixed now. It's easier to ensure the code is correct --- this is
because the data structure is hard to parse, especially while taking
firmware type differences into account but the functions that access it are
relatively simple to use.

The fwnode property API has operations callbacks that are specific to the
type of the node. Most access functions have a firmware specific backend.

With the presence of the "remote-endpoint" property there's no variation
across the firmware types, at least not right now. But still putting it
here right now looks like technical debt to me: much of the code parsing
graph data structure outside the main parser has been buggy in the past.

> 
> > There's no API function to do just this, but you could go and check for the
> > port parent right away. The code might be even more simple that way.
> 
> How will that help ? With OF at least, fwnode_graph_get_port_parent()
> will return the grand-parent if the passed node isn't an endpoint, not
> much can be deduced from that.

I meant to say fwnode_graph_get_remote_endpoint(). You'd need to release
the fwnode reference, too.

> 
> > Alternatively, I guess we could add fwnode_graph_is_endpoint() or something
> > but I wonder if it'd be worth it just for this.
> > 
> > > +
> > > +	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)
> 

-- 
Terveisin,

Sakari Ailus

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

[Laurent Pinchart]

Hi Sakari,

On Wed, Jun 24, 2020 at 02:12:05AM +0300, Sakari Ailus wrote:
> On Wed, Jun 24, 2020 at 12:22:41AM +0300, Laurent Pinchart wrote:
> > On Wed, Jun 24, 2020 at 12:13:06AM +0300, Sakari Ailus wrote:
> >> On Sun, Jun 21, 2020 at 03:00:26AM +0300, 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 presence of remote endpoints)
> >>> 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>
> >>> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>> Changes since v2:
> >>> 
> >>> - Add comment to explain that we're matching connecting endpoints
> >>> - Don't check fwnode name to detect endpoint
> >>> ---
> >>>  drivers/media/v4l2-core/v4l2-async.c | 45 +++++++++++++++++++++++++++-
> >>>  1 file changed, 44 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> >>> index 8bde33c21ce4..f82e0a32647d 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-async.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-async.c
> >>> @@ -71,7 +71,50 @@ 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;
> >>> +
> >>> +	/*
> >>> +	 * 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.
> >>> +	 * Technically speaking this checks if the nodes refer to a connected
> >>> +	 * endpoint, which is the simplest check that works for both OF and
> >>> +	 * ACPI. This won't make a difference, as drivers should not try to
> >>> +	 * match unconnected endpoints.
> >>> +	 */
> >>> +	sd_fwnode_is_ep = fwnode_property_present(sd->fwnode,
> >>> +						  "remote-endpoint");
> >>> +	asd_fwnode_is_ep = fwnode_property_present(asd->match.fwnode,
> >>> +						   "remote-endpoint");
> >> 
> >> Please don't try parsing graph bindings outside the main parsers.
> > 
> > Why is that ? On the DT side, bindings are considered to be stable, so
> > isolating their parsing in helpers would not help with ABI compatibility
> > anyway. Maybe it would be useful if you could explain how graph parsing
> > works in ACPI ? The fact that there's a remote-endpoint property without
> > endpoints is a the minimum quite puzzling.
> 
> No other drivers (or frameworks to my knowledge) work with the graphs
> directly anymore. There was a staging driver (IMX) that did but that has
> been fixed now. It's easier to ensure the code is correct --- this is
> because the data structure is hard to parse, especially while taking
> firmware type differences into account but the functions that access it are
> relatively simple to use.
> 
> The fwnode property API has operations callbacks that are specific to the
> type of the node. Most access functions have a firmware specific backend.
> 
> With the presence of the "remote-endpoint" property there's no variation
> across the firmware types, at least not right now. But still putting it
> here right now looks like technical debt to me: much of the code parsing
> graph data structure outside the main parser has been buggy in the past.

For my information, could you still briefly explain how remote-endpoint
works on ACPI, without any fwnode named "endpoint" ?

> >> There's no API function to do just this, but you could go and check for the
> >> port parent right away. The code might be even more simple that way.
> > 
> > How will that help ? With OF at least, fwnode_graph_get_port_parent()
> > will return the grand-parent if the passed node isn't an endpoint, not
> > much can be deduced from that.
> 
> I meant to say fwnode_graph_get_remote_endpoint(). You'd need to release
> the fwnode reference, too.

That makes more sense :-)

> >> Alternatively, I guess we could add fwnode_graph_is_endpoint() or something
> >> but I wonder if it'd be worth it just for this.

Would

static inline bool fwnode_graph_is_endpoint(struct fwnode_handle *fwnode)
{
	return fwnode_property_present(fwnode, "remote-endpoint");
}

in include/linux/property.h be acceptable to you ?

> >>> +
> >>> +	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 v3 1/3] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Sakari Ailus]

Hi Laurent,

On Wed, Jun 24, 2020 at 03:12:53AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wed, Jun 24, 2020 at 02:12:05AM +0300, Sakari Ailus wrote:
> > On Wed, Jun 24, 2020 at 12:22:41AM +0300, Laurent Pinchart wrote:
> > > On Wed, Jun 24, 2020 at 12:13:06AM +0300, Sakari Ailus wrote:
> > >> On Sun, Jun 21, 2020 at 03:00:26AM +0300, 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 presence of remote endpoints)
> > >>> 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>
> > >>> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > >>> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > >>> ---
> > >>> Changes since v2:
> > >>> 
> > >>> - Add comment to explain that we're matching connecting endpoints
> > >>> - Don't check fwnode name to detect endpoint
> > >>> ---
> > >>>  drivers/media/v4l2-core/v4l2-async.c | 45 +++++++++++++++++++++++++++-
> > >>>  1 file changed, 44 insertions(+), 1 deletion(-)
> > >>> 
> > >>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > >>> index 8bde33c21ce4..f82e0a32647d 100644
> > >>> --- a/drivers/media/v4l2-core/v4l2-async.c
> > >>> +++ b/drivers/media/v4l2-core/v4l2-async.c
> > >>> @@ -71,7 +71,50 @@ 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;
> > >>> +
> > >>> +	/*
> > >>> +	 * 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.
> > >>> +	 * Technically speaking this checks if the nodes refer to a connected
> > >>> +	 * endpoint, which is the simplest check that works for both OF and
> > >>> +	 * ACPI. This won't make a difference, as drivers should not try to
> > >>> +	 * match unconnected endpoints.
> > >>> +	 */
> > >>> +	sd_fwnode_is_ep = fwnode_property_present(sd->fwnode,
> > >>> +						  "remote-endpoint");
> > >>> +	asd_fwnode_is_ep = fwnode_property_present(asd->match.fwnode,
> > >>> +						   "remote-endpoint");
> > >> 
> > >> Please don't try parsing graph bindings outside the main parsers.
> > > 
> > > Why is that ? On the DT side, bindings are considered to be stable, so
> > > isolating their parsing in helpers would not help with ABI compatibility
> > > anyway. Maybe it would be useful if you could explain how graph parsing
> > > works in ACPI ? The fact that there's a remote-endpoint property without
> > > endpoints is a the minimum quite puzzling.
> > 
> > No other drivers (or frameworks to my knowledge) work with the graphs
> > directly anymore. There was a staging driver (IMX) that did but that has
> > been fixed now. It's easier to ensure the code is correct --- this is
> > because the data structure is hard to parse, especially while taking
> > firmware type differences into account but the functions that access it are
> > relatively simple to use.
> > 
> > The fwnode property API has operations callbacks that are specific to the
> > type of the node. Most access functions have a firmware specific backend.
> > 
> > With the presence of the "remote-endpoint" property there's no variation
> > across the firmware types, at least not right now. But still putting it
> > here right now looks like technical debt to me: much of the code parsing
> > graph data structure outside the main parser has been buggy in the past.
> 
> For my information, could you still briefly explain how remote-endpoint
> works on ACPI, without any fwnode named "endpoint" ?

There have been a different versions of the ACPI graph definitions, and
firmware using both exists. See e.g. is_acpi_graph_node() and functions
below that in drivers/acpi/property.c .

> 
> > >> There's no API function to do just this, but you could go and check for the
> > >> port parent right away. The code might be even more simple that way.
> > > 
> > > How will that help ? With OF at least, fwnode_graph_get_port_parent()
> > > will return the grand-parent if the passed node isn't an endpoint, not
> > > much can be deduced from that.
> > 
> > I meant to say fwnode_graph_get_remote_endpoint(). You'd need to release
> > the fwnode reference, too.
> 
> That makes more sense :-)
> 
> > >> Alternatively, I guess we could add fwnode_graph_is_endpoint() or something
> > >> but I wonder if it'd be worth it just for this.
> 
> Would
> 
> static inline bool fwnode_graph_is_endpoint(struct fwnode_handle *fwnode)
> {
> 	return fwnode_property_present(fwnode, "remote-endpoint");
> }
> 
> in include/linux/property.h be acceptable to you ?

I think that'd be fine. If there's a need to change the implementation in
the future, it'll be easy.

-- 
Sakari Ailus

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

[Niklas Söderlund]

Hi Laurent,

Thanks for keep continued work on this.

For the whole series,

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

On 2020-06-21 03:00:25 +0300, Laurent Pinchart wrote:
> Hello,
> 
> This patch series is the third version of fwnode endpoint matching
> support in v4l2-async. The secon version can be found at [1].
> 
> Compared to v2, review comments have been incorporated, patch 4/4 got
> squashed in 1/4 (now 1/3), and the code has been rebased on top of
> v5.8-rc1.
> 
> Sakari, is they way that patch 1/3 determines if a fwnode is an endpoint
> acceptable for you (and for ACPI) ?
> 
> [1] https://lore.kernel.org/linux-media/20200318002507.30336-1-laurent.pinchart+renesas@ideasonboard.com
> 
> Laurent Pinchart (3):
>   media: v4l2-async: Accept endpoints and devices for fwnode matching
>   media: v4l2-async: Pass notifier pointer to match functions
>   media: v4l2-async: Log message in case of heterogeneous fwnode match
> 
>  drivers/media/v4l2-core/v4l2-async.c | 85 +++++++++++++++++++++++++---
>  1 file changed, 77 insertions(+), 8 deletions(-)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Regards,
Niklas Söderlund