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

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

[Laurent Pinchart]

Hello,

This patch series is the second version of fwnode endpoint matching
support in v4l2-async. The first version was a single patch and can be
found at [1].

Compared to v1, two additional changes have been made, which I have kept
as separate patches for now as they're under discussion. On top of the
base patch (1/4), patches 2/4 and 3/4 log a message when an heterogenous
match is detected. This should help speeding up the transition. Patch
4/4 moves away from checking the node name to determine if a fwnode is
an endpoint, as requesting by Sakari.

[1] https://lore.kernel.org/linux-media/20200318001726.GQ2527@pendragon.ideasonboard.com/T/#mfd71ee449a34f4f453941d5ec9a11f02cfb9e494

Laurent Pinchart (4):
  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 heterogenous fwnode match
  media: v4l2-async: Don't check fwnode name to detect endpoint

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

-- 
Regards,

Laurent Pinchart

[PATCH v2 1/4] 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>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 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

[PATCH v2 2/4] 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>
---
 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 995e5464cba7..224b39a7aeb1 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;
@@ -114,7 +116,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 */
@@ -131,7 +134,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) {
@@ -156,7 +160,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

Re: [PATCH v2 2/4] media: v4l2-async: Pass notifier pointer to match functions

[Sakari Ailus]

On Wed, Mar 18, 2020 at 02:25:05AM +0200, Laurent Pinchart wrote:
> 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>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus

Re: [PATCH v2 2/4] media: v4l2-async: Pass notifier pointer to match functions

[Kieran Bingham]

Hi Laurent,

On 18/03/2020 00:25, Laurent Pinchart wrote:
> 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.

The number of times I've added debug prints in these match functions
debugging issues already, I should have done this too!

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


> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  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 995e5464cba7..224b39a7aeb1 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;
> @@ -114,7 +116,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 */
> @@ -131,7 +134,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) {
> @@ -156,7 +160,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;
>  	}
>  
> 

[PATCH v2 3/4] media: v4l2-async: Log message in case of heterogenous 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>
---
 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 224b39a7aeb1..9f393a7be455 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;
 	const char *name;
 
 	/*
@@ -113,7 +114,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 an heterogenous 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_info(dev, "Driver %s uses device fwnode, incorrect match may occur\n",
+				 dev->driver->name);
+		dev_info(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 v2 3/4] media: v4l2-async: Log message in case of heterogenous fwnode match

[Kieran Bingham]

Hi Laurent,

On 18/03/2020 00:25, Laurent Pinchart wrote:
> 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.

Only minor comments and discussion below:

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

> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  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 224b39a7aeb1..9f393a7be455 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;
>  	const char *name;
>  
>  	/*
> @@ -113,7 +114,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 an heterogenous match. Retrieve the struct device of the

s/an/a/

s/heterogenous/heterogeneous/ (and that's not an en-gb/en-us thing)
Also in $SUBJECT

> +	 * 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;

Eugh ... I guess if this gets needed elsewhere, notifiers need a helper
to get the appropriate dev out... but if this is the only place, then so
be it.


> +	else
> +		dev = sd->dev;
> +
> +	if (dev && dev->driver) {
> +		if (sd_fwnode_is_ep)
> +			dev_info(dev, "Driver %s uses device fwnode, incorrect match may occur\n",
> +				 dev->driver->name);
> +		dev_info(dev, "Consider updating driver %s to match on endpoints\n",
> +			 dev->driver->name);

I think I interpret that in the case that existing drivers match on
dev->dev (i.e. no endpoints involved) then this will not print, as we
would already have matched and returned earlier in the function.

I don't think that's a problem, but it means people will not be
'encouraged' to move to endpoint matching until they encounter a device
which uses endpoints.

Perhaps that's ok ... but I was almost thinking of being more 'pushy'
and guiding device matches to move to endpoints too ;-)


> +	}
> +
> +	return true;
>  }
>  
>  static bool match_custom(struct v4l2_async_notifier *notifier,
> 

Re: [PATCH v2 3/4] media: v4l2-async: Log message in case of heterogenous fwnode match

[Laurent Pinchart]

Hi Kieran,

On Wed, Mar 18, 2020 at 09:16:01AM +0000, Kieran Bingham wrote:
> On 18/03/2020 00:25, Laurent Pinchart wrote:
> > 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.
> 
> Only minor comments and discussion below:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  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 224b39a7aeb1..9f393a7be455 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;
> >  	const char *name;
> >  
> >  	/*
> > @@ -113,7 +114,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 an heterogenous match. Retrieve the struct device of the
> 
> s/an/a/
> 
> s/heterogenous/heterogeneous/ (and that's not an en-gb/en-us thing)
> Also in $SUBJECT
> 
> > +	 * 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;
> 
> Eugh ... I guess if this gets needed elsewhere, notifiers need a helper
> to get the appropriate dev out... but if this is the only place, then so
> be it.

I'll let the next person who needs this create a helper :-)

> > +	else
> > +		dev = sd->dev;
> > +
> > +	if (dev && dev->driver) {
> > +		if (sd_fwnode_is_ep)
> > +			dev_info(dev, "Driver %s uses device fwnode, incorrect match may occur\n",
> > +				 dev->driver->name);
> > +		dev_info(dev, "Consider updating driver %s to match on endpoints\n",
> > +			 dev->driver->name);
> 
> I think I interpret that in the case that existing drivers match on
> dev->dev (i.e. no endpoints involved) then this will not print, as we
> would already have matched and returned earlier in the function.
> 
> I don't think that's a problem, but it means people will not be
> 'encouraged' to move to endpoint matching until they encounter a device
> which uses endpoints.
> 
> Perhaps that's ok ... but I was almost thinking of being more 'pushy'
> and guiding device matches to move to endpoints too ;-)

I don't think we can do that, as not all devices have endpoints. For
instance, a lens controller or a flash controller will be referred to by
phandle properties, without using OF graph. We still need to support
matching them.

> > +	}
> > +
> > +	return true;
> >  }
> >  
> >  static bool match_custom(struct v4l2_async_notifier *notifier,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/4] media: v4l2-async: Log message in case of heterogenous fwnode match

[Geert Uytterhoeven]

Hi Laurent,

On Wed, Mar 18, 2020 at 1:26 AM Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> 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>

Thanks for your patch!

> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -113,7 +114,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 an heterogenous 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_info(dev, "Driver %s uses device fwnode, incorrect match may occur\n",
> +                                dev->driver->name);

I think that deserves a dev_warn().

> +               dev_info(dev, "Consider updating driver %s to match on endpoints\n",
> +                        dev->driver->name);

And dev_notice(), while at it ;-)

> +       }
> +
> +       return true;
>  }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/4] media: v4l2-async: Log message in case of heterogenous fwnode match

[Laurent Pinchart]

Hi Geert,

On Wed, Mar 18, 2020 at 10:16:37AM +0100, Geert Uytterhoeven wrote:
> On Wed, Mar 18, 2020 at 1:26 AM Laurent Pinchart wrote:
> > 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>
> 
> Thanks for your patch!
> 
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -113,7 +114,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 an heterogenous 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_info(dev, "Driver %s uses device fwnode, incorrect match may occur\n",
> > +                                dev->driver->name);
> 
> I think that deserves a dev_warn().
> 
> > +               dev_info(dev, "Consider updating driver %s to match on endpoints\n",
> > +                        dev->driver->name);
> 
> And dev_notice(), while at it ;-)

A rarely used, but totally appropriate level. I'll fix both.

> > +       }
> > +
> > +       return true;
> >  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/4] media: v4l2-async: Log message in case of heterogenous fwnode match

[Jacopo Mondi]

Hi Laurent,

On Wed, Mar 18, 2020 at 02:25:06AM +0200, Laurent Pinchart wrote:
> 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>
> ---
>  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 224b39a7aeb1..9f393a7be455 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;
>  	const char *name;
>
>  	/*
> @@ -113,7 +114,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 an heterogenous 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;

Have you considered passing the device directly ? seems notifier is
only used for that...

Apart this small nit, for the series
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
   j

> +	else
> +		dev = sd->dev;
> +
> +	if (dev && dev->driver) {
> +		if (sd_fwnode_is_ep)
> +			dev_info(dev, "Driver %s uses device fwnode, incorrect match may occur\n",
> +				 dev->driver->name);
> +		dev_info(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 v2 3/4] media: v4l2-async: Log message in case of heterogenous fwnode match

[Laurent Pinchart]

Hi Jacopo,

On Wed, Mar 18, 2020 at 03:03:26PM +0100, Jacopo Mondi wrote:
> On Wed, Mar 18, 2020 at 02:25:06AM +0200, Laurent Pinchart wrote:
> > 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>
> > ---
> >  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 224b39a7aeb1..9f393a7be455 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;
> >  	const char *name;
> >
> >  	/*
> > @@ -113,7 +114,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 an heterogenous 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;
> 
> Have you considered passing the device directly ? seems notifier is
> only used for that...

Yes, but I thought that match functions could use the notifier for other
purposes in the future, so I think this approach is more versatile.

> Apart this small nit, for the series
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> > +	else
> > +		dev = sd->dev;
> > +
> > +	if (dev && dev->driver) {
> > +		if (sd_fwnode_is_ep)
> > +			dev_info(dev, "Driver %s uses device fwnode, incorrect match may occur\n",
> > +				 dev->driver->name);
> > +		dev_info(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

[PATCH v2 4/4] media: v4l2-async: Don't check fwnode name to detect endpoint

[Laurent Pinchart]

Use the presence of a "remote-endpoint" property to detect if a fwnode
is an endpoint node, as checking the node name won't work on ACPI-based
implementations.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-async.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 9f393a7be455..a5f83ba502df 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -78,7 +78,6 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
 	bool asd_fwnode_is_ep;
 	bool sd_fwnode_is_ep;
 	struct device *dev;
-	const char *name;
 
 	/*
 	 * Both the subdev and the async subdev can provide either an endpoint
@@ -92,10 +91,10 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
 	 * 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");
+	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;
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/4] media: v4l2-async: Don't check fwnode name to detect endpoint

[Kieran Bingham]

Hi Laurent,

On 18/03/2020 00:25, Laurent Pinchart wrote:
> Use the presence of a "remote-endpoint" property to detect if a fwnode
> is an endpoint node, as checking the node name won't work on ACPI-based
> implementations.

Technically, won't this property only detect that the endpoint is
connected to another endpoint, and 'un-connected' endpoints wont' match?

Of course in this instance - an unconnected endpoint is likely not much
use and probably even shouldn't match ;-) ~(but it may still 'be' an
endpoint).

Also - would this patch be squashed into 1/4?

I'll leave it to Sakari to comment on the actual validity of this
approach all the same :-)

--
Kieran


> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 9f393a7be455..a5f83ba502df 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -78,7 +78,6 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
>  	bool asd_fwnode_is_ep;
>  	bool sd_fwnode_is_ep;
>  	struct device *dev;
> -	const char *name;
>  
>  	/*
>  	 * Both the subdev and the async subdev can provide either an endpoint
> @@ -92,10 +91,10 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
>  	 * 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");
> +	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;
> 

Re: [PATCH v2 4/4] media: v4l2-async: Don't check fwnode name to detect endpoint

[Laurent Pinchart]

Hi Kieran,

On Wed, Mar 18, 2020 at 09:22:21AM +0000, Kieran Bingham wrote:
> On 18/03/2020 00:25, Laurent Pinchart wrote:
> > Use the presence of a "remote-endpoint" property to detect if a fwnode
> > is an endpoint node, as checking the node name won't work on ACPI-based
> > implementations.
> 
> Technically, won't this property only detect that the endpoint is
> connected to another endpoint, and 'un-connected' endpoints wont' match?
> 
> Of course in this instance - an unconnected endpoint is likely not much
> use and probably even shouldn't match ;-) ~(but it may still 'be' an
> endpoint).

Yes, technically speaking, this detects whether the node is an endpoint
and is connected. As you mentioned, there's little use in unconnected
endpoints here, as nobody would attempt to match them.

> Also - would this patch be squashed into 1/4?

I think that would make sense, yes. I'll do so in v3.

> I'll leave it to Sakari to comment on the actual validity of this
> approach all the same :-)

ACPI has a remote-endpoint property, but has no endpoint nodes. I wonder
what the remote-endpoint points to...

> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 9f393a7be455..a5f83ba502df 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -78,7 +78,6 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
> >  	bool asd_fwnode_is_ep;
> >  	bool sd_fwnode_is_ep;
> >  	struct device *dev;
> > -	const char *name;
> >  
> >  	/*
> >  	 * Both the subdev and the async subdev can provide either an endpoint
> > @@ -92,10 +91,10 @@ static bool match_fwnode(struct v4l2_async_notifier *notifier,
> >  	 * 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");
> > +	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;

-- 
Regards,

Laurent Pinchart

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

[Lad, Prabhakar]

Hi Laurent,

On Wed, Mar 18, 2020 at 12:25 AM Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
>
> Hello,
>
> This patch series is the second version of fwnode endpoint matching
> support in v4l2-async. The first version was a single patch and can be
> found at [1].
>
> Compared to v1, two additional changes have been made, which I have kept
> as separate patches for now as they're under discussion. On top of the
> base patch (1/4), patches 2/4 and 3/4 log a message when an heterogenous
> match is detected. This should help speeding up the transition. Patch
> 4/4 moves away from checking the node name to determine if a fwnode is
> an endpoint, as requesting by Sakari.
>
> [1] https://lore.kernel.org/linux-media/20200318001726.GQ2527@pendragon.ideasonboard.com/T/#mfd71ee449a34f4f453941d5ec9a11f02cfb9e494
>
> Laurent Pinchart (4):
>   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 heterogenous fwnode match
>   media: v4l2-async: Don't check fwnode name to detect endpoint
>
>  drivers/media/v4l2-core/v4l2-async.c | 81 +++++++++++++++++++++++++---
>  1 file changed, 73 insertions(+), 8 deletions(-)
>
Thank you for the patches,

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

Cheers,
--Prabhakar

> --
> Regards,
>
> Laurent Pinchart
>