[PATCH v3 00/13] media: imx: Switch to subdev notifiers

[PATCH v3 00/13] media: imx: Switch to subdev notifiers

[Steve Longerbeam]

This patchset converts the imx-media driver and its dependent
subdevs to use subdev notifiers.

There are a couple shortcomings in v4l2-core that prevented
subdev notifiers from working correctly in imx-media:

1. v4l2_async_notifier_fwnode_parse_endpoint() treats a fwnode
   endpoint that is not connected to a remote device as an error.
   But in the case of the video-mux subdev, this is not an error,
   it is OK if some of the mux inputs have no connection. Also,
   Documentation/devicetree/bindings/media/video-interfaces.txt explicitly
   states that the 'remote-endpoint' property is optional. So the first
   patch is a small modification to ignore empty endpoints in
   v4l2_async_notifier_fwnode_parse_endpoint() and allow
   __v4l2_async_notifier_parse_fwnode_endpoints() to continue to
   parse the remaining port endpoints of the device.

2. In the imx-media graph, multiple subdevs will encounter the same
   upstream subdev (such as the imx6-mipi-csi2 receiver), and so
   v4l2_async_notifier_parse_fwnode_endpoints() will add imx6-mipi-csi2
   multiple times. This is treated as an error by
   v4l2_async_notifier_register() later.

   To get around this problem, add an v4l2_async_notifier_add_subdev()
   which first verifies the provided asd does not already exist in the
   given notifier asd list or in other registered notifiers. If the asd
   exists, the function returns -EEXIST and it's up to the caller to
   decide if that is an error (in imx-media case it is never an error).

   Patches 2-4 deal with adding that support.

3. Patch 5 adds v4l2_async_register_fwnode_subdev(), which is a
   convenience function for parsing a subdev's fwnode port endpoints
   for connected remote subdevs, registering a subdev notifier, and
   then registering the sub-device itself.

The remaining patches update the subdev drivers to register a
subdev notifier with endpoint parsing, and the changes to imx-media
to support that.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

History:
v3:
- code optimization in asd_equal(), and remove unneeded braces,
  suggested by Sakari Ailus.
- add a NULL asd pointer check to v4l2_async_notifier_asd_valid().
- fix an error-out path in v4l2_async_register_fwnode_subdev() that
  forgot to put device.

v2:
- don't pass an empty endpoint to the parse_endpoint callback, 
  v4l2_async_notifier_fwnode_parse_endpoint() now just ignores them
  and returns success.
- Fix a couple compile warnings and errors seen in i386 and sh archs.


Steve Longerbeam (13):
  media: v4l2-fwnode: ignore endpoints that have no remote port parent
  media: v4l2: async: Allow searching for asd of any type
  media: v4l2: async: Add v4l2_async_notifier_add_subdev
  media: v4l2-fwnode: Switch to v4l2_async_notifier_add_subdev
  media: v4l2-fwnode: Add a convenience function for registering subdevs
    with notifiers
  media: platform: video-mux: Register a subdev notifier
  media: imx: csi: Register a subdev notifier
  media: imx: mipi csi-2: Register a subdev notifier
  media: staging/imx: of: Remove recursive graph walk
  media: staging/imx: Loop through all registered subdevs for media
    links
  media: staging/imx: Rename root notifier
  media: staging/imx: Switch to v4l2_async_notifier_add_subdev
  media: staging/imx: TODO: Remove one assumption about OF graph parsing

 drivers/media/pci/intel/ipu3/ipu3-cio2.c          |  10 +-
 drivers/media/platform/video-mux.c                |  36 ++-
 drivers/media/v4l2-core/v4l2-async.c              | 268 ++++++++++++++++------
 drivers/media/v4l2-core/v4l2-fwnode.c             | 231 +++++++++++--------
 drivers/staging/media/imx/TODO                    |  29 +--
 drivers/staging/media/imx/imx-media-csi.c         |  11 +-
 drivers/staging/media/imx/imx-media-dev.c         | 134 +++--------
 drivers/staging/media/imx/imx-media-internal-sd.c |   5 +-
 drivers/staging/media/imx/imx-media-of.c          | 106 +--------
 drivers/staging/media/imx/imx-media.h             |   6 +-
 drivers/staging/media/imx/imx6-mipi-csi2.c        |  31 ++-
 include/media/v4l2-async.h                        |  24 +-
 include/media/v4l2-fwnode.h                       |  65 +++++-
 13 files changed, 534 insertions(+), 422 deletions(-)

-- 
2.7.4

[PATCH v3 01/13] media: v4l2-fwnode: ignore endpoints that have no remote port parent

[Steve Longerbeam]

Documentation/devicetree/bindings/media/video-interfaces.txt states that
the 'remote-endpoint' property is optional.

So v4l2_async_notifier_fwnode_parse_endpoint() should not return error
if the endpoint has no remote port parent. Just ignore the endpoint,
skip adding an asd to the notifier and return 0.
__v4l2_async_notifier_parse_fwnode_endpoints() will then continue
parsing the remaining port endpoints of the device.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
Changes since v2:
- none
Changes since v1:
- don't pass an empty endpoint to the parse_endpoint callback,
  v4l2_async_notifier_fwnode_parse_endpoint() now just ignores them
  and returns success. The current users of
  v4l2_async_notifier_parse_fwnode_endpoints() (omap3isp, rcar-vin,
  intel-ipu3) no longer need modification.
---
 drivers/media/v4l2-core/v4l2-fwnode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index d630640..b8afbac 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -363,7 +363,7 @@ static int v4l2_async_notifier_fwnode_parse_endpoint(
 		fwnode_graph_get_remote_port_parent(endpoint);
 	if (!asd->match.fwnode) {
 		dev_warn(dev, "bad remote port parent\n");
-		ret = -EINVAL;
+		ret = -ENOTCONN;
 		goto out_err;
 	}
 
-- 
2.7.4

Re: [PATCH v3 01/13] media: v4l2-fwnode: ignore endpoints that have no remote port parent

[Hans Verkuil]

On 03/21/18 01:37, Steve Longerbeam wrote:
> Documentation/devicetree/bindings/media/video-interfaces.txt states that
> the 'remote-endpoint' property is optional.
> 
> So v4l2_async_notifier_fwnode_parse_endpoint() should not return error
> if the endpoint has no remote port parent. Just ignore the endpoint,
> skip adding an asd to the notifier and return 0.
> __v4l2_async_notifier_parse_fwnode_endpoints() will then continue
> parsing the remaining port endpoints of the device.
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

> ---
> Changes since v2:
> - none
> Changes since v1:
> - don't pass an empty endpoint to the parse_endpoint callback,
>   v4l2_async_notifier_fwnode_parse_endpoint() now just ignores them
>   and returns success. The current users of
>   v4l2_async_notifier_parse_fwnode_endpoints() (omap3isp, rcar-vin,
>   intel-ipu3) no longer need modification.
> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index d630640..b8afbac 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -363,7 +363,7 @@ static int v4l2_async_notifier_fwnode_parse_endpoint(
>  		fwnode_graph_get_remote_port_parent(endpoint);
>  	if (!asd->match.fwnode) {
>  		dev_warn(dev, "bad remote port parent\n");
> -		ret = -EINVAL;
> +		ret = -ENOTCONN;
>  		goto out_err;
>  	}
>  
> 

[PATCH v3 02/13] media: v4l2: async: Allow searching for asd of any type

[Steve Longerbeam]

Generalize v4l2_async_notifier_fwnode_has_async_subdev() to allow
searching for any type of async subdev, not just fwnodes. Rename to
v4l2_async_notifier_has_async_subdev() and pass it an asd pointer.

TODO: support asd compare with CUSTOM match type in asd_equal().

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
Changes since v2:
- code optimization in asd_equal(), and remove unneeded braces,
  suggested by Sakari Ailus.
Changes since v1:
- none
---
 drivers/media/v4l2-core/v4l2-async.c | 76 ++++++++++++++++++++++--------------
 1 file changed, 46 insertions(+), 30 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 2b08d03..b59bbac 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -124,6 +124,34 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
 	return NULL;
 }
 
+/* Compare two asd's for equivalence */
+static bool asd_equal(struct v4l2_async_subdev *asd_x,
+		      struct v4l2_async_subdev *asd_y)
+{
+	if (asd_x->match_type != asd_y->match_type)
+		return false;
+
+	switch (asd_x->match_type) {
+	case V4L2_ASYNC_MATCH_DEVNAME:
+		return strcmp(asd_x->match.device_name,
+			      asd_y->match.device_name) == 0;
+	case V4L2_ASYNC_MATCH_I2C:
+		return asd_x->match.i2c.adapter_id ==
+			asd_y->match.i2c.adapter_id &&
+			asd_x->match.i2c.address ==
+			asd_y->match.i2c.address;
+	case V4L2_ASYNC_MATCH_FWNODE:
+		return asd_x->match.fwnode == asd_y->match.fwnode;
+	case V4L2_ASYNC_MATCH_CUSTOM:
+		/* TODO */
+		return false;
+	default:
+		break;
+	}
+
+	return false;
+}
+
 /* Find the sub-device notifier registered by a sub-device driver. */
 static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
 	struct v4l2_subdev *sd)
@@ -308,29 +336,22 @@ static void v4l2_async_notifier_unbind_all_subdevs(
 	notifier->parent = NULL;
 }
 
-/* See if an fwnode can be found in a notifier's lists. */
-static bool __v4l2_async_notifier_fwnode_has_async_subdev(
-	struct v4l2_async_notifier *notifier, struct fwnode_handle *fwnode)
+/* See if an async sub-device can be found in a notifier's lists. */
+static bool __v4l2_async_notifier_has_async_subdev(
+	struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd)
 {
-	struct v4l2_async_subdev *asd;
+	struct v4l2_async_subdev *asd_y;
 	struct v4l2_subdev *sd;
 
-	list_for_each_entry(asd, &notifier->waiting, list) {
-		if (asd->match_type != V4L2_ASYNC_MATCH_FWNODE)
-			continue;
-
-		if (asd->match.fwnode == fwnode)
+	list_for_each_entry(asd_y, &notifier->waiting, list)
+		if (asd_equal(asd, asd_y))
 			return true;
-	}
 
 	list_for_each_entry(sd, &notifier->done, async_list) {
 		if (WARN_ON(!sd->asd))
 			continue;
 
-		if (sd->asd->match_type != V4L2_ASYNC_MATCH_FWNODE)
-			continue;
-
-		if (sd->asd->match.fwnode == fwnode)
+		if (asd_equal(asd, sd->asd))
 			return true;
 	}
 
@@ -338,32 +359,28 @@ static bool __v4l2_async_notifier_fwnode_has_async_subdev(
 }
 
 /*
- * Find out whether an async sub-device was set up for an fwnode already or
+ * Find out whether an async sub-device was set up already or
  * whether it exists in a given notifier before @this_index.
  */
-static bool v4l2_async_notifier_fwnode_has_async_subdev(
-	struct v4l2_async_notifier *notifier, struct fwnode_handle *fwnode,
+static bool v4l2_async_notifier_has_async_subdev(
+	struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd,
 	unsigned int this_index)
 {
 	unsigned int j;
 
 	lockdep_assert_held(&list_lock);
 
-	/* Check that an fwnode is not being added more than once. */
+	/* Check that an asd is not being added more than once. */
 	for (j = 0; j < this_index; j++) {
-		struct v4l2_async_subdev *asd = notifier->subdevs[this_index];
-		struct v4l2_async_subdev *other_asd = notifier->subdevs[j];
+		struct v4l2_async_subdev *asd_y = notifier->subdevs[j];
 
-		if (other_asd->match_type == V4L2_ASYNC_MATCH_FWNODE &&
-		    asd->match.fwnode ==
-		    other_asd->match.fwnode)
+		if (asd_equal(asd, asd_y))
 			return true;
 	}
 
-	/* Check than an fwnode did not exist in other notifiers. */
+	/* Check that an asd does not exist in other notifiers. */
 	list_for_each_entry(notifier, &notifier_list, list)
-		if (__v4l2_async_notifier_fwnode_has_async_subdev(
-			    notifier, fwnode))
+		if (__v4l2_async_notifier_has_async_subdev(notifier, asd))
 			return true;
 
 	return false;
@@ -392,12 +409,11 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
 		case V4L2_ASYNC_MATCH_CUSTOM:
 		case V4L2_ASYNC_MATCH_DEVNAME:
 		case V4L2_ASYNC_MATCH_I2C:
-			break;
 		case V4L2_ASYNC_MATCH_FWNODE:
-			if (v4l2_async_notifier_fwnode_has_async_subdev(
-				    notifier, asd->match.fwnode, i)) {
+			if (v4l2_async_notifier_has_async_subdev(
+				    notifier, asd, i)) {
 				dev_err(dev,
-					"fwnode has already been registered or in notifier's subdev list\n");
+					"asd has already been registered or in notifier's subdev list\n");
 				ret = -EEXIST;
 				goto err_unlock;
 			}
-- 
2.7.4

Re: [PATCH v3 02/13] media: v4l2: async: Allow searching for asd of any type

[Hans Verkuil]

On 03/21/18 01:37, Steve Longerbeam wrote:
> Generalize v4l2_async_notifier_fwnode_has_async_subdev() to allow
> searching for any type of async subdev, not just fwnodes. Rename to
> v4l2_async_notifier_has_async_subdev() and pass it an asd pointer.
> 
> TODO: support asd compare with CUSTOM match type in asd_equal().
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
> Changes since v2:
> - code optimization in asd_equal(), and remove unneeded braces,
>   suggested by Sakari Ailus.
> Changes since v1:
> - none
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 76 ++++++++++++++++++++++--------------
>  1 file changed, 46 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 2b08d03..b59bbac 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -124,6 +124,34 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
>  	return NULL;
>  }
>  
> +/* Compare two asd's for equivalence */
> +static bool asd_equal(struct v4l2_async_subdev *asd_x,
> +		      struct v4l2_async_subdev *asd_y)
> +{
> +	if (asd_x->match_type != asd_y->match_type)
> +		return false;
> +
> +	switch (asd_x->match_type) {
> +	case V4L2_ASYNC_MATCH_DEVNAME:
> +		return strcmp(asd_x->match.device_name,
> +			      asd_y->match.device_name) == 0;
> +	case V4L2_ASYNC_MATCH_I2C:
> +		return asd_x->match.i2c.adapter_id ==
> +			asd_y->match.i2c.adapter_id &&
> +			asd_x->match.i2c.address ==
> +			asd_y->match.i2c.address;
> +	case V4L2_ASYNC_MATCH_FWNODE:
> +		return asd_x->match.fwnode == asd_y->match.fwnode;
> +	case V4L2_ASYNC_MATCH_CUSTOM:
> +		/* TODO */

This TODO suggests that this is needed for some driver(s) and that it
will be implemented later, but it seems more that nobody actually needs
this. If that's the case, then I'd just drop this case altogether.

Or do I misunderstand this comment?

Regards,

	Hans

> +		return false;
> +	default:
> +		break;
> +	}
> +
> +	return false;
> +}
> +
>  /* Find the sub-device notifier registered by a sub-device driver. */
>  static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
>  	struct v4l2_subdev *sd)
> @@ -308,29 +336,22 @@ static void v4l2_async_notifier_unbind_all_subdevs(
>  	notifier->parent = NULL;
>  }
>  
> -/* See if an fwnode can be found in a notifier's lists. */
> -static bool __v4l2_async_notifier_fwnode_has_async_subdev(
> -	struct v4l2_async_notifier *notifier, struct fwnode_handle *fwnode)
> +/* See if an async sub-device can be found in a notifier's lists. */
> +static bool __v4l2_async_notifier_has_async_subdev(
> +	struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd)
>  {
> -	struct v4l2_async_subdev *asd;
> +	struct v4l2_async_subdev *asd_y;
>  	struct v4l2_subdev *sd;
>  
> -	list_for_each_entry(asd, &notifier->waiting, list) {
> -		if (asd->match_type != V4L2_ASYNC_MATCH_FWNODE)
> -			continue;
> -
> -		if (asd->match.fwnode == fwnode)
> +	list_for_each_entry(asd_y, &notifier->waiting, list)
> +		if (asd_equal(asd, asd_y))
>  			return true;
> -	}
>  
>  	list_for_each_entry(sd, &notifier->done, async_list) {
>  		if (WARN_ON(!sd->asd))
>  			continue;
>  
> -		if (sd->asd->match_type != V4L2_ASYNC_MATCH_FWNODE)
> -			continue;
> -
> -		if (sd->asd->match.fwnode == fwnode)
> +		if (asd_equal(asd, sd->asd))
>  			return true;
>  	}
>  
> @@ -338,32 +359,28 @@ static bool __v4l2_async_notifier_fwnode_has_async_subdev(
>  }
>  
>  /*
> - * Find out whether an async sub-device was set up for an fwnode already or
> + * Find out whether an async sub-device was set up already or
>   * whether it exists in a given notifier before @this_index.
>   */
> -static bool v4l2_async_notifier_fwnode_has_async_subdev(
> -	struct v4l2_async_notifier *notifier, struct fwnode_handle *fwnode,
> +static bool v4l2_async_notifier_has_async_subdev(
> +	struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd,
>  	unsigned int this_index)
>  {
>  	unsigned int j;
>  
>  	lockdep_assert_held(&list_lock);
>  
> -	/* Check that an fwnode is not being added more than once. */
> +	/* Check that an asd is not being added more than once. */
>  	for (j = 0; j < this_index; j++) {
> -		struct v4l2_async_subdev *asd = notifier->subdevs[this_index];
> -		struct v4l2_async_subdev *other_asd = notifier->subdevs[j];
> +		struct v4l2_async_subdev *asd_y = notifier->subdevs[j];
>  
> -		if (other_asd->match_type == V4L2_ASYNC_MATCH_FWNODE &&
> -		    asd->match.fwnode ==
> -		    other_asd->match.fwnode)
> +		if (asd_equal(asd, asd_y))
>  			return true;
>  	}
>  
> -	/* Check than an fwnode did not exist in other notifiers. */
> +	/* Check that an asd does not exist in other notifiers. */
>  	list_for_each_entry(notifier, &notifier_list, list)
> -		if (__v4l2_async_notifier_fwnode_has_async_subdev(
> -			    notifier, fwnode))
> +		if (__v4l2_async_notifier_has_async_subdev(notifier, asd))
>  			return true;
>  
>  	return false;
> @@ -392,12 +409,11 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
>  		case V4L2_ASYNC_MATCH_CUSTOM:
>  		case V4L2_ASYNC_MATCH_DEVNAME:
>  		case V4L2_ASYNC_MATCH_I2C:
> -			break;
>  		case V4L2_ASYNC_MATCH_FWNODE:
> -			if (v4l2_async_notifier_fwnode_has_async_subdev(
> -				    notifier, asd->match.fwnode, i)) {
> +			if (v4l2_async_notifier_has_async_subdev(
> +				    notifier, asd, i)) {
>  				dev_err(dev,
> -					"fwnode has already been registered or in notifier's subdev list\n");
> +					"asd has already been registered or in notifier's subdev list\n");
>  				ret = -EEXIST;
>  				goto err_unlock;
>  			}
> 

Re: [PATCH v3 02/13] media: v4l2: async: Allow searching for asd of any type

[Steve Longerbeam]

Hi Hans,


On 04/20/2018 05:22 AM, Hans Verkuil wrote:
> On 03/21/18 01:37, Steve Longerbeam wrote:
>> Generalize v4l2_async_notifier_fwnode_has_async_subdev() to allow
>> searching for any type of async subdev, not just fwnodes. Rename to
>> v4l2_async_notifier_has_async_subdev() and pass it an asd pointer.
>>
>> TODO: support asd compare with CUSTOM match type in asd_equal().
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> ---
>> Changes since v2:
>> - code optimization in asd_equal(), and remove unneeded braces,
>>    suggested by Sakari Ailus.
>> Changes since v1:
>> - none
>> ---
>>   drivers/media/v4l2-core/v4l2-async.c | 76 ++++++++++++++++++++++--------------
>>   1 file changed, 46 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>> index 2b08d03..b59bbac 100644
>> --- a/drivers/media/v4l2-core/v4l2-async.c
>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>> @@ -124,6 +124,34 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
>>   	return NULL;
>>   }
>>   
>> +/* Compare two asd's for equivalence */
>> +static bool asd_equal(struct v4l2_async_subdev *asd_x,
>> +		      struct v4l2_async_subdev *asd_y)
>> +{
>> +	if (asd_x->match_type != asd_y->match_type)
>> +		return false;
>> +
>> +	switch (asd_x->match_type) {
>> +	case V4L2_ASYNC_MATCH_DEVNAME:
>> +		return strcmp(asd_x->match.device_name,
>> +			      asd_y->match.device_name) == 0;
>> +	case V4L2_ASYNC_MATCH_I2C:
>> +		return asd_x->match.i2c.adapter_id ==
>> +			asd_y->match.i2c.adapter_id &&
>> +			asd_x->match.i2c.address ==
>> +			asd_y->match.i2c.address;
>> +	case V4L2_ASYNC_MATCH_FWNODE:
>> +		return asd_x->match.fwnode == asd_y->match.fwnode;
>> +	case V4L2_ASYNC_MATCH_CUSTOM:
>> +		/* TODO */
> This TODO suggests that this is needed for some driver(s) and that it
> will be implemented later, but it seems more that nobody actually needs
> this. If that's the case, then I'd just drop this case altogether.
>
> Or do I misunderstand this comment?

No you are correct, I thought maybe this could be implemented
later. There are no platforms that make use of V4L2_ASYNC_MATCH_CUSTOM.
If you don't think there ever will be, I will just drop this.


Steve

>> +		return false;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>>   /* Find the sub-device notifier registered by a sub-device driver. */
>>   static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
>>   	struct v4l2_subdev *sd)
>> @@ -308,29 +336,22 @@ static void v4l2_async_notifier_unbind_all_subdevs(
>>   	notifier->parent = NULL;
>>   }
>>   
>> -/* See if an fwnode can be found in a notifier's lists. */
>> -static bool __v4l2_async_notifier_fwnode_has_async_subdev(
>> -	struct v4l2_async_notifier *notifier, struct fwnode_handle *fwnode)
>> +/* See if an async sub-device can be found in a notifier's lists. */
>> +static bool __v4l2_async_notifier_has_async_subdev(
>> +	struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd)
>>   {
>> -	struct v4l2_async_subdev *asd;
>> +	struct v4l2_async_subdev *asd_y;
>>   	struct v4l2_subdev *sd;
>>   
>> -	list_for_each_entry(asd, &notifier->waiting, list) {
>> -		if (asd->match_type != V4L2_ASYNC_MATCH_FWNODE)
>> -			continue;
>> -
>> -		if (asd->match.fwnode == fwnode)
>> +	list_for_each_entry(asd_y, &notifier->waiting, list)
>> +		if (asd_equal(asd, asd_y))
>>   			return true;
>> -	}
>>   
>>   	list_for_each_entry(sd, &notifier->done, async_list) {
>>   		if (WARN_ON(!sd->asd))
>>   			continue;
>>   
>> -		if (sd->asd->match_type != V4L2_ASYNC_MATCH_FWNODE)
>> -			continue;
>> -
>> -		if (sd->asd->match.fwnode == fwnode)
>> +		if (asd_equal(asd, sd->asd))
>>   			return true;
>>   	}
>>   
>> @@ -338,32 +359,28 @@ static bool __v4l2_async_notifier_fwnode_has_async_subdev(
>>   }
>>   
>>   /*
>> - * Find out whether an async sub-device was set up for an fwnode already or
>> + * Find out whether an async sub-device was set up already or
>>    * whether it exists in a given notifier before @this_index.
>>    */
>> -static bool v4l2_async_notifier_fwnode_has_async_subdev(
>> -	struct v4l2_async_notifier *notifier, struct fwnode_handle *fwnode,
>> +static bool v4l2_async_notifier_has_async_subdev(
>> +	struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd,
>>   	unsigned int this_index)
>>   {
>>   	unsigned int j;
>>   
>>   	lockdep_assert_held(&list_lock);
>>   
>> -	/* Check that an fwnode is not being added more than once. */
>> +	/* Check that an asd is not being added more than once. */
>>   	for (j = 0; j < this_index; j++) {
>> -		struct v4l2_async_subdev *asd = notifier->subdevs[this_index];
>> -		struct v4l2_async_subdev *other_asd = notifier->subdevs[j];
>> +		struct v4l2_async_subdev *asd_y = notifier->subdevs[j];
>>   
>> -		if (other_asd->match_type == V4L2_ASYNC_MATCH_FWNODE &&
>> -		    asd->match.fwnode ==
>> -		    other_asd->match.fwnode)
>> +		if (asd_equal(asd, asd_y))
>>   			return true;
>>   	}
>>   
>> -	/* Check than an fwnode did not exist in other notifiers. */
>> +	/* Check that an asd does not exist in other notifiers. */
>>   	list_for_each_entry(notifier, &notifier_list, list)
>> -		if (__v4l2_async_notifier_fwnode_has_async_subdev(
>> -			    notifier, fwnode))
>> +		if (__v4l2_async_notifier_has_async_subdev(notifier, asd))
>>   			return true;
>>   
>>   	return false;
>> @@ -392,12 +409,11 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
>>   		case V4L2_ASYNC_MATCH_CUSTOM:
>>   		case V4L2_ASYNC_MATCH_DEVNAME:
>>   		case V4L2_ASYNC_MATCH_I2C:
>> -			break;
>>   		case V4L2_ASYNC_MATCH_FWNODE:
>> -			if (v4l2_async_notifier_fwnode_has_async_subdev(
>> -				    notifier, asd->match.fwnode, i)) {
>> +			if (v4l2_async_notifier_has_async_subdev(
>> +				    notifier, asd, i)) {
>>   				dev_err(dev,
>> -					"fwnode has already been registered or in notifier's subdev list\n");
>> +					"asd has already been registered or in notifier's subdev list\n");
>>   				ret = -EEXIST;
>>   				goto err_unlock;
>>   			}
>>

[PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev

[Steve Longerbeam]

v4l2_async_notifier_add_subdev() adds an asd to the notifier. It checks
that the asd's match_type is valid and that no other equivalent asd's
have already been added to this notifier's asd list, or to other
registered notifier's waiting or done lists, and increments num_subdevs.

v4l2_async_notifier_add_subdev() does not make use of the notifier subdevs
array, otherwise it would have to re-allocate the array every time the
function was called. In place of the subdevs array, the function adds
the asd to a new master asd_list. The function will return error with a
WARN() if it is ever called with the subdevs array allocated.

In v4l2_async_notifier_has_async_subdev(), __v4l2_async_notifier_register(),
and v4l2_async_notifier_cleanup(), alternatively operate on the subdevs
array or a non-empty notifier->asd_list.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
Changes since v2:
- add a NULL asd pointer check to v4l2_async_notifier_asd_valid().
Changes since v1:
- none
---
 drivers/media/v4l2-core/v4l2-async.c | 206 +++++++++++++++++++++++++++--------
 include/media/v4l2-async.h           |  22 ++++
 2 files changed, 184 insertions(+), 44 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index b59bbac..7b7f7e2 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -366,16 +366,26 @@ static bool v4l2_async_notifier_has_async_subdev(
 	struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd,
 	unsigned int this_index)
 {
+	struct v4l2_async_subdev *asd_y;
 	unsigned int j;
 
 	lockdep_assert_held(&list_lock);
 
 	/* Check that an asd is not being added more than once. */
-	for (j = 0; j < this_index; j++) {
-		struct v4l2_async_subdev *asd_y = notifier->subdevs[j];
-
-		if (asd_equal(asd, asd_y))
-			return true;
+	if (notifier->subdevs) {
+		for (j = 0; j < this_index; j++) {
+			asd_y = notifier->subdevs[j];
+			if (asd_equal(asd, asd_y))
+				return true;
+		}
+	} else {
+		j = 0;
+		list_for_each_entry(asd_y, &notifier->asd_list, asd_list) {
+			if (j++ >= this_index)
+				break;
+			if (asd_equal(asd, asd_y))
+				return true;
+		}
 	}
 
 	/* Check that an asd does not exist in other notifiers. */
@@ -386,10 +396,46 @@ static bool v4l2_async_notifier_has_async_subdev(
 	return false;
 }
 
-static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
+static int v4l2_async_notifier_asd_valid(struct v4l2_async_notifier *notifier,
+					 struct v4l2_async_subdev *asd,
+					 unsigned int this_index)
 {
 	struct device *dev =
 		notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
+
+	if (!asd)
+		return -EINVAL;
+
+	switch (asd->match_type) {
+	case V4L2_ASYNC_MATCH_CUSTOM:
+	case V4L2_ASYNC_MATCH_DEVNAME:
+	case V4L2_ASYNC_MATCH_I2C:
+	case V4L2_ASYNC_MATCH_FWNODE:
+		if (v4l2_async_notifier_has_async_subdev(notifier, asd,
+							 this_index))
+			return -EEXIST;
+		break;
+	default:
+		dev_err(dev, "Invalid match type %u on %p\n",
+			asd->match_type, asd);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void __v4l2_async_notifier_init(struct v4l2_async_notifier *notifier)
+{
+	lockdep_assert_held(&list_lock);
+
+	INIT_LIST_HEAD(&notifier->asd_list);
+	INIT_LIST_HEAD(&notifier->waiting);
+	INIT_LIST_HEAD(&notifier->done);
+	notifier->lists_initialized = true;
+}
+
+static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
+{
 	struct v4l2_async_subdev *asd;
 	int ret;
 	int i;
@@ -397,34 +443,40 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
 	if (notifier->num_subdevs > V4L2_MAX_SUBDEVS)
 		return -EINVAL;
 
-	INIT_LIST_HEAD(&notifier->waiting);
-	INIT_LIST_HEAD(&notifier->done);
-
 	mutex_lock(&list_lock);
 
-	for (i = 0; i < notifier->num_subdevs; i++) {
-		asd = notifier->subdevs[i];
+	if (!notifier->lists_initialized)
+		__v4l2_async_notifier_init(notifier);
 
-		switch (asd->match_type) {
-		case V4L2_ASYNC_MATCH_CUSTOM:
-		case V4L2_ASYNC_MATCH_DEVNAME:
-		case V4L2_ASYNC_MATCH_I2C:
-		case V4L2_ASYNC_MATCH_FWNODE:
-			if (v4l2_async_notifier_has_async_subdev(
-				    notifier, asd, i)) {
-				dev_err(dev,
-					"asd has already been registered or in notifier's subdev list\n");
-				ret = -EEXIST;
-				goto err_unlock;
-			}
-			break;
-		default:
-			dev_err(dev, "Invalid match type %u on %p\n",
-				asd->match_type, asd);
+	if (!list_empty(&notifier->asd_list)) {
+		/*
+		 * Caller must have either used v4l2_async_notifier_add_subdev
+		 * to add asd's to notifier->asd_list, or provided the
+		 * notifier->subdevs array, but not both.
+		 */
+		if (WARN_ON(notifier->subdevs)) {
 			ret = -EINVAL;
 			goto err_unlock;
 		}
-		list_add_tail(&asd->list, &notifier->waiting);
+
+		i = 0;
+		list_for_each_entry(asd, &notifier->asd_list, asd_list) {
+			ret = v4l2_async_notifier_asd_valid(notifier, asd, i++);
+			if (ret)
+				goto err_unlock;
+
+			list_add_tail(&asd->list, &notifier->waiting);
+		}
+	} else if (notifier->subdevs) {
+		for (i = 0; i < notifier->num_subdevs; i++) {
+			asd = notifier->subdevs[i];
+
+			ret = v4l2_async_notifier_asd_valid(notifier, asd, i);
+			if (ret)
+				goto err_unlock;
+
+			list_add_tail(&asd->list, &notifier->waiting);
+		}
 	}
 
 	ret = v4l2_async_notifier_try_all_subdevs(notifier);
@@ -514,36 +566,102 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 }
 EXPORT_SYMBOL(v4l2_async_notifier_unregister);
 
-void v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
+static void __v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
 {
+	struct v4l2_async_subdev *asd, *tmp;
 	unsigned int i;
 
-	if (!notifier || !notifier->max_subdevs)
+	if (!notifier)
 		return;
 
-	for (i = 0; i < notifier->num_subdevs; i++) {
-		struct v4l2_async_subdev *asd = notifier->subdevs[i];
+	if (notifier->subdevs) {
+		if (!notifier->max_subdevs)
+			return;
 
-		switch (asd->match_type) {
-		case V4L2_ASYNC_MATCH_FWNODE:
-			fwnode_handle_put(asd->match.fwnode);
-			break;
-		default:
-			WARN_ON_ONCE(true);
-			break;
+		for (i = 0; i < notifier->num_subdevs; i++) {
+			asd = notifier->subdevs[i];
+
+			switch (asd->match_type) {
+			case V4L2_ASYNC_MATCH_FWNODE:
+				fwnode_handle_put(asd->match.fwnode);
+				break;
+			default:
+				break;
+			}
+
+			kfree(asd);
 		}
 
-		kfree(asd);
+		notifier->max_subdevs = 0;
+		kvfree(notifier->subdevs);
+		notifier->subdevs = NULL;
+	} else if (notifier->lists_initialized) {
+		list_for_each_entry_safe(asd, tmp,
+					 &notifier->asd_list, asd_list) {
+			switch (asd->match_type) {
+			case V4L2_ASYNC_MATCH_FWNODE:
+				fwnode_handle_put(asd->match.fwnode);
+				break;
+			default:
+				break;
+			}
+
+			list_del(&asd->asd_list);
+			kfree(asd);
+		}
 	}
 
-	notifier->max_subdevs = 0;
 	notifier->num_subdevs = 0;
+}
 
-	kvfree(notifier->subdevs);
-	notifier->subdevs = NULL;
+void v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
+{
+	mutex_lock(&list_lock);
+
+	__v4l2_async_notifier_cleanup(notifier);
+
+	mutex_unlock(&list_lock);
 }
 EXPORT_SYMBOL_GPL(v4l2_async_notifier_cleanup);
 
+int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
+				   struct v4l2_async_subdev *asd)
+{
+	int ret = 0;
+
+	mutex_lock(&list_lock);
+
+	if (notifier->num_subdevs >= V4L2_MAX_SUBDEVS) {
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	if (!notifier->lists_initialized)
+		__v4l2_async_notifier_init(notifier);
+
+	/*
+	 * If caller uses this function, it cannot also allocate and
+	 * place asd's in the notifier->subdevs array.
+	 */
+	if (WARN_ON(notifier->subdevs)) {
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	ret = v4l2_async_notifier_asd_valid(notifier, asd,
+					    notifier->num_subdevs);
+	if (ret)
+		goto unlock;
+
+	list_add_tail(&asd->asd_list, &notifier->asd_list);
+	notifier->num_subdevs++;
+
+unlock:
+	mutex_unlock(&list_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_async_notifier_add_subdev);
+
 int v4l2_async_register_subdev(struct v4l2_subdev *sd)
 {
 	struct v4l2_async_notifier *subdev_notifier;
@@ -617,7 +735,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
 	mutex_lock(&list_lock);
 
 	__v4l2_async_notifier_unregister(sd->subdev_notifier);
-	v4l2_async_notifier_cleanup(sd->subdev_notifier);
+	__v4l2_async_notifier_cleanup(sd->subdev_notifier);
 	kfree(sd->subdev_notifier);
 	sd->subdev_notifier = NULL;
 
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index 1592d32..fa05905 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -73,6 +73,8 @@ enum v4l2_async_match_type {
  * @match.custom.priv:
  *		Driver-specific private struct with match parameters
  *		to be used if %V4L2_ASYNC_MATCH_CUSTOM.
+ * @asd_list:	used to add struct v4l2_async_subdev objects to the
+ *		master notifier->asd_list
  * @list:	used to link struct v4l2_async_subdev objects, waiting to be
  *		probed, to a notifier->waiting list
  *
@@ -98,6 +100,7 @@ struct v4l2_async_subdev {
 
 	/* v4l2-async core private: not to be used by drivers */
 	struct list_head list;
+	struct list_head asd_list;
 };
 
 /**
@@ -127,9 +130,11 @@ struct v4l2_async_notifier_operations {
  * @v4l2_dev:	v4l2_device of the root notifier, NULL otherwise
  * @sd:		sub-device that registered the notifier, NULL otherwise
  * @parent:	parent notifier
+ * @asd_list:	master list of struct v4l2_async_subdev, replaces @subdevs
  * @waiting:	list of struct v4l2_async_subdev, waiting for their drivers
  * @done:	list of struct v4l2_subdev, already probed
  * @list:	member in a global list of notifiers
+ * @lists_initialized: list_head's have been initialized
  */
 struct v4l2_async_notifier {
 	const struct v4l2_async_notifier_operations *ops;
@@ -139,12 +144,29 @@ struct v4l2_async_notifier {
 	struct v4l2_device *v4l2_dev;
 	struct v4l2_subdev *sd;
 	struct v4l2_async_notifier *parent;
+	struct list_head asd_list;
 	struct list_head waiting;
 	struct list_head done;
 	struct list_head list;
+	bool lists_initialized;
 };
 
 /**
+ * v4l2_async_notifier_add_subdev - Add an async subdev to the
+ *				notifier's master asd_list.
+ *
+ * @notifier: pointer to &struct v4l2_async_notifier
+ * @asd: pointer to &struct v4l2_async_subdev
+ *
+ * This can be used before registering a notifier to add an
+ * asd to the notifiers master asd_list. If the caller uses
+ * this method to compose an asd list, it must never allocate
+ * or place asd's in the @subdevs array.
+ */
+int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
+				   struct v4l2_async_subdev *asd);
+
+/**
  * v4l2_async_notifier_register - registers a subdevice asynchronous notifier
  *
  * @v4l2_dev: pointer to &struct v4l2_device
-- 
2.7.4

Re: [PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev

[Sakari Ailus]

Hi Steve,

Thanks for the patchset.

On Tue, Mar 20, 2018 at 05:37:19PM -0700, Steve Longerbeam wrote:
> v4l2_async_notifier_add_subdev() adds an asd to the notifier. It checks
> that the asd's match_type is valid and that no other equivalent asd's
> have already been added to this notifier's asd list, or to other
> registered notifier's waiting or done lists, and increments num_subdevs.
> 
> v4l2_async_notifier_add_subdev() does not make use of the notifier subdevs
> array, otherwise it would have to re-allocate the array every time the
> function was called. In place of the subdevs array, the function adds
> the asd to a new master asd_list. The function will return error with a
> WARN() if it is ever called with the subdevs array allocated.
> 
> In v4l2_async_notifier_has_async_subdev(), __v4l2_async_notifier_register(),
> and v4l2_async_notifier_cleanup(), alternatively operate on the subdevs
> array or a non-empty notifier->asd_list.

I do agree with the approach, but this patch leaves the remaining users of
the subdevs array in the notifier intact. Could you rework them to use the
v4l2_async_notifier_add_subdev() as well?

There seem to be just a few of them --- exynos4-is and rcar_drif.

> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
> Changes since v2:
> - add a NULL asd pointer check to v4l2_async_notifier_asd_valid().
> Changes since v1:
> - none
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 206 +++++++++++++++++++++++++++--------
>  include/media/v4l2-async.h           |  22 ++++
>  2 files changed, 184 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index b59bbac..7b7f7e2 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -366,16 +366,26 @@ static bool v4l2_async_notifier_has_async_subdev(
>  	struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd,
>  	unsigned int this_index)
>  {
> +	struct v4l2_async_subdev *asd_y;
>  	unsigned int j;
>  
>  	lockdep_assert_held(&list_lock);
>  
>  	/* Check that an asd is not being added more than once. */
> -	for (j = 0; j < this_index; j++) {
> -		struct v4l2_async_subdev *asd_y = notifier->subdevs[j];
> -
> -		if (asd_equal(asd, asd_y))
> -			return true;
> +	if (notifier->subdevs) {
> +		for (j = 0; j < this_index; j++) {
> +			asd_y = notifier->subdevs[j];
> +			if (asd_equal(asd, asd_y))
> +				return true;
> +		}
> +	} else {
> +		j = 0;
> +		list_for_each_entry(asd_y, &notifier->asd_list, asd_list) {
> +			if (j++ >= this_index)
> +				break;
> +			if (asd_equal(asd, asd_y))
> +				return true;
> +		}
>  	}
>  
>  	/* Check that an asd does not exist in other notifiers. */
> @@ -386,10 +396,46 @@ static bool v4l2_async_notifier_has_async_subdev(
>  	return false;
>  }
>  
> -static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> +static int v4l2_async_notifier_asd_valid(struct v4l2_async_notifier *notifier,
> +					 struct v4l2_async_subdev *asd,
> +					 unsigned int this_index)
>  {
>  	struct device *dev =
>  		notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
> +
> +	if (!asd)
> +		return -EINVAL;
> +
> +	switch (asd->match_type) {
> +	case V4L2_ASYNC_MATCH_CUSTOM:
> +	case V4L2_ASYNC_MATCH_DEVNAME:
> +	case V4L2_ASYNC_MATCH_I2C:
> +	case V4L2_ASYNC_MATCH_FWNODE:
> +		if (v4l2_async_notifier_has_async_subdev(notifier, asd,
> +							 this_index))
> +			return -EEXIST;
> +		break;
> +	default:
> +		dev_err(dev, "Invalid match type %u on %p\n",
> +			asd->match_type, asd);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void __v4l2_async_notifier_init(struct v4l2_async_notifier *notifier)
> +{
> +	lockdep_assert_held(&list_lock);
> +
> +	INIT_LIST_HEAD(&notifier->asd_list);
> +	INIT_LIST_HEAD(&notifier->waiting);
> +	INIT_LIST_HEAD(&notifier->done);
> +	notifier->lists_initialized = true;
> +}
> +
> +static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> +{
>  	struct v4l2_async_subdev *asd;
>  	int ret;
>  	int i;
> @@ -397,34 +443,40 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
>  	if (notifier->num_subdevs > V4L2_MAX_SUBDEVS)
>  		return -EINVAL;
>  
> -	INIT_LIST_HEAD(&notifier->waiting);
> -	INIT_LIST_HEAD(&notifier->done);
> -
>  	mutex_lock(&list_lock);
>  
> -	for (i = 0; i < notifier->num_subdevs; i++) {
> -		asd = notifier->subdevs[i];
> +	if (!notifier->lists_initialized)
> +		__v4l2_async_notifier_init(notifier);
>  
> -		switch (asd->match_type) {
> -		case V4L2_ASYNC_MATCH_CUSTOM:
> -		case V4L2_ASYNC_MATCH_DEVNAME:
> -		case V4L2_ASYNC_MATCH_I2C:
> -		case V4L2_ASYNC_MATCH_FWNODE:
> -			if (v4l2_async_notifier_has_async_subdev(
> -				    notifier, asd, i)) {
> -				dev_err(dev,
> -					"asd has already been registered or in notifier's subdev list\n");
> -				ret = -EEXIST;
> -				goto err_unlock;
> -			}
> -			break;
> -		default:
> -			dev_err(dev, "Invalid match type %u on %p\n",
> -				asd->match_type, asd);
> +	if (!list_empty(&notifier->asd_list)) {
> +		/*
> +		 * Caller must have either used v4l2_async_notifier_add_subdev
> +		 * to add asd's to notifier->asd_list, or provided the
> +		 * notifier->subdevs array, but not both.
> +		 */
> +		if (WARN_ON(notifier->subdevs)) {
>  			ret = -EINVAL;
>  			goto err_unlock;
>  		}
> -		list_add_tail(&asd->list, &notifier->waiting);
> +
> +		i = 0;
> +		list_for_each_entry(asd, &notifier->asd_list, asd_list) {
> +			ret = v4l2_async_notifier_asd_valid(notifier, asd, i++);
> +			if (ret)
> +				goto err_unlock;
> +
> +			list_add_tail(&asd->list, &notifier->waiting);
> +		}
> +	} else if (notifier->subdevs) {
> +		for (i = 0; i < notifier->num_subdevs; i++) {
> +			asd = notifier->subdevs[i];
> +
> +			ret = v4l2_async_notifier_asd_valid(notifier, asd, i);
> +			if (ret)
> +				goto err_unlock;
> +
> +			list_add_tail(&asd->list, &notifier->waiting);
> +		}
>  	}
>  
>  	ret = v4l2_async_notifier_try_all_subdevs(notifier);
> @@ -514,36 +566,102 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>  }
>  EXPORT_SYMBOL(v4l2_async_notifier_unregister);
>  
> -void v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
> +static void __v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
>  {
> +	struct v4l2_async_subdev *asd, *tmp;
>  	unsigned int i;
>  
> -	if (!notifier || !notifier->max_subdevs)
> +	if (!notifier)
>  		return;
>  
> -	for (i = 0; i < notifier->num_subdevs; i++) {
> -		struct v4l2_async_subdev *asd = notifier->subdevs[i];
> +	if (notifier->subdevs) {
> +		if (!notifier->max_subdevs)
> +			return;
>  
> -		switch (asd->match_type) {
> -		case V4L2_ASYNC_MATCH_FWNODE:
> -			fwnode_handle_put(asd->match.fwnode);
> -			break;
> -		default:
> -			WARN_ON_ONCE(true);
> -			break;
> +		for (i = 0; i < notifier->num_subdevs; i++) {
> +			asd = notifier->subdevs[i];
> +
> +			switch (asd->match_type) {
> +			case V4L2_ASYNC_MATCH_FWNODE:
> +				fwnode_handle_put(asd->match.fwnode);
> +				break;
> +			default:
> +				break;
> +			}
> +
> +			kfree(asd);
>  		}
>  
> -		kfree(asd);
> +		notifier->max_subdevs = 0;
> +		kvfree(notifier->subdevs);
> +		notifier->subdevs = NULL;
> +	} else if (notifier->lists_initialized) {
> +		list_for_each_entry_safe(asd, tmp,
> +					 &notifier->asd_list, asd_list) {
> +			switch (asd->match_type) {
> +			case V4L2_ASYNC_MATCH_FWNODE:
> +				fwnode_handle_put(asd->match.fwnode);
> +				break;
> +			default:
> +				break;
> +			}
> +
> +			list_del(&asd->asd_list);
> +			kfree(asd);
> +		}
>  	}
>  
> -	notifier->max_subdevs = 0;
>  	notifier->num_subdevs = 0;
> +}
>  
> -	kvfree(notifier->subdevs);
> -	notifier->subdevs = NULL;
> +void v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
> +{
> +	mutex_lock(&list_lock);
> +
> +	__v4l2_async_notifier_cleanup(notifier);
> +
> +	mutex_unlock(&list_lock);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_async_notifier_cleanup);
>  
> +int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
> +				   struct v4l2_async_subdev *asd)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&list_lock);
> +
> +	if (notifier->num_subdevs >= V4L2_MAX_SUBDEVS) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	if (!notifier->lists_initialized)
> +		__v4l2_async_notifier_init(notifier);
> +
> +	/*
> +	 * If caller uses this function, it cannot also allocate and
> +	 * place asd's in the notifier->subdevs array.
> +	 */
> +	if (WARN_ON(notifier->subdevs)) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	ret = v4l2_async_notifier_asd_valid(notifier, asd,
> +					    notifier->num_subdevs);
> +	if (ret)
> +		goto unlock;
> +
> +	list_add_tail(&asd->asd_list, &notifier->asd_list);
> +	notifier->num_subdevs++;
> +
> +unlock:
> +	mutex_unlock(&list_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_add_subdev);
> +
>  int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>  {
>  	struct v4l2_async_notifier *subdev_notifier;
> @@ -617,7 +735,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>  	mutex_lock(&list_lock);
>  
>  	__v4l2_async_notifier_unregister(sd->subdev_notifier);
> -	v4l2_async_notifier_cleanup(sd->subdev_notifier);
> +	__v4l2_async_notifier_cleanup(sd->subdev_notifier);
>  	kfree(sd->subdev_notifier);
>  	sd->subdev_notifier = NULL;
>  
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 1592d32..fa05905 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -73,6 +73,8 @@ enum v4l2_async_match_type {
>   * @match.custom.priv:
>   *		Driver-specific private struct with match parameters
>   *		to be used if %V4L2_ASYNC_MATCH_CUSTOM.
> + * @asd_list:	used to add struct v4l2_async_subdev objects to the
> + *		master notifier->asd_list
>   * @list:	used to link struct v4l2_async_subdev objects, waiting to be
>   *		probed, to a notifier->waiting list
>   *
> @@ -98,6 +100,7 @@ struct v4l2_async_subdev {
>  
>  	/* v4l2-async core private: not to be used by drivers */
>  	struct list_head list;
> +	struct list_head asd_list;
>  };
>  
>  /**
> @@ -127,9 +130,11 @@ struct v4l2_async_notifier_operations {
>   * @v4l2_dev:	v4l2_device of the root notifier, NULL otherwise
>   * @sd:		sub-device that registered the notifier, NULL otherwise
>   * @parent:	parent notifier
> + * @asd_list:	master list of struct v4l2_async_subdev, replaces @subdevs
>   * @waiting:	list of struct v4l2_async_subdev, waiting for their drivers
>   * @done:	list of struct v4l2_subdev, already probed
>   * @list:	member in a global list of notifiers
> + * @lists_initialized: list_head's have been initialized
>   */
>  struct v4l2_async_notifier {
>  	const struct v4l2_async_notifier_operations *ops;
> @@ -139,12 +144,29 @@ struct v4l2_async_notifier {
>  	struct v4l2_device *v4l2_dev;
>  	struct v4l2_subdev *sd;
>  	struct v4l2_async_notifier *parent;
> +	struct list_head asd_list;
>  	struct list_head waiting;
>  	struct list_head done;
>  	struct list_head list;
> +	bool lists_initialized;
>  };
>  
>  /**
> + * v4l2_async_notifier_add_subdev - Add an async subdev to the
> + *				notifier's master asd_list.
> + *
> + * @notifier: pointer to &struct v4l2_async_notifier
> + * @asd: pointer to &struct v4l2_async_subdev
> + *
> + * This can be used before registering a notifier to add an
> + * asd to the notifiers master asd_list. If the caller uses
> + * this method to compose an asd list, it must never allocate
> + * or place asd's in the @subdevs array.
> + */
> +int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
> +				   struct v4l2_async_subdev *asd);
> +
> +/**
>   * v4l2_async_notifier_register - registers a subdevice asynchronous notifier
>   *
>   * @v4l2_dev: pointer to &struct v4l2_device
> -- 
> 2.7.4
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev

[Steve Longerbeam]

Hi Sakari,


On 04/20/2018 05:24 AM, Sakari Ailus wrote:
> Hi Steve,
>
> Thanks for the patchset.
>
> On Tue, Mar 20, 2018 at 05:37:19PM -0700, Steve Longerbeam wrote:
>> v4l2_async_notifier_add_subdev() adds an asd to the notifier. It checks
>> that the asd's match_type is valid and that no other equivalent asd's
>> have already been added to this notifier's asd list, or to other
>> registered notifier's waiting or done lists, and increments num_subdevs.
>>
>> v4l2_async_notifier_add_subdev() does not make use of the notifier subdevs
>> array, otherwise it would have to re-allocate the array every time the
>> function was called. In place of the subdevs array, the function adds
>> the asd to a new master asd_list. The function will return error with a
>> WARN() if it is ever called with the subdevs array allocated.
>>
>> In v4l2_async_notifier_has_async_subdev(), __v4l2_async_notifier_register(),
>> and v4l2_async_notifier_cleanup(), alternatively operate on the subdevs
>> array or a non-empty notifier->asd_list.
> I do agree with the approach, but this patch leaves the remaining users of
> the subdevs array in the notifier intact. Could you rework them to use the
> v4l2_async_notifier_add_subdev() as well?
>
> There seem to be just a few of them --- exynos4-is and rcar_drif.

I count more than a few :)

% cd drivers/media && grep -l -r --include "*.[ch]" 
'notifier[\.\-]>*subdevs[   ]*='
v4l2-core/v4l2-async.c
platform/pxa_camera.c
platform/ti-vpe/cal.c
platform/exynos4-is/media-dev.c
platform/qcom/camss-8x16/camss.c
platform/soc_camera/soc_camera.c
platform/atmel/atmel-isi.c
platform/atmel/atmel-isc.c
platform/stm32/stm32-dcmi.c
platform/davinci/vpif_capture.c
platform/davinci/vpif_display.c
platform/renesas-ceu.c
platform/am437x/am437x-vpfe.c
platform/xilinx/xilinx-vipp.c
platform/rcar_drif.c


So not including v4l2-core, the list is:

pxa_camera
ti-vpe
exynos4-is
qcom
soc_camera
atmel
stm32
davinci
renesas-ceu
am437x
xilinx
rcar_drif


Given such a large list of the users of the notifier->subdevs array,
I think this should be done is two steps: submit this patchset first,
that keeps the backward compatibility, and then a subsequent patchset
that converts all drivers to use v4l2_async_notifier_add_subdev(), at
which point the subdevs array can be removed from struct 
v4l2_async_notifier.

Or, do you still think this should be done all at once? I could add a
large patch to this patchset that does the conversion and removes
the subdevs array.

Steve


>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> ---
>> Changes since v2:
>> - add a NULL asd pointer check to v4l2_async_notifier_asd_valid().
>> Changes since v1:
>> - none
>> ---
>>   drivers/media/v4l2-core/v4l2-async.c | 206 +++++++++++++++++++++++++++--------
>>   include/media/v4l2-async.h           |  22 ++++
>>   2 files changed, 184 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>> index b59bbac..7b7f7e2 100644
>> --- a/drivers/media/v4l2-core/v4l2-async.c
>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>> @@ -366,16 +366,26 @@ static bool v4l2_async_notifier_has_async_subdev(
>>   	struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd,
>>   	unsigned int this_index)
>>   {
>> +	struct v4l2_async_subdev *asd_y;
>>   	unsigned int j;
>>   
>>   	lockdep_assert_held(&list_lock);
>>   
>>   	/* Check that an asd is not being added more than once. */
>> -	for (j = 0; j < this_index; j++) {
>> -		struct v4l2_async_subdev *asd_y = notifier->subdevs[j];
>> -
>> -		if (asd_equal(asd, asd_y))
>> -			return true;
>> +	if (notifier->subdevs) {
>> +		for (j = 0; j < this_index; j++) {
>> +			asd_y = notifier->subdevs[j];
>> +			if (asd_equal(asd, asd_y))
>> +				return true;
>> +		}
>> +	} else {
>> +		j = 0;
>> +		list_for_each_entry(asd_y, &notifier->asd_list, asd_list) {
>> +			if (j++ >= this_index)
>> +				break;
>> +			if (asd_equal(asd, asd_y))
>> +				return true;
>> +		}
>>   	}
>>   
>>   	/* Check that an asd does not exist in other notifiers. */
>> @@ -386,10 +396,46 @@ static bool v4l2_async_notifier_has_async_subdev(
>>   	return false;
>>   }
>>   
>> -static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
>> +static int v4l2_async_notifier_asd_valid(struct v4l2_async_notifier *notifier,
>> +					 struct v4l2_async_subdev *asd,
>> +					 unsigned int this_index)
>>   {
>>   	struct device *dev =
>>   		notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
>> +
>> +	if (!asd)
>> +		return -EINVAL;
>> +
>> +	switch (asd->match_type) {
>> +	case V4L2_ASYNC_MATCH_CUSTOM:
>> +	case V4L2_ASYNC_MATCH_DEVNAME:
>> +	case V4L2_ASYNC_MATCH_I2C:
>> +	case V4L2_ASYNC_MATCH_FWNODE:
>> +		if (v4l2_async_notifier_has_async_subdev(notifier, asd,
>> +							 this_index))
>> +			return -EEXIST;
>> +		break;
>> +	default:
>> +		dev_err(dev, "Invalid match type %u on %p\n",
>> +			asd->match_type, asd);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void __v4l2_async_notifier_init(struct v4l2_async_notifier *notifier)
>> +{
>> +	lockdep_assert_held(&list_lock);
>> +
>> +	INIT_LIST_HEAD(&notifier->asd_list);
>> +	INIT_LIST_HEAD(&notifier->waiting);
>> +	INIT_LIST_HEAD(&notifier->done);
>> +	notifier->lists_initialized = true;
>> +}
>> +
>> +static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
>> +{
>>   	struct v4l2_async_subdev *asd;
>>   	int ret;
>>   	int i;
>> @@ -397,34 +443,40 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
>>   	if (notifier->num_subdevs > V4L2_MAX_SUBDEVS)
>>   		return -EINVAL;
>>   
>> -	INIT_LIST_HEAD(&notifier->waiting);
>> -	INIT_LIST_HEAD(&notifier->done);
>> -
>>   	mutex_lock(&list_lock);
>>   
>> -	for (i = 0; i < notifier->num_subdevs; i++) {
>> -		asd = notifier->subdevs[i];
>> +	if (!notifier->lists_initialized)
>> +		__v4l2_async_notifier_init(notifier);
>>   
>> -		switch (asd->match_type) {
>> -		case V4L2_ASYNC_MATCH_CUSTOM:
>> -		case V4L2_ASYNC_MATCH_DEVNAME:
>> -		case V4L2_ASYNC_MATCH_I2C:
>> -		case V4L2_ASYNC_MATCH_FWNODE:
>> -			if (v4l2_async_notifier_has_async_subdev(
>> -				    notifier, asd, i)) {
>> -				dev_err(dev,
>> -					"asd has already been registered or in notifier's subdev list\n");
>> -				ret = -EEXIST;
>> -				goto err_unlock;
>> -			}
>> -			break;
>> -		default:
>> -			dev_err(dev, "Invalid match type %u on %p\n",
>> -				asd->match_type, asd);
>> +	if (!list_empty(&notifier->asd_list)) {
>> +		/*
>> +		 * Caller must have either used v4l2_async_notifier_add_subdev
>> +		 * to add asd's to notifier->asd_list, or provided the
>> +		 * notifier->subdevs array, but not both.
>> +		 */
>> +		if (WARN_ON(notifier->subdevs)) {
>>   			ret = -EINVAL;
>>   			goto err_unlock;
>>   		}
>> -		list_add_tail(&asd->list, &notifier->waiting);
>> +
>> +		i = 0;
>> +		list_for_each_entry(asd, &notifier->asd_list, asd_list) {
>> +			ret = v4l2_async_notifier_asd_valid(notifier, asd, i++);
>> +			if (ret)
>> +				goto err_unlock;
>> +
>> +			list_add_tail(&asd->list, &notifier->waiting);
>> +		}
>> +	} else if (notifier->subdevs) {
>> +		for (i = 0; i < notifier->num_subdevs; i++) {
>> +			asd = notifier->subdevs[i];
>> +
>> +			ret = v4l2_async_notifier_asd_valid(notifier, asd, i);
>> +			if (ret)
>> +				goto err_unlock;
>> +
>> +			list_add_tail(&asd->list, &notifier->waiting);
>> +		}
>>   	}
>>   
>>   	ret = v4l2_async_notifier_try_all_subdevs(notifier);
>> @@ -514,36 +566,102 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>>   }
>>   EXPORT_SYMBOL(v4l2_async_notifier_unregister);
>>   
>> -void v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
>> +static void __v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
>>   {
>> +	struct v4l2_async_subdev *asd, *tmp;
>>   	unsigned int i;
>>   
>> -	if (!notifier || !notifier->max_subdevs)
>> +	if (!notifier)
>>   		return;
>>   
>> -	for (i = 0; i < notifier->num_subdevs; i++) {
>> -		struct v4l2_async_subdev *asd = notifier->subdevs[i];
>> +	if (notifier->subdevs) {
>> +		if (!notifier->max_subdevs)
>> +			return;
>>   
>> -		switch (asd->match_type) {
>> -		case V4L2_ASYNC_MATCH_FWNODE:
>> -			fwnode_handle_put(asd->match.fwnode);
>> -			break;
>> -		default:
>> -			WARN_ON_ONCE(true);
>> -			break;
>> +		for (i = 0; i < notifier->num_subdevs; i++) {
>> +			asd = notifier->subdevs[i];
>> +
>> +			switch (asd->match_type) {
>> +			case V4L2_ASYNC_MATCH_FWNODE:
>> +				fwnode_handle_put(asd->match.fwnode);
>> +				break;
>> +			default:
>> +				break;
>> +			}
>> +
>> +			kfree(asd);
>>   		}
>>   
>> -		kfree(asd);
>> +		notifier->max_subdevs = 0;
>> +		kvfree(notifier->subdevs);
>> +		notifier->subdevs = NULL;
>> +	} else if (notifier->lists_initialized) {
>> +		list_for_each_entry_safe(asd, tmp,
>> +					 &notifier->asd_list, asd_list) {
>> +			switch (asd->match_type) {
>> +			case V4L2_ASYNC_MATCH_FWNODE:
>> +				fwnode_handle_put(asd->match.fwnode);
>> +				break;
>> +			default:
>> +				break;
>> +			}
>> +
>> +			list_del(&asd->asd_list);
>> +			kfree(asd);
>> +		}
>>   	}
>>   
>> -	notifier->max_subdevs = 0;
>>   	notifier->num_subdevs = 0;
>> +}
>>   
>> -	kvfree(notifier->subdevs);
>> -	notifier->subdevs = NULL;
>> +void v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
>> +{
>> +	mutex_lock(&list_lock);
>> +
>> +	__v4l2_async_notifier_cleanup(notifier);
>> +
>> +	mutex_unlock(&list_lock);
>>   }
>>   EXPORT_SYMBOL_GPL(v4l2_async_notifier_cleanup);
>>   
>> +int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
>> +				   struct v4l2_async_subdev *asd)
>> +{
>> +	int ret = 0;
>> +
>> +	mutex_lock(&list_lock);
>> +
>> +	if (notifier->num_subdevs >= V4L2_MAX_SUBDEVS) {
>> +		ret = -EINVAL;
>> +		goto unlock;
>> +	}
>> +
>> +	if (!notifier->lists_initialized)
>> +		__v4l2_async_notifier_init(notifier);
>> +
>> +	/*
>> +	 * If caller uses this function, it cannot also allocate and
>> +	 * place asd's in the notifier->subdevs array.
>> +	 */
>> +	if (WARN_ON(notifier->subdevs)) {
>> +		ret = -EINVAL;
>> +		goto unlock;
>> +	}
>> +
>> +	ret = v4l2_async_notifier_asd_valid(notifier, asd,
>> +					    notifier->num_subdevs);
>> +	if (ret)
>> +		goto unlock;
>> +
>> +	list_add_tail(&asd->asd_list, &notifier->asd_list);
>> +	notifier->num_subdevs++;
>> +
>> +unlock:
>> +	mutex_unlock(&list_lock);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_add_subdev);
>> +
>>   int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>>   {
>>   	struct v4l2_async_notifier *subdev_notifier;
>> @@ -617,7 +735,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>>   	mutex_lock(&list_lock);
>>   
>>   	__v4l2_async_notifier_unregister(sd->subdev_notifier);
>> -	v4l2_async_notifier_cleanup(sd->subdev_notifier);
>> +	__v4l2_async_notifier_cleanup(sd->subdev_notifier);
>>   	kfree(sd->subdev_notifier);
>>   	sd->subdev_notifier = NULL;
>>   
>> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
>> index 1592d32..fa05905 100644
>> --- a/include/media/v4l2-async.h
>> +++ b/include/media/v4l2-async.h
>> @@ -73,6 +73,8 @@ enum v4l2_async_match_type {
>>    * @match.custom.priv:
>>    *		Driver-specific private struct with match parameters
>>    *		to be used if %V4L2_ASYNC_MATCH_CUSTOM.
>> + * @asd_list:	used to add struct v4l2_async_subdev objects to the
>> + *		master notifier->asd_list
>>    * @list:	used to link struct v4l2_async_subdev objects, waiting to be
>>    *		probed, to a notifier->waiting list
>>    *
>> @@ -98,6 +100,7 @@ struct v4l2_async_subdev {
>>   
>>   	/* v4l2-async core private: not to be used by drivers */
>>   	struct list_head list;
>> +	struct list_head asd_list;
>>   };
>>   
>>   /**
>> @@ -127,9 +130,11 @@ struct v4l2_async_notifier_operations {
>>    * @v4l2_dev:	v4l2_device of the root notifier, NULL otherwise
>>    * @sd:		sub-device that registered the notifier, NULL otherwise
>>    * @parent:	parent notifier
>> + * @asd_list:	master list of struct v4l2_async_subdev, replaces @subdevs
>>    * @waiting:	list of struct v4l2_async_subdev, waiting for their drivers
>>    * @done:	list of struct v4l2_subdev, already probed
>>    * @list:	member in a global list of notifiers
>> + * @lists_initialized: list_head's have been initialized
>>    */
>>   struct v4l2_async_notifier {
>>   	const struct v4l2_async_notifier_operations *ops;
>> @@ -139,12 +144,29 @@ struct v4l2_async_notifier {
>>   	struct v4l2_device *v4l2_dev;
>>   	struct v4l2_subdev *sd;
>>   	struct v4l2_async_notifier *parent;
>> +	struct list_head asd_list;
>>   	struct list_head waiting;
>>   	struct list_head done;
>>   	struct list_head list;
>> +	bool lists_initialized;
>>   };
>>   
>>   /**
>> + * v4l2_async_notifier_add_subdev - Add an async subdev to the
>> + *				notifier's master asd_list.
>> + *
>> + * @notifier: pointer to &struct v4l2_async_notifier
>> + * @asd: pointer to &struct v4l2_async_subdev
>> + *
>> + * This can be used before registering a notifier to add an
>> + * asd to the notifiers master asd_list. If the caller uses
>> + * this method to compose an asd list, it must never allocate
>> + * or place asd's in the @subdevs array.
>> + */
>> +int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
>> +				   struct v4l2_async_subdev *asd);
>> +
>> +/**
>>    * v4l2_async_notifier_register - registers a subdevice asynchronous notifier
>>    *
>>    * @v4l2_dev: pointer to &struct v4l2_device
>> -- 
>> 2.7.4
>>

Re: [PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev

[Sakari Ailus]

On Fri, Apr 20, 2018 at 10:12:33AM -0700, Steve Longerbeam wrote:
> Hi Sakari,
> 
> 
> On 04/20/2018 05:24 AM, Sakari Ailus wrote:
> > Hi Steve,
> > 
> > Thanks for the patchset.
> > 
> > On Tue, Mar 20, 2018 at 05:37:19PM -0700, Steve Longerbeam wrote:
> > > v4l2_async_notifier_add_subdev() adds an asd to the notifier. It checks
> > > that the asd's match_type is valid and that no other equivalent asd's
> > > have already been added to this notifier's asd list, or to other
> > > registered notifier's waiting or done lists, and increments num_subdevs.
> > > 
> > > v4l2_async_notifier_add_subdev() does not make use of the notifier subdevs
> > > array, otherwise it would have to re-allocate the array every time the
> > > function was called. In place of the subdevs array, the function adds
> > > the asd to a new master asd_list. The function will return error with a
> > > WARN() if it is ever called with the subdevs array allocated.
> > > 
> > > In v4l2_async_notifier_has_async_subdev(), __v4l2_async_notifier_register(),
> > > and v4l2_async_notifier_cleanup(), alternatively operate on the subdevs
> > > array or a non-empty notifier->asd_list.
> > I do agree with the approach, but this patch leaves the remaining users of
> > the subdevs array in the notifier intact. Could you rework them to use the
> > v4l2_async_notifier_add_subdev() as well?
> > 
> > There seem to be just a few of them --- exynos4-is and rcar_drif.
> 
> I count more than a few :)
> 
> % cd drivers/media && grep -l -r --include "*.[ch]"
> 'notifier[\.\-]>*subdevs[   ]*='
> v4l2-core/v4l2-async.c
> platform/pxa_camera.c
> platform/ti-vpe/cal.c
> platform/exynos4-is/media-dev.c
> platform/qcom/camss-8x16/camss.c
> platform/soc_camera/soc_camera.c
> platform/atmel/atmel-isi.c
> platform/atmel/atmel-isc.c
> platform/stm32/stm32-dcmi.c
> platform/davinci/vpif_capture.c
> platform/davinci/vpif_display.c
> platform/renesas-ceu.c
> platform/am437x/am437x-vpfe.c
> platform/xilinx/xilinx-vipp.c
> platform/rcar_drif.c
> 
> 
> So not including v4l2-core, the list is:
> 
> pxa_camera
> ti-vpe
> exynos4-is
> qcom
> soc_camera
> atmel
> stm32
> davinci
> renesas-ceu
> am437x
> xilinx
> rcar_drif
> 
> 
> Given such a large list of the users of the notifier->subdevs array,
> I think this should be done is two steps: submit this patchset first,
> that keeps the backward compatibility, and then a subsequent patchset
> that converts all drivers to use v4l2_async_notifier_add_subdev(), at
> which point the subdevs array can be removed from struct
> v4l2_async_notifier.
> 
> Or, do you still think this should be done all at once? I could add a
> large patch to this patchset that does the conversion and removes
> the subdevs array.

Sorry for the delay. I grepped for this, too, but I guess I ended up using
an expression that missed most of the users. :-(

I think it'd be very good to perform that conversion --- the code in the
framework would be quite a bit cleaner and easier to maintain whereas the
per-driver conversions seem pretty simple, such as this on in
drivers/media/platform/atmel/atmel-isi.c :

	/* Register the subdevices notifier. */
	subdevs = devm_kzalloc(isi->dev, sizeof(*subdevs), GFP_KERNEL);
	if (!subdevs) {
	        of_node_put(isi->entity.node);
		return -ENOMEM;
	}

	subdevs[0] = &isi->entity.asd;

	isi->notifier.subdevs = subdevs;
	isi->notifier.num_subdevs = 1;
	isi->notifier.ops = &isi_graph_notify_ops;

> 
> Steve
> 
> 
> > 
> > > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> > > ---
> > > Changes since v2:
> > > - add a NULL asd pointer check to v4l2_async_notifier_asd_valid().
> > > Changes since v1:
> > > - none
> > > ---
> > >   drivers/media/v4l2-core/v4l2-async.c | 206 +++++++++++++++++++++++++++--------
> > >   include/media/v4l2-async.h           |  22 ++++
> > >   2 files changed, 184 insertions(+), 44 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > index b59bbac..7b7f7e2 100644
> > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > @@ -366,16 +366,26 @@ static bool v4l2_async_notifier_has_async_subdev(
> > >   	struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd,
> > >   	unsigned int this_index)
> > >   {
> > > +	struct v4l2_async_subdev *asd_y;
> > >   	unsigned int j;
> > >   	lockdep_assert_held(&list_lock);
> > >   	/* Check that an asd is not being added more than once. */
> > > -	for (j = 0; j < this_index; j++) {
> > > -		struct v4l2_async_subdev *asd_y = notifier->subdevs[j];
> > > -
> > > -		if (asd_equal(asd, asd_y))
> > > -			return true;
> > > +	if (notifier->subdevs) {
> > > +		for (j = 0; j < this_index; j++) {
> > > +			asd_y = notifier->subdevs[j];
> > > +			if (asd_equal(asd, asd_y))
> > > +				return true;
> > > +		}
> > > +	} else {
> > > +		j = 0;
> > > +		list_for_each_entry(asd_y, &notifier->asd_list, asd_list) {
> > > +			if (j++ >= this_index)
> > > +				break;
> > > +			if (asd_equal(asd, asd_y))
> > > +				return true;
> > > +		}
> > >   	}
> > >   	/* Check that an asd does not exist in other notifiers. */
> > > @@ -386,10 +396,46 @@ static bool v4l2_async_notifier_has_async_subdev(
> > >   	return false;
> > >   }
> > > -static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> > > +static int v4l2_async_notifier_asd_valid(struct v4l2_async_notifier *notifier,
> > > +					 struct v4l2_async_subdev *asd,
> > > +					 unsigned int this_index)
> > >   {
> > >   	struct device *dev =
> > >   		notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
> > > +
> > > +	if (!asd)
> > > +		return -EINVAL;
> > > +
> > > +	switch (asd->match_type) {
> > > +	case V4L2_ASYNC_MATCH_CUSTOM:
> > > +	case V4L2_ASYNC_MATCH_DEVNAME:
> > > +	case V4L2_ASYNC_MATCH_I2C:
> > > +	case V4L2_ASYNC_MATCH_FWNODE:
> > > +		if (v4l2_async_notifier_has_async_subdev(notifier, asd,
> > > +							 this_index))
> > > +			return -EEXIST;
> > > +		break;
> > > +	default:
> > > +		dev_err(dev, "Invalid match type %u on %p\n",
> > > +			asd->match_type, asd);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void __v4l2_async_notifier_init(struct v4l2_async_notifier *notifier)
> > > +{
> > > +	lockdep_assert_held(&list_lock);
> > > +
> > > +	INIT_LIST_HEAD(&notifier->asd_list);
> > > +	INIT_LIST_HEAD(&notifier->waiting);
> > > +	INIT_LIST_HEAD(&notifier->done);
> > > +	notifier->lists_initialized = true;
> > > +}
> > > +
> > > +static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> > > +{
> > >   	struct v4l2_async_subdev *asd;
> > >   	int ret;
> > >   	int i;
> > > @@ -397,34 +443,40 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> > >   	if (notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> > >   		return -EINVAL;
> > > -	INIT_LIST_HEAD(&notifier->waiting);
> > > -	INIT_LIST_HEAD(&notifier->done);
> > > -
> > >   	mutex_lock(&list_lock);
> > > -	for (i = 0; i < notifier->num_subdevs; i++) {
> > > -		asd = notifier->subdevs[i];
> > > +	if (!notifier->lists_initialized)
> > > +		__v4l2_async_notifier_init(notifier);
> > > -		switch (asd->match_type) {
> > > -		case V4L2_ASYNC_MATCH_CUSTOM:
> > > -		case V4L2_ASYNC_MATCH_DEVNAME:
> > > -		case V4L2_ASYNC_MATCH_I2C:
> > > -		case V4L2_ASYNC_MATCH_FWNODE:
> > > -			if (v4l2_async_notifier_has_async_subdev(
> > > -				    notifier, asd, i)) {
> > > -				dev_err(dev,
> > > -					"asd has already been registered or in notifier's subdev list\n");
> > > -				ret = -EEXIST;
> > > -				goto err_unlock;
> > > -			}
> > > -			break;
> > > -		default:
> > > -			dev_err(dev, "Invalid match type %u on %p\n",
> > > -				asd->match_type, asd);
> > > +	if (!list_empty(&notifier->asd_list)) {
> > > +		/*
> > > +		 * Caller must have either used v4l2_async_notifier_add_subdev
> > > +		 * to add asd's to notifier->asd_list, or provided the
> > > +		 * notifier->subdevs array, but not both.
> > > +		 */
> > > +		if (WARN_ON(notifier->subdevs)) {
> > >   			ret = -EINVAL;
> > >   			goto err_unlock;
> > >   		}
> > > -		list_add_tail(&asd->list, &notifier->waiting);
> > > +
> > > +		i = 0;
> > > +		list_for_each_entry(asd, &notifier->asd_list, asd_list) {
> > > +			ret = v4l2_async_notifier_asd_valid(notifier, asd, i++);
> > > +			if (ret)
> > > +				goto err_unlock;
> > > +
> > > +			list_add_tail(&asd->list, &notifier->waiting);
> > > +		}
> > > +	} else if (notifier->subdevs) {
> > > +		for (i = 0; i < notifier->num_subdevs; i++) {
> > > +			asd = notifier->subdevs[i];
> > > +
> > > +			ret = v4l2_async_notifier_asd_valid(notifier, asd, i);
> > > +			if (ret)
> > > +				goto err_unlock;
> > > +
> > > +			list_add_tail(&asd->list, &notifier->waiting);
> > > +		}
> > >   	}
> > >   	ret = v4l2_async_notifier_try_all_subdevs(notifier);
> > > @@ -514,36 +566,102 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> > >   }
> > >   EXPORT_SYMBOL(v4l2_async_notifier_unregister);
> > > -void v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
> > > +static void __v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
> > >   {
> > > +	struct v4l2_async_subdev *asd, *tmp;
> > >   	unsigned int i;
> > > -	if (!notifier || !notifier->max_subdevs)
> > > +	if (!notifier)
> > >   		return;
> > > -	for (i = 0; i < notifier->num_subdevs; i++) {
> > > -		struct v4l2_async_subdev *asd = notifier->subdevs[i];
> > > +	if (notifier->subdevs) {
> > > +		if (!notifier->max_subdevs)
> > > +			return;
> > > -		switch (asd->match_type) {
> > > -		case V4L2_ASYNC_MATCH_FWNODE:
> > > -			fwnode_handle_put(asd->match.fwnode);
> > > -			break;
> > > -		default:
> > > -			WARN_ON_ONCE(true);
> > > -			break;
> > > +		for (i = 0; i < notifier->num_subdevs; i++) {
> > > +			asd = notifier->subdevs[i];
> > > +
> > > +			switch (asd->match_type) {
> > > +			case V4L2_ASYNC_MATCH_FWNODE:
> > > +				fwnode_handle_put(asd->match.fwnode);
> > > +				break;
> > > +			default:
> > > +				break;
> > > +			}
> > > +
> > > +			kfree(asd);
> > >   		}
> > > -		kfree(asd);
> > > +		notifier->max_subdevs = 0;
> > > +		kvfree(notifier->subdevs);
> > > +		notifier->subdevs = NULL;
> > > +	} else if (notifier->lists_initialized) {
> > > +		list_for_each_entry_safe(asd, tmp,
> > > +					 &notifier->asd_list, asd_list) {
> > > +			switch (asd->match_type) {
> > > +			case V4L2_ASYNC_MATCH_FWNODE:
> > > +				fwnode_handle_put(asd->match.fwnode);
> > > +				break;
> > > +			default:
> > > +				break;
> > > +			}
> > > +
> > > +			list_del(&asd->asd_list);
> > > +			kfree(asd);
> > > +		}
> > >   	}
> > > -	notifier->max_subdevs = 0;
> > >   	notifier->num_subdevs = 0;
> > > +}
> > > -	kvfree(notifier->subdevs);
> > > -	notifier->subdevs = NULL;
> > > +void v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
> > > +{
> > > +	mutex_lock(&list_lock);
> > > +
> > > +	__v4l2_async_notifier_cleanup(notifier);
> > > +
> > > +	mutex_unlock(&list_lock);
> > >   }
> > >   EXPORT_SYMBOL_GPL(v4l2_async_notifier_cleanup);
> > > +int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
> > > +				   struct v4l2_async_subdev *asd)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	mutex_lock(&list_lock);
> > > +
> > > +	if (notifier->num_subdevs >= V4L2_MAX_SUBDEVS) {
> > > +		ret = -EINVAL;
> > > +		goto unlock;
> > > +	}
> > > +
> > > +	if (!notifier->lists_initialized)
> > > +		__v4l2_async_notifier_init(notifier);
> > > +
> > > +	/*
> > > +	 * If caller uses this function, it cannot also allocate and
> > > +	 * place asd's in the notifier->subdevs array.
> > > +	 */
> > > +	if (WARN_ON(notifier->subdevs)) {
> > > +		ret = -EINVAL;
> > > +		goto unlock;
> > > +	}
> > > +
> > > +	ret = v4l2_async_notifier_asd_valid(notifier, asd,
> > > +					    notifier->num_subdevs);
> > > +	if (ret)
> > > +		goto unlock;
> > > +
> > > +	list_add_tail(&asd->asd_list, &notifier->asd_list);
> > > +	notifier->num_subdevs++;
> > > +
> > > +unlock:
> > > +	mutex_unlock(&list_lock);
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(v4l2_async_notifier_add_subdev);
> > > +
> > >   int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> > >   {
> > >   	struct v4l2_async_notifier *subdev_notifier;
> > > @@ -617,7 +735,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
> > >   	mutex_lock(&list_lock);
> > >   	__v4l2_async_notifier_unregister(sd->subdev_notifier);
> > > -	v4l2_async_notifier_cleanup(sd->subdev_notifier);
> > > +	__v4l2_async_notifier_cleanup(sd->subdev_notifier);
> > >   	kfree(sd->subdev_notifier);
> > >   	sd->subdev_notifier = NULL;
> > > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > > index 1592d32..fa05905 100644
> > > --- a/include/media/v4l2-async.h
> > > +++ b/include/media/v4l2-async.h
> > > @@ -73,6 +73,8 @@ enum v4l2_async_match_type {
> > >    * @match.custom.priv:
> > >    *		Driver-specific private struct with match parameters
> > >    *		to be used if %V4L2_ASYNC_MATCH_CUSTOM.
> > > + * @asd_list:	used to add struct v4l2_async_subdev objects to the
> > > + *		master notifier->asd_list
> > >    * @list:	used to link struct v4l2_async_subdev objects, waiting to be
> > >    *		probed, to a notifier->waiting list
> > >    *
> > > @@ -98,6 +100,7 @@ struct v4l2_async_subdev {
> > >   	/* v4l2-async core private: not to be used by drivers */
> > >   	struct list_head list;
> > > +	struct list_head asd_list;
> > >   };
> > >   /**
> > > @@ -127,9 +130,11 @@ struct v4l2_async_notifier_operations {
> > >    * @v4l2_dev:	v4l2_device of the root notifier, NULL otherwise
> > >    * @sd:		sub-device that registered the notifier, NULL otherwise
> > >    * @parent:	parent notifier
> > > + * @asd_list:	master list of struct v4l2_async_subdev, replaces @subdevs
> > >    * @waiting:	list of struct v4l2_async_subdev, waiting for their drivers
> > >    * @done:	list of struct v4l2_subdev, already probed
> > >    * @list:	member in a global list of notifiers
> > > + * @lists_initialized: list_head's have been initialized
> > >    */
> > >   struct v4l2_async_notifier {
> > >   	const struct v4l2_async_notifier_operations *ops;
> > > @@ -139,12 +144,29 @@ struct v4l2_async_notifier {
> > >   	struct v4l2_device *v4l2_dev;
> > >   	struct v4l2_subdev *sd;
> > >   	struct v4l2_async_notifier *parent;
> > > +	struct list_head asd_list;
> > >   	struct list_head waiting;
> > >   	struct list_head done;
> > >   	struct list_head list;
> > > +	bool lists_initialized;
> > >   };
> > >   /**
> > > + * v4l2_async_notifier_add_subdev - Add an async subdev to the
> > > + *				notifier's master asd_list.
> > > + *
> > > + * @notifier: pointer to &struct v4l2_async_notifier
> > > + * @asd: pointer to &struct v4l2_async_subdev
> > > + *
> > > + * This can be used before registering a notifier to add an
> > > + * asd to the notifiers master asd_list. If the caller uses
> > > + * this method to compose an asd list, it must never allocate
> > > + * or place asd's in the @subdevs array.
> > > + */
> > > +int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
> > > +				   struct v4l2_async_subdev *asd);
> > > +
> > > +/**
> > >    * v4l2_async_notifier_register - registers a subdevice asynchronous notifier
> > >    *
> > >    * @v4l2_dev: pointer to &struct v4l2_device
> > > -- 
> > > 2.7.4
> > > 
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev

[Steve Longerbeam]

On 05/08/2018 03:12 AM, Sakari Ailus wrote:
> On Fri, Apr 20, 2018 at 10:12:33AM -0700, Steve Longerbeam wrote:
>> Hi Sakari,
>>
>>
>> On 04/20/2018 05:24 AM, Sakari Ailus wrote:
>>> Hi Steve,
>>>
>>> Thanks for the patchset.
>>>
>>> On Tue, Mar 20, 2018 at 05:37:19PM -0700, Steve Longerbeam wrote:
>>>> v4l2_async_notifier_add_subdev() adds an asd to the notifier. It checks
>>>> that the asd's match_type is valid and that no other equivalent asd's
>>>> have already been added to this notifier's asd list, or to other
>>>> registered notifier's waiting or done lists, and increments num_subdevs.
>>>>
>>>> v4l2_async_notifier_add_subdev() does not make use of the notifier subdevs
>>>> array, otherwise it would have to re-allocate the array every time the
>>>> function was called. In place of the subdevs array, the function adds
>>>> the asd to a new master asd_list. The function will return error with a
>>>> WARN() if it is ever called with the subdevs array allocated.
>>>>
>>>> In v4l2_async_notifier_has_async_subdev(), __v4l2_async_notifier_register(),
>>>> and v4l2_async_notifier_cleanup(), alternatively operate on the subdevs
>>>> array or a non-empty notifier->asd_list.
>>> I do agree with the approach, but this patch leaves the remaining users of
>>> the subdevs array in the notifier intact. Could you rework them to use the
>>> v4l2_async_notifier_add_subdev() as well?
>>>
>>> There seem to be just a few of them --- exynos4-is and rcar_drif.
>> I count more than a few :)
>>
>> % cd drivers/media && grep -l -r --include "*.[ch]"
>> 'notifier[\.\-]>*subdevs[   ]*='
>> v4l2-core/v4l2-async.c
>> platform/pxa_camera.c
>> platform/ti-vpe/cal.c
>> platform/exynos4-is/media-dev.c
>> platform/qcom/camss-8x16/camss.c
>> platform/soc_camera/soc_camera.c
>> platform/atmel/atmel-isi.c
>> platform/atmel/atmel-isc.c
>> platform/stm32/stm32-dcmi.c
>> platform/davinci/vpif_capture.c
>> platform/davinci/vpif_display.c
>> platform/renesas-ceu.c
>> platform/am437x/am437x-vpfe.c
>> platform/xilinx/xilinx-vipp.c
>> platform/rcar_drif.c
>>
>>
>> So not including v4l2-core, the list is:
>>
>> pxa_camera
>> ti-vpe
>> exynos4-is
>> qcom
>> soc_camera
>> atmel
>> stm32
>> davinci
>> renesas-ceu
>> am437x
>> xilinx
>> rcar_drif
>>
>>
>> Given such a large list of the users of the notifier->subdevs array,
>> I think this should be done is two steps: submit this patchset first,
>> that keeps the backward compatibility, and then a subsequent patchset
>> that converts all drivers to use v4l2_async_notifier_add_subdev(), at
>> which point the subdevs array can be removed from struct
>> v4l2_async_notifier.
>>
>> Or, do you still think this should be done all at once? I could add a
>> large patch to this patchset that does the conversion and removes
>> the subdevs array.
> Sorry for the delay. I grepped for this, too, but I guess I ended up using
> an expression that missed most of the users. :-(
>
> I think it'd be very good to perform that conversion --- the code in the
> framework would be quite a bit cleaner and easier to maintain whereas the
> per-driver conversions seem pretty simple, such as this on in
> drivers/media/platform/atmel/atmel-isi.c :
>
> 	/* Register the subdevices notifier. */
> 	subdevs = devm_kzalloc(isi->dev, sizeof(*subdevs), GFP_KERNEL);
> 	if (!subdevs) {
> 	        of_node_put(isi->entity.node);
> 		return -ENOMEM;
> 	}
>
> 	subdevs[0] = &isi->entity.asd;
>
> 	isi->notifier.subdevs = subdevs;
> 	isi->notifier.num_subdevs = 1;
> 	isi->notifier.ops = &isi_graph_notify_ops;


Yes, the conversions are fairly straightforward. I've completed that work,
but it was a very manual conversion, every platform is different and needed
careful study.

Although I was careful about getting the error-out paths correct, there 
could
be mistakes there, which would result in memory leaks. And obviously I can't
re-test all these platforms. So this patch is very high-risk. More eyes are
needed on it, ideally the maintainer(s) of each affected platform.

I just submitted v4 of this series, but did not include this large un-tested
patch in v4 for those reasons.

Instead, this patch, and follow-up patches that strips support for subdevs
array altogether from v4l2-async.c, and updates rst docs, are available 
at my
media-tree mirror on github:

git@github.com:slongerbeam/mediatree.git

in the branch 'remove-subdevs-array'. The branch is based off this series
(branch 'imx-subdev-notifiers.6').

Steve

Re: [PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev

[Sakari Ailus]

On Wed, May 09, 2018 at 04:06:32PM -0700, Steve Longerbeam wrote:
> 
> 
> On 05/08/2018 03:12 AM, Sakari Ailus wrote:
> > On Fri, Apr 20, 2018 at 10:12:33AM -0700, Steve Longerbeam wrote:
> > > Hi Sakari,
> > > 
> > > 
> > > On 04/20/2018 05:24 AM, Sakari Ailus wrote:
> > > > Hi Steve,
> > > > 
> > > > Thanks for the patchset.
> > > > 
> > > > On Tue, Mar 20, 2018 at 05:37:19PM -0700, Steve Longerbeam wrote:
> > > > > v4l2_async_notifier_add_subdev() adds an asd to the notifier. It checks
> > > > > that the asd's match_type is valid and that no other equivalent asd's
> > > > > have already been added to this notifier's asd list, or to other
> > > > > registered notifier's waiting or done lists, and increments num_subdevs.
> > > > > 
> > > > > v4l2_async_notifier_add_subdev() does not make use of the notifier subdevs
> > > > > array, otherwise it would have to re-allocate the array every time the
> > > > > function was called. In place of the subdevs array, the function adds
> > > > > the asd to a new master asd_list. The function will return error with a
> > > > > WARN() if it is ever called with the subdevs array allocated.
> > > > > 
> > > > > In v4l2_async_notifier_has_async_subdev(), __v4l2_async_notifier_register(),
> > > > > and v4l2_async_notifier_cleanup(), alternatively operate on the subdevs
> > > > > array or a non-empty notifier->asd_list.
> > > > I do agree with the approach, but this patch leaves the remaining users of
> > > > the subdevs array in the notifier intact. Could you rework them to use the
> > > > v4l2_async_notifier_add_subdev() as well?
> > > > 
> > > > There seem to be just a few of them --- exynos4-is and rcar_drif.
> > > I count more than a few :)
> > > 
> > > % cd drivers/media && grep -l -r --include "*.[ch]"
> > > 'notifier[\.\-]>*subdevs[   ]*='
> > > v4l2-core/v4l2-async.c
> > > platform/pxa_camera.c
> > > platform/ti-vpe/cal.c
> > > platform/exynos4-is/media-dev.c
> > > platform/qcom/camss-8x16/camss.c
> > > platform/soc_camera/soc_camera.c
> > > platform/atmel/atmel-isi.c
> > > platform/atmel/atmel-isc.c
> > > platform/stm32/stm32-dcmi.c
> > > platform/davinci/vpif_capture.c
> > > platform/davinci/vpif_display.c
> > > platform/renesas-ceu.c
> > > platform/am437x/am437x-vpfe.c
> > > platform/xilinx/xilinx-vipp.c
> > > platform/rcar_drif.c
> > > 
> > > 
> > > So not including v4l2-core, the list is:
> > > 
> > > pxa_camera
> > > ti-vpe
> > > exynos4-is
> > > qcom
> > > soc_camera
> > > atmel
> > > stm32
> > > davinci
> > > renesas-ceu
> > > am437x
> > > xilinx
> > > rcar_drif
> > > 
> > > 
> > > Given such a large list of the users of the notifier->subdevs array,
> > > I think this should be done is two steps: submit this patchset first,
> > > that keeps the backward compatibility, and then a subsequent patchset
> > > that converts all drivers to use v4l2_async_notifier_add_subdev(), at
> > > which point the subdevs array can be removed from struct
> > > v4l2_async_notifier.
> > > 
> > > Or, do you still think this should be done all at once? I could add a
> > > large patch to this patchset that does the conversion and removes
> > > the subdevs array.
> > Sorry for the delay. I grepped for this, too, but I guess I ended up using
> > an expression that missed most of the users. :-(
> > 
> > I think it'd be very good to perform that conversion --- the code in the
> > framework would be quite a bit cleaner and easier to maintain whereas the
> > per-driver conversions seem pretty simple, such as this on in
> > drivers/media/platform/atmel/atmel-isi.c :
> > 
> > 	/* Register the subdevices notifier. */
> > 	subdevs = devm_kzalloc(isi->dev, sizeof(*subdevs), GFP_KERNEL);
> > 	if (!subdevs) {
> > 	        of_node_put(isi->entity.node);
> > 		return -ENOMEM;
> > 	}
> > 
> > 	subdevs[0] = &isi->entity.asd;
> > 
> > 	isi->notifier.subdevs = subdevs;
> > 	isi->notifier.num_subdevs = 1;
> > 	isi->notifier.ops = &isi_graph_notify_ops;
> 
> 
> Yes, the conversions are fairly straightforward. I've completed that work,
> but it was a very manual conversion, every platform is different and needed
> careful study.
> 
> Although I was careful about getting the error-out paths correct, there
> could
> be mistakes there, which would result in memory leaks. And obviously I can't
> re-test all these platforms. So this patch is very high-risk. More eyes are
> needed on it, ideally the maintainer(s) of each affected platform.
> 
> I just submitted v4 of this series, but did not include this large un-tested
> patch in v4 for those reasons.
> 
> Instead, this patch, and follow-up patches that strips support for subdevs
> array altogether from v4l2-async.c, and updates rst docs, are available at
> my
> media-tree mirror on github:
> 
> git@github.com:slongerbeam/mediatree.git
> 
> in the branch 'remove-subdevs-array'. The branch is based off this series
> (branch 'imx-subdev-notifiers.6').

Would you be able to post these to the list? I'd really like this being
done as part of the related patchset, rather than leaving the mess in the
framework.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev

[Steve Longerbeam]

On 06/26/2018 12:12 AM, Sakari Ailus wrote:
> On Wed, May 09, 2018 at 04:06:32PM -0700, Steve Longerbeam wrote:
>>
>> On 05/08/2018 03:12 AM, Sakari Ailus wrote:
>>> On Fri, Apr 20, 2018 at 10:12:33AM -0700, Steve Longerbeam wrote:
>>>> Hi Sakari,
>>>>
>>>>
>>>> On 04/20/2018 05:24 AM, Sakari Ailus wrote:
>>>>> Hi Steve,
>>>>>
>>>>> Thanks for the patchset.
>>>>>
>>>>> On Tue, Mar 20, 2018 at 05:37:19PM -0700, Steve Longerbeam wrote:
>>>>>> v4l2_async_notifier_add_subdev() adds an asd to the notifier. It checks
>>>>>> that the asd's match_type is valid and that no other equivalent asd's
>>>>>> have already been added to this notifier's asd list, or to other
>>>>>> registered notifier's waiting or done lists, and increments num_subdevs.
>>>>>>
>>>>>> v4l2_async_notifier_add_subdev() does not make use of the notifier subdevs
>>>>>> array, otherwise it would have to re-allocate the array every time the
>>>>>> function was called. In place of the subdevs array, the function adds
>>>>>> the asd to a new master asd_list. The function will return error with a
>>>>>> WARN() if it is ever called with the subdevs array allocated.
>>>>>>
>>>>>> In v4l2_async_notifier_has_async_subdev(), __v4l2_async_notifier_register(),
>>>>>> and v4l2_async_notifier_cleanup(), alternatively operate on the subdevs
>>>>>> array or a non-empty notifier->asd_list.
>>>>> I do agree with the approach, but this patch leaves the remaining users of
>>>>> the subdevs array in the notifier intact. Could you rework them to use the
>>>>> v4l2_async_notifier_add_subdev() as well?
>>>>>
>>>>> There seem to be just a few of them --- exynos4-is and rcar_drif.
>>>> I count more than a few :)
>>>>
>>>> % cd drivers/media && grep -l -r --include "*.[ch]"
>>>> 'notifier[\.\-]>*subdevs[   ]*='
>>>> v4l2-core/v4l2-async.c
>>>> platform/pxa_camera.c
>>>> platform/ti-vpe/cal.c
>>>> platform/exynos4-is/media-dev.c
>>>> platform/qcom/camss-8x16/camss.c
>>>> platform/soc_camera/soc_camera.c
>>>> platform/atmel/atmel-isi.c
>>>> platform/atmel/atmel-isc.c
>>>> platform/stm32/stm32-dcmi.c
>>>> platform/davinci/vpif_capture.c
>>>> platform/davinci/vpif_display.c
>>>> platform/renesas-ceu.c
>>>> platform/am437x/am437x-vpfe.c
>>>> platform/xilinx/xilinx-vipp.c
>>>> platform/rcar_drif.c
>>>>
>>>>
>>>> So not including v4l2-core, the list is:
>>>>
>>>> pxa_camera
>>>> ti-vpe
>>>> exynos4-is
>>>> qcom
>>>> soc_camera
>>>> atmel
>>>> stm32
>>>> davinci
>>>> renesas-ceu
>>>> am437x
>>>> xilinx
>>>> rcar_drif
>>>>
>>>>
>>>> Given such a large list of the users of the notifier->subdevs array,
>>>> I think this should be done is two steps: submit this patchset first,
>>>> that keeps the backward compatibility, and then a subsequent patchset
>>>> that converts all drivers to use v4l2_async_notifier_add_subdev(), at
>>>> which point the subdevs array can be removed from struct
>>>> v4l2_async_notifier.
>>>>
>>>> Or, do you still think this should be done all at once? I could add a
>>>> large patch to this patchset that does the conversion and removes
>>>> the subdevs array.
>>> Sorry for the delay. I grepped for this, too, but I guess I ended up using
>>> an expression that missed most of the users. :-(
>>>
>>> I think it'd be very good to perform that conversion --- the code in the
>>> framework would be quite a bit cleaner and easier to maintain whereas the
>>> per-driver conversions seem pretty simple, such as this on in
>>> drivers/media/platform/atmel/atmel-isi.c :
>>>
>>> 	/* Register the subdevices notifier. */
>>> 	subdevs = devm_kzalloc(isi->dev, sizeof(*subdevs), GFP_KERNEL);
>>> 	if (!subdevs) {
>>> 	        of_node_put(isi->entity.node);
>>> 		return -ENOMEM;
>>> 	}
>>>
>>> 	subdevs[0] = &isi->entity.asd;
>>>
>>> 	isi->notifier.subdevs = subdevs;
>>> 	isi->notifier.num_subdevs = 1;
>>> 	isi->notifier.ops = &isi_graph_notify_ops;
>>
>> Yes, the conversions are fairly straightforward. I've completed that work,
>> but it was a very manual conversion, every platform is different and needed
>> careful study.
>>
>> Although I was careful about getting the error-out paths correct, there
>> could
>> be mistakes there, which would result in memory leaks. And obviously I can't
>> re-test all these platforms. So this patch is very high-risk. More eyes are
>> needed on it, ideally the maintainer(s) of each affected platform.
>>
>> I just submitted v4 of this series, but did not include this large un-tested
>> patch in v4 for those reasons.
>>
>> Instead, this patch, and follow-up patches that strips support for subdevs
>> array altogether from v4l2-async.c, and updates rst docs, are available at
>> my
>> media-tree mirror on github:
>>
>> git@github.com:slongerbeam/mediatree.git
>>
>> in the branch 'remove-subdevs-array'. The branch is based off this series
>> (branch 'imx-subdev-notifiers.6').
> Would you be able to post these to the list? I'd really like this being
> done as part of the related patchset, rather than leaving the mess in the
> framework.

Backward compatibility can look messy.

I can include the patch that converts all platforms at once. But as I
said it is completely untested.

So I'm curious, what is the policy in V4L2 community regarding
merging untested patches? Do we go ahead and merge and then
fixup errors as they are discovered, or should the patch get basic
validation by everyone who has access to the affected hardware
first?

Steve

Re: [PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev

[Sakari Ailus]

On Tue, Jun 26, 2018 at 01:47:45PM -0700, Steve Longerbeam wrote:
> 
> 
> On 06/26/2018 12:12 AM, Sakari Ailus wrote:
> > On Wed, May 09, 2018 at 04:06:32PM -0700, Steve Longerbeam wrote:
> > > 
> > > On 05/08/2018 03:12 AM, Sakari Ailus wrote:
> > > > On Fri, Apr 20, 2018 at 10:12:33AM -0700, Steve Longerbeam wrote:
> > > > > Hi Sakari,
> > > > > 
> > > > > 
> > > > > On 04/20/2018 05:24 AM, Sakari Ailus wrote:
> > > > > > Hi Steve,
> > > > > > 
> > > > > > Thanks for the patchset.
> > > > > > 
> > > > > > On Tue, Mar 20, 2018 at 05:37:19PM -0700, Steve Longerbeam wrote:
> > > > > > > v4l2_async_notifier_add_subdev() adds an asd to the notifier. It checks
> > > > > > > that the asd's match_type is valid and that no other equivalent asd's
> > > > > > > have already been added to this notifier's asd list, or to other
> > > > > > > registered notifier's waiting or done lists, and increments num_subdevs.
> > > > > > > 
> > > > > > > v4l2_async_notifier_add_subdev() does not make use of the notifier subdevs
> > > > > > > array, otherwise it would have to re-allocate the array every time the
> > > > > > > function was called. In place of the subdevs array, the function adds
> > > > > > > the asd to a new master asd_list. The function will return error with a
> > > > > > > WARN() if it is ever called with the subdevs array allocated.
> > > > > > > 
> > > > > > > In v4l2_async_notifier_has_async_subdev(), __v4l2_async_notifier_register(),
> > > > > > > and v4l2_async_notifier_cleanup(), alternatively operate on the subdevs
> > > > > > > array or a non-empty notifier->asd_list.
> > > > > > I do agree with the approach, but this patch leaves the remaining users of
> > > > > > the subdevs array in the notifier intact. Could you rework them to use the
> > > > > > v4l2_async_notifier_add_subdev() as well?
> > > > > > 
> > > > > > There seem to be just a few of them --- exynos4-is and rcar_drif.
> > > > > I count more than a few :)
> > > > > 
> > > > > % cd drivers/media && grep -l -r --include "*.[ch]"
> > > > > 'notifier[\.\-]>*subdevs[   ]*='
> > > > > v4l2-core/v4l2-async.c
> > > > > platform/pxa_camera.c
> > > > > platform/ti-vpe/cal.c
> > > > > platform/exynos4-is/media-dev.c
> > > > > platform/qcom/camss-8x16/camss.c
> > > > > platform/soc_camera/soc_camera.c
> > > > > platform/atmel/atmel-isi.c
> > > > > platform/atmel/atmel-isc.c
> > > > > platform/stm32/stm32-dcmi.c
> > > > > platform/davinci/vpif_capture.c
> > > > > platform/davinci/vpif_display.c
> > > > > platform/renesas-ceu.c
> > > > > platform/am437x/am437x-vpfe.c
> > > > > platform/xilinx/xilinx-vipp.c
> > > > > platform/rcar_drif.c
> > > > > 
> > > > > 
> > > > > So not including v4l2-core, the list is:
> > > > > 
> > > > > pxa_camera
> > > > > ti-vpe
> > > > > exynos4-is
> > > > > qcom
> > > > > soc_camera
> > > > > atmel
> > > > > stm32
> > > > > davinci
> > > > > renesas-ceu
> > > > > am437x
> > > > > xilinx
> > > > > rcar_drif
> > > > > 
> > > > > 
> > > > > Given such a large list of the users of the notifier->subdevs array,
> > > > > I think this should be done is two steps: submit this patchset first,
> > > > > that keeps the backward compatibility, and then a subsequent patchset
> > > > > that converts all drivers to use v4l2_async_notifier_add_subdev(), at
> > > > > which point the subdevs array can be removed from struct
> > > > > v4l2_async_notifier.
> > > > > 
> > > > > Or, do you still think this should be done all at once? I could add a
> > > > > large patch to this patchset that does the conversion and removes
> > > > > the subdevs array.
> > > > Sorry for the delay. I grepped for this, too, but I guess I ended up using
> > > > an expression that missed most of the users. :-(
> > > > 
> > > > I think it'd be very good to perform that conversion --- the code in the
> > > > framework would be quite a bit cleaner and easier to maintain whereas the
> > > > per-driver conversions seem pretty simple, such as this on in
> > > > drivers/media/platform/atmel/atmel-isi.c :
> > > > 
> > > > 	/* Register the subdevices notifier. */
> > > > 	subdevs = devm_kzalloc(isi->dev, sizeof(*subdevs), GFP_KERNEL);
> > > > 	if (!subdevs) {
> > > > 	        of_node_put(isi->entity.node);
> > > > 		return -ENOMEM;
> > > > 	}
> > > > 
> > > > 	subdevs[0] = &isi->entity.asd;
> > > > 
> > > > 	isi->notifier.subdevs = subdevs;
> > > > 	isi->notifier.num_subdevs = 1;
> > > > 	isi->notifier.ops = &isi_graph_notify_ops;
> > > 
> > > Yes, the conversions are fairly straightforward. I've completed that work,
> > > but it was a very manual conversion, every platform is different and needed
> > > careful study.
> > > 
> > > Although I was careful about getting the error-out paths correct, there
> > > could
> > > be mistakes there, which would result in memory leaks. And obviously I can't
> > > re-test all these platforms. So this patch is very high-risk. More eyes are
> > > needed on it, ideally the maintainer(s) of each affected platform.
> > > 
> > > I just submitted v4 of this series, but did not include this large un-tested
> > > patch in v4 for those reasons.
> > > 
> > > Instead, this patch, and follow-up patches that strips support for subdevs
> > > array altogether from v4l2-async.c, and updates rst docs, are available at
> > > my
> > > media-tree mirror on github:
> > > 
> > > git@github.com:slongerbeam/mediatree.git
> > > 
> > > in the branch 'remove-subdevs-array'. The branch is based off this series
> > > (branch 'imx-subdev-notifiers.6').
> > Would you be able to post these to the list? I'd really like this being
> > done as part of the related patchset, rather than leaving the mess in the
> > framework.
> 
> Backward compatibility can look messy.
> 
> I can include the patch that converts all platforms at once. But as I
> said it is completely untested.
> 
> So I'm curious, what is the policy in V4L2 community regarding
> merging untested patches? Do we go ahead and merge and then
> fixup errors as they are discovered, or should the patch get basic
> validation by everyone who has access to the affected hardware
> first?

Good question. You can't always have all the hardware of the drivers you
end up having to change. We've had quite a few such changes to the
frameworks; the patches are still reviewed and hopefully the maintainer of
the relevant driver reviews and tests the patches. Not everything has been
always tested though. Still, I don't remember any of such (mostly trivial)
framework-wide changes having been a noteworty source of bugs.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

[PATCH v3 04/13] media: v4l2-fwnode: Switch to v4l2_async_notifier_add_subdev

[Steve Longerbeam]

The fwnode endpoint and reference parsing functions in v4l2-fwnode.c
are modified to make use of v4l2_async_notifier_add_subdev(). As a
result the notifier->subdevs array is no longer allocated or
re-allocated, and by extension the max_subdevs value is also no
longer needed.

Since the notifier->subdevs array is no longer allocated in the
fwnode endpoint and reference parsing functions, the callers of
those functions must never reference that array, since it is now
NULL. Of the drivers that make use of the fwnode/ref parsing,
only the intel-ipu3 driver references the ->subdevs[] array,
(in the notifier completion callback), so that driver has been
modified to iterate through the notifier->asd_list instead.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c |  10 +--
 drivers/media/v4l2-core/v4l2-async.c     |   4 -
 drivers/media/v4l2-core/v4l2-fwnode.c    | 128 ++++++++-----------------------
 include/media/v4l2-async.h               |   2 -
 include/media/v4l2-fwnode.h              |  22 +++---
 5 files changed, 51 insertions(+), 115 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 7d768ec..b0cadc8 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -1423,13 +1423,13 @@ static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
 	struct cio2_device *cio2 = container_of(notifier, struct cio2_device,
 						notifier);
 	struct sensor_async_subdev *s_asd;
+	struct v4l2_async_subdev *asd;
 	struct cio2_queue *q;
-	unsigned int i, pad;
+	unsigned int pad;
 	int ret;
 
-	for (i = 0; i < notifier->num_subdevs; i++) {
-		s_asd = container_of(cio2->notifier.subdevs[i],
-				     struct sensor_async_subdev, asd);
+	list_for_each_entry(asd, &cio2->notifier.asd_list, asd_list) {
+		s_asd = container_of(asd, struct sensor_async_subdev, asd);
 		q = &cio2->queue[s_asd->csi2.port];
 
 		for (pad = 0; pad < q->sensor->entity.num_pads; pad++)
@@ -1451,7 +1451,7 @@ static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
 		if (ret) {
 			dev_err(&cio2->pci_dev->dev,
 				"failed to create link for %s\n",
-				cio2->queue[i].sensor->name);
+				q->sensor->name);
 			return ret;
 		}
 	}
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 7b7f7e2..7fa5de0 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -575,9 +575,6 @@ static void __v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
 		return;
 
 	if (notifier->subdevs) {
-		if (!notifier->max_subdevs)
-			return;
-
 		for (i = 0; i < notifier->num_subdevs; i++) {
 			asd = notifier->subdevs[i];
 
@@ -592,7 +589,6 @@ static void __v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
 			kfree(asd);
 		}
 
-		notifier->max_subdevs = 0;
 		kvfree(notifier->subdevs);
 		notifier->subdevs = NULL;
 	} else if (notifier->lists_initialized) {
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index b8afbac..99198b9 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -316,33 +316,6 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
 }
 EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
 
-static int v4l2_async_notifier_realloc(struct v4l2_async_notifier *notifier,
-				       unsigned int max_subdevs)
-{
-	struct v4l2_async_subdev **subdevs;
-
-	if (max_subdevs <= notifier->max_subdevs)
-		return 0;
-
-	subdevs = kvmalloc_array(
-		max_subdevs, sizeof(*notifier->subdevs),
-		GFP_KERNEL | __GFP_ZERO);
-	if (!subdevs)
-		return -ENOMEM;
-
-	if (notifier->subdevs) {
-		memcpy(subdevs, notifier->subdevs,
-		       sizeof(*subdevs) * notifier->num_subdevs);
-
-		kvfree(notifier->subdevs);
-	}
-
-	notifier->subdevs = subdevs;
-	notifier->max_subdevs = max_subdevs;
-
-	return 0;
-}
-
 static int v4l2_async_notifier_fwnode_parse_endpoint(
 	struct device *dev, struct v4l2_async_notifier *notifier,
 	struct fwnode_handle *endpoint, unsigned int asd_struct_size,
@@ -387,8 +360,13 @@ static int v4l2_async_notifier_fwnode_parse_endpoint(
 	if (ret < 0)
 		goto out_err;
 
-	notifier->subdevs[notifier->num_subdevs] = asd;
-	notifier->num_subdevs++;
+	ret = v4l2_async_notifier_add_subdev(notifier, asd);
+	if (ret < 0) {
+		/* not an error if asd already exists */
+		if (ret == -EEXIST)
+			ret = 0;
+		goto out_err;
+	}
 
 	return 0;
 
@@ -407,8 +385,7 @@ static int __v4l2_async_notifier_parse_fwnode_endpoints(
 			    struct v4l2_async_subdev *asd))
 {
 	struct fwnode_handle *fwnode;
-	unsigned int max_subdevs = notifier->max_subdevs;
-	int ret;
+	int ret = 0;
 
 	if (WARN_ON(asd_struct_size < sizeof(struct v4l2_async_subdev)))
 		return -EINVAL;
@@ -428,40 +405,6 @@ static int __v4l2_async_notifier_parse_fwnode_endpoints(
 			struct fwnode_endpoint ep;
 
 			ret = fwnode_graph_parse_endpoint(fwnode, &ep);
-			if (ret) {
-				fwnode_handle_put(fwnode);
-				return ret;
-			}
-
-			if (ep.port != port)
-				continue;
-		}
-		max_subdevs++;
-	}
-
-	/* No subdevs to add? Return here. */
-	if (max_subdevs == notifier->max_subdevs)
-		return 0;
-
-	ret = v4l2_async_notifier_realloc(notifier, max_subdevs);
-	if (ret)
-		return ret;
-
-	for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
-				     dev_fwnode(dev), fwnode)); ) {
-		struct fwnode_handle *dev_fwnode;
-		bool is_available;
-
-		dev_fwnode = fwnode_graph_get_port_parent(fwnode);
-		is_available = fwnode_device_is_available(dev_fwnode);
-		fwnode_handle_put(dev_fwnode);
-		if (!is_available)
-			continue;
-
-		if (has_port) {
-			struct fwnode_endpoint ep;
-
-			ret = fwnode_graph_parse_endpoint(fwnode, &ep);
 			if (ret)
 				break;
 
@@ -469,11 +412,6 @@ static int __v4l2_async_notifier_parse_fwnode_endpoints(
 				continue;
 		}
 
-		if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) {
-			ret = -EINVAL;
-			break;
-		}
-
 		ret = v4l2_async_notifier_fwnode_parse_endpoint(
 			dev, notifier, fwnode, asd_struct_size, parse_endpoint);
 		if (ret < 0)
@@ -544,31 +482,32 @@ static int v4l2_fwnode_reference_parse(
 	if (ret != -ENOENT && ret != -ENODATA)
 		return ret;
 
-	ret = v4l2_async_notifier_realloc(notifier,
-					  notifier->num_subdevs + index);
-	if (ret)
-		return ret;
-
 	for (index = 0; !fwnode_property_get_reference_args(
 		     dev_fwnode(dev), prop, NULL, 0, index, &args);
 	     index++) {
 		struct v4l2_async_subdev *asd;
 
-		if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) {
-			ret = -EINVAL;
-			goto error;
-		}
-
 		asd = kzalloc(sizeof(*asd), GFP_KERNEL);
 		if (!asd) {
 			ret = -ENOMEM;
 			goto error;
 		}
 
-		notifier->subdevs[notifier->num_subdevs] = asd;
 		asd->match.fwnode = args.fwnode;
 		asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
-		notifier->num_subdevs++;
+
+		ret = v4l2_async_notifier_add_subdev(notifier, asd);
+		if (ret < 0) {
+			kfree(asd);
+
+			/* not an error if asd already exists */
+			if (ret == -EEXIST) {
+				fwnode_handle_put(args.fwnode);
+				continue;
+			}
+
+			goto error;
+		}
 	}
 
 	return 0;
@@ -831,31 +770,32 @@ static int v4l2_fwnode_reference_parse_int_props(
 	if (PTR_ERR(fwnode) != -ENOENT && PTR_ERR(fwnode) != -ENODATA)
 		return PTR_ERR(fwnode);
 
-	ret = v4l2_async_notifier_realloc(notifier,
-					  notifier->num_subdevs + index);
-	if (ret)
-		return -ENOMEM;
-
 	for (index = 0; !IS_ERR((fwnode = v4l2_fwnode_reference_get_int_prop(
 					 dev_fwnode(dev), prop, index, props,
 					 nprops))); index++) {
 		struct v4l2_async_subdev *asd;
 
-		if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) {
-			ret = -EINVAL;
-			goto error;
-		}
-
 		asd = kzalloc(sizeof(struct v4l2_async_subdev), GFP_KERNEL);
 		if (!asd) {
 			ret = -ENOMEM;
 			goto error;
 		}
 
-		notifier->subdevs[notifier->num_subdevs] = asd;
 		asd->match.fwnode = fwnode;
 		asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
-		notifier->num_subdevs++;
+
+		ret = v4l2_async_notifier_add_subdev(notifier, asd);
+		if (ret < 0) {
+			kfree(asd);
+
+			/* not an error if asd already exists */
+			if (ret == -EEXIST) {
+				fwnode_handle_put(fwnode);
+				continue;
+			}
+
+			goto error;
+		}
 	}
 
 	return PTR_ERR(fwnode) == -ENOENT ? 0 : PTR_ERR(fwnode);
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index fa05905..1cafa33 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -125,7 +125,6 @@ struct v4l2_async_notifier_operations {
  *
  * @ops:	notifier operations
  * @num_subdevs: number of subdevices used in the subdevs array
- * @max_subdevs: number of subdevices allocated in the subdevs array
  * @subdevs:	array of pointers to subdevice descriptors
  * @v4l2_dev:	v4l2_device of the root notifier, NULL otherwise
  * @sd:		sub-device that registered the notifier, NULL otherwise
@@ -139,7 +138,6 @@ struct v4l2_async_notifier_operations {
 struct v4l2_async_notifier {
 	const struct v4l2_async_notifier_operations *ops;
 	unsigned int num_subdevs;
-	unsigned int max_subdevs;
 	struct v4l2_async_subdev **subdevs;
 	struct v4l2_device *v4l2_dev;
 	struct v4l2_subdev *sd;
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index c228ec1..9a4b3f8 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -247,7 +247,7 @@ typedef int (*parse_endpoint_func)(struct device *dev,
  *		    endpoint. Optional.
  *
  * Parse the fwnode endpoints of the @dev device and populate the async sub-
- * devices array of the notifier. The @parse_endpoint callback function is
+ * devices list in the notifier. The @parse_endpoint callback function is
  * called for each endpoint with the corresponding async sub-device pointer to
  * let the caller initialize the driver-specific part of the async sub-device
  * structure.
@@ -258,10 +258,11 @@ typedef int (*parse_endpoint_func)(struct device *dev,
  * This function may not be called on a registered notifier and may be called on
  * a notifier only once.
  *
- * Do not change the notifier's subdevs array, take references to the subdevs
- * array itself or change the notifier's num_subdevs field. This is because this
- * function allocates and reallocates the subdevs array based on parsing
- * endpoints.
+ * Do not allocate the notifier's subdevs array, or change the notifier's
+ * num_subdevs field. This is because this function uses
+ * @v4l2_async_notifier_add_subdev to populate the notifier's asd_list,
+ * which is in-place-of the subdevs array which must remain unallocated
+ * and unused.
  *
  * The &struct v4l2_fwnode_endpoint passed to the callback function
  * @parse_endpoint is released once the function is finished. If there is a need
@@ -303,7 +304,7 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
  * devices). In this case the driver must know which ports to parse.
  *
  * Parse the fwnode endpoints of the @dev device on a given @port and populate
- * the async sub-devices array of the notifier. The @parse_endpoint callback
+ * the async sub-devices list of the notifier. The @parse_endpoint callback
  * function is called for each endpoint with the corresponding async sub-device
  * pointer to let the caller initialize the driver-specific part of the async
  * sub-device structure.
@@ -314,10 +315,11 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
  * This function may not be called on a registered notifier and may be called on
  * a notifier only once per port.
  *
- * Do not change the notifier's subdevs array, take references to the subdevs
- * array itself or change the notifier's num_subdevs field. This is because this
- * function allocates and reallocates the subdevs array based on parsing
- * endpoints.
+ * Do not allocate the notifier's subdevs array, or change the notifier's
+ * num_subdevs field. This is because this function uses
+ * @v4l2_async_notifier_add_subdev to populate the notifier's asd_list,
+ * which is in-place-of the subdevs array which must remain unallocated
+ * and unused.
  *
  * The &struct v4l2_fwnode_endpoint passed to the callback function
  * @parse_endpoint is released once the function is finished. If there is a need
-- 
2.7.4

[PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers

[Steve Longerbeam]

Adds v4l2_async_register_fwnode_subdev(), which is a convenience function
for parsing a sub-device's fwnode port endpoints for connected remote
sub-devices, registering a sub-device notifier, and then registering
the sub-device itself.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
Changes since v2:
- fix error-out path in v4l2_async_register_fwnode_subdev() that forgot
  to put device.
Changes since v1:
- add #include <media/v4l2-subdev.h> to v4l2-fwnode.h for
  'struct v4l2_subdev' declaration.
---
 drivers/media/v4l2-core/v4l2-fwnode.c | 101 ++++++++++++++++++++++++++++++++++
 include/media/v4l2-fwnode.h           |  43 +++++++++++++++
 2 files changed, 144 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 99198b9..d42024d 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -880,6 +880,107 @@ int v4l2_async_register_subdev_sensor_common(struct v4l2_subdev *sd)
 }
 EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common);
 
+int v4l2_async_register_fwnode_subdev(
+	struct v4l2_subdev *sd, size_t asd_struct_size,
+	unsigned int *ports, unsigned int num_ports,
+	int (*parse_endpoint)(struct device *dev,
+			      struct v4l2_fwnode_endpoint *vep,
+			      struct v4l2_async_subdev *asd))
+{
+	struct v4l2_async_notifier *notifier;
+	struct device *dev = sd->dev;
+	struct fwnode_handle *fwnode;
+	unsigned int subdev_port;
+	bool is_port;
+	int ret;
+
+	if (WARN_ON(!dev))
+		return -ENODEV;
+
+	fwnode = dev_fwnode(dev);
+	if (!fwnode_device_is_available(fwnode))
+		return -ENODEV;
+
+	is_port = (is_of_node(fwnode) &&
+		   of_node_cmp(to_of_node(fwnode)->name, "port") == 0);
+
+	/*
+	 * If the sub-device is a port, only parse fwnode endpoints from
+	 * this sub-device's single port id.
+	 */
+	if (is_port) {
+		/* verify the caller did not provide a ports array */
+		if (ports)
+			return -EINVAL;
+
+		ret = fwnode_property_read_u32(fwnode, "reg", &subdev_port);
+		if (ret < 0)
+			return ret;
+
+		/*
+		 * the device given to the fwnode endpoint parsing
+		 * must be the port sub-device's parent.
+		 */
+		dev = get_device(sd->dev->parent);
+
+		if (WARN_ON(!dev))
+			return -ENODEV;
+
+		ports = &subdev_port;
+		num_ports = 1;
+	}
+
+	notifier = kzalloc(sizeof(*notifier), GFP_KERNEL);
+	if (!notifier) {
+		ret = -ENOMEM;
+		goto out_putdev;
+	}
+
+	if (!ports) {
+		ret = v4l2_async_notifier_parse_fwnode_endpoints(
+			dev, notifier, asd_struct_size, parse_endpoint);
+		if (ret < 0)
+			goto out_cleanup;
+	} else {
+		unsigned int i;
+
+		for (i = 0; i < num_ports; i++) {
+			ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
+				dev, notifier, asd_struct_size,
+				ports[i], parse_endpoint);
+			if (ret < 0)
+				goto out_cleanup;
+		}
+	}
+
+	ret = v4l2_async_subdev_notifier_register(sd, notifier);
+	if (ret < 0)
+		goto out_cleanup;
+
+	ret = v4l2_async_register_subdev(sd);
+	if (ret < 0)
+		goto out_unregister;
+
+	sd->subdev_notifier = notifier;
+
+	if (is_port)
+		put_device(dev);
+
+	return 0;
+
+out_unregister:
+	v4l2_async_notifier_unregister(notifier);
+out_cleanup:
+	v4l2_async_notifier_cleanup(notifier);
+	kfree(notifier);
+out_putdev:
+	if (is_port)
+		put_device(dev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_async_register_fwnode_subdev);
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
 MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index 9a4b3f8..4de0ac2 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -23,6 +23,7 @@
 #include <linux/types.h>
 
 #include <media/v4l2-mediabus.h>
+#include <media/v4l2-subdev.h>
 
 struct fwnode_handle;
 struct v4l2_async_notifier;
@@ -360,4 +361,46 @@ int v4l2_async_notifier_parse_fwnode_endpoints_by_port(
 int v4l2_async_notifier_parse_fwnode_sensor_common(
 	struct device *dev, struct v4l2_async_notifier *notifier);
 
+/**
+ * v4l2_async_register_fwnode_subdev - registers a sub-device to the
+ *					asynchronous sub-device framework
+ *					and parses fwnode endpoints
+ *
+ * @sd: pointer to struct &v4l2_subdev
+ * @asd_struct_size: size of the driver's async sub-device struct, including
+ *		     sizeof(struct v4l2_async_subdev). The &struct
+ *		     v4l2_async_subdev shall be the first member of
+ *		     the driver's async sub-device struct, i.e. both
+ *		     begin at the same memory address.
+ * @ports: array of port id's to parse for fwnode endpoints. If NULL, will
+ *	   parse all ports owned by the sub-device.
+ * @num_ports: number of ports in @ports array. Ignored if @ports is NULL.
+ * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
+ *		    endpoint. Optional.
+ *
+ * This function is just like v4l2_async_register_subdev() with the exception
+ * that calling it will also parse the sub-device's firmware node endpoints
+ * using v4l2_async_notifier_parse_fwnode_endpoints() or
+ * v4l2_async_notifier_parse_fwnode_endpoints_by_port(), and registers the
+ * async sub-devices. The sub-device is similarly unregistered by calling
+ * v4l2_async_unregister_subdev().
+ *
+ * This function will work as expected if the sub-device fwnode is
+ * itself a port. The endpoints of this single port are parsed using
+ * v4l2_async_notifier_parse_fwnode_endpoints_by_port(), passing the
+ * parent of the sub-device as the port's owner. The caller must not
+ * provide a @ports array, since the sub-device owns only this port.
+ *
+ * While registered, the subdev module is marked as in-use.
+ *
+ * An error is returned if the module is no longer loaded on any attempts
+ * to register it.
+ */
+int v4l2_async_register_fwnode_subdev(
+	struct v4l2_subdev *sd, size_t asd_struct_size,
+	unsigned int *ports, unsigned int num_ports,
+	int (*parse_endpoint)(struct device *dev,
+			      struct v4l2_fwnode_endpoint *vep,
+			      struct v4l2_async_subdev *asd));
+
 #endif /* _V4L2_FWNODE_H */
-- 
2.7.4

Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers

[Sakari Ailus]

Hi Steve,

On Tue, Mar 20, 2018 at 05:37:21PM -0700, Steve Longerbeam wrote:
> Adds v4l2_async_register_fwnode_subdev(), which is a convenience function
> for parsing a sub-device's fwnode port endpoints for connected remote
> sub-devices, registering a sub-device notifier, and then registering
> the sub-device itself.
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
> Changes since v2:
> - fix error-out path in v4l2_async_register_fwnode_subdev() that forgot
>   to put device.
> Changes since v1:
> - add #include <media/v4l2-subdev.h> to v4l2-fwnode.h for
>   'struct v4l2_subdev' declaration.
> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 101 ++++++++++++++++++++++++++++++++++
>  include/media/v4l2-fwnode.h           |  43 +++++++++++++++
>  2 files changed, 144 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 99198b9..d42024d 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -880,6 +880,107 @@ int v4l2_async_register_subdev_sensor_common(struct v4l2_subdev *sd)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common);
>  
> +int v4l2_async_register_fwnode_subdev(
> +	struct v4l2_subdev *sd, size_t asd_struct_size,
> +	unsigned int *ports, unsigned int num_ports,
> +	int (*parse_endpoint)(struct device *dev,
> +			      struct v4l2_fwnode_endpoint *vep,
> +			      struct v4l2_async_subdev *asd))
> +{
> +	struct v4l2_async_notifier *notifier;
> +	struct device *dev = sd->dev;
> +	struct fwnode_handle *fwnode;
> +	unsigned int subdev_port;
> +	bool is_port;
> +	int ret;
> +
> +	if (WARN_ON(!dev))
> +		return -ENODEV;
> +
> +	fwnode = dev_fwnode(dev);
> +	if (!fwnode_device_is_available(fwnode))
> +		return -ENODEV;
> +
> +	is_port = (is_of_node(fwnode) &&
> +		   of_node_cmp(to_of_node(fwnode)->name, "port") == 0);

What's the intent of this and the code below? You may not parse the graph
data structure here, it should be done in the actual firmware
implementation instead.

Also, sub-devices generally do not match ports. How sub-devices generally
correspond to fwnodes is up to the device.

> +
> +	/*
> +	 * If the sub-device is a port, only parse fwnode endpoints from
> +	 * this sub-device's single port id.
> +	 */
> +	if (is_port) {
> +		/* verify the caller did not provide a ports array */
> +		if (ports)
> +			return -EINVAL;
> +
> +		ret = fwnode_property_read_u32(fwnode, "reg", &subdev_port);
> +		if (ret < 0)
> +			return ret;
> +
> +		/*
> +		 * the device given to the fwnode endpoint parsing
> +		 * must be the port sub-device's parent.
> +		 */
> +		dev = get_device(sd->dev->parent);
> +
> +		if (WARN_ON(!dev))
> +			return -ENODEV;
> +
> +		ports = &subdev_port;
> +		num_ports = 1;
> +	}
> +
> +	notifier = kzalloc(sizeof(*notifier), GFP_KERNEL);
> +	if (!notifier) {
> +		ret = -ENOMEM;
> +		goto out_putdev;
> +	}
> +
> +	if (!ports) {
> +		ret = v4l2_async_notifier_parse_fwnode_endpoints(
> +			dev, notifier, asd_struct_size, parse_endpoint);
> +		if (ret < 0)
> +			goto out_cleanup;
> +	} else {
> +		unsigned int i;
> +
> +		for (i = 0; i < num_ports; i++) {
> +			ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> +				dev, notifier, asd_struct_size,
> +				ports[i], parse_endpoint);
> +			if (ret < 0)
> +				goto out_cleanup;
> +		}
> +	}
> +
> +	ret = v4l2_async_subdev_notifier_register(sd, notifier);
> +	if (ret < 0)
> +		goto out_cleanup;
> +
> +	ret = v4l2_async_register_subdev(sd);
> +	if (ret < 0)
> +		goto out_unregister;
> +
> +	sd->subdev_notifier = notifier;
> +
> +	if (is_port)
> +		put_device(dev);
> +
> +	return 0;
> +
> +out_unregister:
> +	v4l2_async_notifier_unregister(notifier);
> +out_cleanup:
> +	v4l2_async_notifier_cleanup(notifier);
> +	kfree(notifier);
> +out_putdev:
> +	if (is_port)
> +		put_device(dev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_async_register_fwnode_subdev);
> +
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
>  MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 9a4b3f8..4de0ac2 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -23,6 +23,7 @@
>  #include <linux/types.h>
>  
>  #include <media/v4l2-mediabus.h>
> +#include <media/v4l2-subdev.h>
>  
>  struct fwnode_handle;
>  struct v4l2_async_notifier;
> @@ -360,4 +361,46 @@ int v4l2_async_notifier_parse_fwnode_endpoints_by_port(
>  int v4l2_async_notifier_parse_fwnode_sensor_common(
>  	struct device *dev, struct v4l2_async_notifier *notifier);
>  
> +/**
> + * v4l2_async_register_fwnode_subdev - registers a sub-device to the
> + *					asynchronous sub-device framework
> + *					and parses fwnode endpoints
> + *
> + * @sd: pointer to struct &v4l2_subdev
> + * @asd_struct_size: size of the driver's async sub-device struct, including
> + *		     sizeof(struct v4l2_async_subdev). The &struct
> + *		     v4l2_async_subdev shall be the first member of
> + *		     the driver's async sub-device struct, i.e. both
> + *		     begin at the same memory address.
> + * @ports: array of port id's to parse for fwnode endpoints. If NULL, will
> + *	   parse all ports owned by the sub-device.
> + * @num_ports: number of ports in @ports array. Ignored if @ports is NULL.
> + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
> + *		    endpoint. Optional.
> + *
> + * This function is just like v4l2_async_register_subdev() with the exception
> + * that calling it will also parse the sub-device's firmware node endpoints
> + * using v4l2_async_notifier_parse_fwnode_endpoints() or
> + * v4l2_async_notifier_parse_fwnode_endpoints_by_port(), and registers the
> + * async sub-devices. The sub-device is similarly unregistered by calling
> + * v4l2_async_unregister_subdev().
> + *
> + * This function will work as expected if the sub-device fwnode is
> + * itself a port. The endpoints of this single port are parsed using
> + * v4l2_async_notifier_parse_fwnode_endpoints_by_port(), passing the
> + * parent of the sub-device as the port's owner. The caller must not
> + * provide a @ports array, since the sub-device owns only this port.
> + *
> + * While registered, the subdev module is marked as in-use.
> + *
> + * An error is returned if the module is no longer loaded on any attempts
> + * to register it.
> + */
> +int v4l2_async_register_fwnode_subdev(
> +	struct v4l2_subdev *sd, size_t asd_struct_size,
> +	unsigned int *ports, unsigned int num_ports,
> +	int (*parse_endpoint)(struct device *dev,
> +			      struct v4l2_fwnode_endpoint *vep,
> +			      struct v4l2_async_subdev *asd));
> +
>  #endif /* _V4L2_FWNODE_H */
> -- 
> 2.7.4
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers

[Steve Longerbeam]

On 04/23/2018 12:14 AM, Sakari Ailus wrote:
> Hi Steve,
>
> On Tue, Mar 20, 2018 at 05:37:21PM -0700, Steve Longerbeam wrote:
>> Adds v4l2_async_register_fwnode_subdev(), which is a convenience function
>> for parsing a sub-device's fwnode port endpoints for connected remote
>> sub-devices, registering a sub-device notifier, and then registering
>> the sub-device itself.
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> ---
>> Changes since v2:
>> - fix error-out path in v4l2_async_register_fwnode_subdev() that forgot
>>    to put device.
>> Changes since v1:
>> - add #include <media/v4l2-subdev.h> to v4l2-fwnode.h for
>>    'struct v4l2_subdev' declaration.
>> ---
>>   drivers/media/v4l2-core/v4l2-fwnode.c | 101 ++++++++++++++++++++++++++++++++++
>>   include/media/v4l2-fwnode.h           |  43 +++++++++++++++
>>   2 files changed, 144 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
>> index 99198b9..d42024d 100644
>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
>> @@ -880,6 +880,107 @@ int v4l2_async_register_subdev_sensor_common(struct v4l2_subdev *sd)
>>   }
>>   EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common);
>>   
>> +int v4l2_async_register_fwnode_subdev(
>> +	struct v4l2_subdev *sd, size_t asd_struct_size,
>> +	unsigned int *ports, unsigned int num_ports,
>> +	int (*parse_endpoint)(struct device *dev,
>> +			      struct v4l2_fwnode_endpoint *vep,
>> +			      struct v4l2_async_subdev *asd))
>> +{
>> +	struct v4l2_async_notifier *notifier;
>> +	struct device *dev = sd->dev;
>> +	struct fwnode_handle *fwnode;
>> +	unsigned int subdev_port;
>> +	bool is_port;
>> +	int ret;
>> +
>> +	if (WARN_ON(!dev))
>> +		return -ENODEV;
>> +
>> +	fwnode = dev_fwnode(dev);
>> +	if (!fwnode_device_is_available(fwnode))
>> +		return -ENODEV;
>> +
>> +	is_port = (is_of_node(fwnode) &&
>> +		   of_node_cmp(to_of_node(fwnode)->name, "port") == 0);
> What's the intent of this and the code below? You may not parse the graph
> data structure here, it should be done in the actual firmware
> implementation instead.

The i.MX6 CSI sub-device registers itself from a port fwnode, so
the intent of the is_port code below is to support the i.MX6 CSI.

I can remove the is_port checks, but it means
v4l2_async_register_fwnode_subdev() won't be usable by the CSI
sub-device.

>
> Also, sub-devices generally do not match ports.

Yes that's generally true, sub-devices generally match to port parent
nodes. But I do know of one other sub-device that buck that trend.
The ADV748x CSI-2 output sub-devices match against endpoint nodes.

>   How sub-devices generally
> correspond to fwnodes is up to the device.

What do you think of adding a v4l2_async_register_port_fwnode_subdev(),
and a v4l2_async_register_endpoint_fwnode_subdev() to support such
sub-devices?


Steve


>
>> +
>> +	/*
>> +	 * If the sub-device is a port, only parse fwnode endpoints from
>> +	 * this sub-device's single port id.
>> +	 */
>> +	if (is_port) {
>> +		/* verify the caller did not provide a ports array */
>> +		if (ports)
>> +			return -EINVAL;
>> +
>> +		ret = fwnode_property_read_u32(fwnode, "reg", &subdev_port);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		/*
>> +		 * the device given to the fwnode endpoint parsing
>> +		 * must be the port sub-device's parent.
>> +		 */
>> +		dev = get_device(sd->dev->parent);
>> +
>> +		if (WARN_ON(!dev))
>> +			return -ENODEV;
>> +
>> +		ports = &subdev_port;
>> +		num_ports = 1;
>> +	}
>> +
>> +	notifier = kzalloc(sizeof(*notifier), GFP_KERNEL);
>> +	if (!notifier) {
>> +		ret = -ENOMEM;
>> +		goto out_putdev;
>> +	}
>> +
>> +	if (!ports) {
>> +		ret = v4l2_async_notifier_parse_fwnode_endpoints(
>> +			dev, notifier, asd_struct_size, parse_endpoint);
>> +		if (ret < 0)
>> +			goto out_cleanup;
>> +	} else {
>> +		unsigned int i;
>> +
>> +		for (i = 0; i < num_ports; i++) {
>> +			ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
>> +				dev, notifier, asd_struct_size,
>> +				ports[i], parse_endpoint);
>> +			if (ret < 0)
>> +				goto out_cleanup;
>> +		}
>> +	}
>> +
>> +	ret = v4l2_async_subdev_notifier_register(sd, notifier);
>> +	if (ret < 0)
>> +		goto out_cleanup;
>> +
>> +	ret = v4l2_async_register_subdev(sd);
>> +	if (ret < 0)
>> +		goto out_unregister;
>> +
>> +	sd->subdev_notifier = notifier;
>> +
>> +	if (is_port)
>> +		put_device(dev);
>> +
>> +	return 0;
>> +
>> +out_unregister:
>> +	v4l2_async_notifier_unregister(notifier);
>> +out_cleanup:
>> +	v4l2_async_notifier_cleanup(notifier);
>> +	kfree(notifier);
>> +out_putdev:
>> +	if (is_port)
>> +		put_device(dev);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_async_register_fwnode_subdev);
>> +
>>   MODULE_LICENSE("GPL");
>>   MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
>>   MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
>> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
>> index 9a4b3f8..4de0ac2 100644
>> --- a/include/media/v4l2-fwnode.h
>> +++ b/include/media/v4l2-fwnode.h
>> @@ -23,6 +23,7 @@
>>   #include <linux/types.h>
>>   
>>   #include <media/v4l2-mediabus.h>
>> +#include <media/v4l2-subdev.h>
>>   
>>   struct fwnode_handle;
>>   struct v4l2_async_notifier;
>> @@ -360,4 +361,46 @@ int v4l2_async_notifier_parse_fwnode_endpoints_by_port(
>>   int v4l2_async_notifier_parse_fwnode_sensor_common(
>>   	struct device *dev, struct v4l2_async_notifier *notifier);
>>   
>> +/**
>> + * v4l2_async_register_fwnode_subdev - registers a sub-device to the
>> + *					asynchronous sub-device framework
>> + *					and parses fwnode endpoints
>> + *
>> + * @sd: pointer to struct &v4l2_subdev
>> + * @asd_struct_size: size of the driver's async sub-device struct, including
>> + *		     sizeof(struct v4l2_async_subdev). The &struct
>> + *		     v4l2_async_subdev shall be the first member of
>> + *		     the driver's async sub-device struct, i.e. both
>> + *		     begin at the same memory address.
>> + * @ports: array of port id's to parse for fwnode endpoints. If NULL, will
>> + *	   parse all ports owned by the sub-device.
>> + * @num_ports: number of ports in @ports array. Ignored if @ports is NULL.
>> + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
>> + *		    endpoint. Optional.
>> + *
>> + * This function is just like v4l2_async_register_subdev() with the exception
>> + * that calling it will also parse the sub-device's firmware node endpoints
>> + * using v4l2_async_notifier_parse_fwnode_endpoints() or
>> + * v4l2_async_notifier_parse_fwnode_endpoints_by_port(), and registers the
>> + * async sub-devices. The sub-device is similarly unregistered by calling
>> + * v4l2_async_unregister_subdev().
>> + *
>> + * This function will work as expected if the sub-device fwnode is
>> + * itself a port. The endpoints of this single port are parsed using
>> + * v4l2_async_notifier_parse_fwnode_endpoints_by_port(), passing the
>> + * parent of the sub-device as the port's owner. The caller must not
>> + * provide a @ports array, since the sub-device owns only this port.
>> + *
>> + * While registered, the subdev module is marked as in-use.
>> + *
>> + * An error is returned if the module is no longer loaded on any attempts
>> + * to register it.
>> + */
>> +int v4l2_async_register_fwnode_subdev(
>> +	struct v4l2_subdev *sd, size_t asd_struct_size,
>> +	unsigned int *ports, unsigned int num_ports,
>> +	int (*parse_endpoint)(struct device *dev,
>> +			      struct v4l2_fwnode_endpoint *vep,
>> +			      struct v4l2_async_subdev *asd));
>> +
>>   #endif /* _V4L2_FWNODE_H */
>> -- 
>> 2.7.4
>>

Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers

[Sakari Ailus]

Hi Steve,

Again, sorry about the delay. This thread got buried in my inbox. :-(
Please see my reply below.

On Mon, Apr 23, 2018 at 11:00:22AM -0700, Steve Longerbeam wrote:
> 
> 
> On 04/23/2018 12:14 AM, Sakari Ailus wrote:
> > Hi Steve,
> > 
> > On Tue, Mar 20, 2018 at 05:37:21PM -0700, Steve Longerbeam wrote:
> > > Adds v4l2_async_register_fwnode_subdev(), which is a convenience function
> > > for parsing a sub-device's fwnode port endpoints for connected remote
> > > sub-devices, registering a sub-device notifier, and then registering
> > > the sub-device itself.
> > > 
> > > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> > > ---
> > > Changes since v2:
> > > - fix error-out path in v4l2_async_register_fwnode_subdev() that forgot
> > >    to put device.
> > > Changes since v1:
> > > - add #include <media/v4l2-subdev.h> to v4l2-fwnode.h for
> > >    'struct v4l2_subdev' declaration.
> > > ---
> > >   drivers/media/v4l2-core/v4l2-fwnode.c | 101 ++++++++++++++++++++++++++++++++++
> > >   include/media/v4l2-fwnode.h           |  43 +++++++++++++++
> > >   2 files changed, 144 insertions(+)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > index 99198b9..d42024d 100644
> > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > @@ -880,6 +880,107 @@ int v4l2_async_register_subdev_sensor_common(struct v4l2_subdev *sd)
> > >   }
> > >   EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common);
> > > +int v4l2_async_register_fwnode_subdev(
> > > +	struct v4l2_subdev *sd, size_t asd_struct_size,
> > > +	unsigned int *ports, unsigned int num_ports,
> > > +	int (*parse_endpoint)(struct device *dev,
> > > +			      struct v4l2_fwnode_endpoint *vep,
> > > +			      struct v4l2_async_subdev *asd))
> > > +{
> > > +	struct v4l2_async_notifier *notifier;
> > > +	struct device *dev = sd->dev;
> > > +	struct fwnode_handle *fwnode;
> > > +	unsigned int subdev_port;
> > > +	bool is_port;
> > > +	int ret;
> > > +
> > > +	if (WARN_ON(!dev))
> > > +		return -ENODEV;
> > > +
> > > +	fwnode = dev_fwnode(dev);
> > > +	if (!fwnode_device_is_available(fwnode))
> > > +		return -ENODEV;
> > > +
> > > +	is_port = (is_of_node(fwnode) &&
> > > +		   of_node_cmp(to_of_node(fwnode)->name, "port") == 0);
> > What's the intent of this and the code below? You may not parse the graph
> > data structure here, it should be done in the actual firmware
> > implementation instead.
> 
> The i.MX6 CSI sub-device registers itself from a port fwnode, so
> the intent of the is_port code below is to support the i.MX6 CSI.
> 
> I can remove the is_port checks, but it means
> v4l2_async_register_fwnode_subdev() won't be usable by the CSI
> sub-device.

This won't scale. Instead, I think we'd need to separate registering
sub-devices (through async sub-devices) and binding them with the driver
that registered the notifier. Or at least change how that process works: a
single sub-device can well be bound to multiple notifiers, or multiple
times to the same notifier while it may be registered only once.

> 
> > 
> > Also, sub-devices generally do not match ports.
> 
> Yes that's generally true, sub-devices generally match to port parent
> nodes. But I do know of one other sub-device that buck that trend.
> The ADV748x CSI-2 output sub-devices match against endpoint nodes.

Endpoints, yes, but not ports.

> 
> >   How sub-devices generally
> > correspond to fwnodes is up to the device.
> 
> What do you think of adding a v4l2_async_register_port_fwnode_subdev(),
> and a v4l2_async_register_endpoint_fwnode_subdev() to support such
> sub-devices?

The endpoint is more specific than a port, so why the port and not the
endpoint?

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers

[Steve Longerbeam]

On 05/08/2018 03:28 AM, Sakari Ailus wrote:
> Hi Steve,
>
> Again, sorry about the delay. This thread got buried in my inbox. :-(
> Please see my reply below.
>
> On Mon, Apr 23, 2018 at 11:00:22AM -0700, Steve Longerbeam wrote:
>>
>> On 04/23/2018 12:14 AM, Sakari Ailus wrote:
>>> Hi Steve,
>>>
>>> On Tue, Mar 20, 2018 at 05:37:21PM -0700, Steve Longerbeam wrote:
>>>> Adds v4l2_async_register_fwnode_subdev(), which is a convenience function
>>>> for parsing a sub-device's fwnode port endpoints for connected remote
>>>> sub-devices, registering a sub-device notifier, and then registering
>>>> the sub-device itself.
>>>>
>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>> ---
>>>> Changes since v2:
>>>> - fix error-out path in v4l2_async_register_fwnode_subdev() that forgot
>>>>     to put device.
>>>> Changes since v1:
>>>> - add #include <media/v4l2-subdev.h> to v4l2-fwnode.h for
>>>>     'struct v4l2_subdev' declaration.
>>>> ---
>>>>    drivers/media/v4l2-core/v4l2-fwnode.c | 101 ++++++++++++++++++++++++++++++++++
>>>>    include/media/v4l2-fwnode.h           |  43 +++++++++++++++
>>>>    2 files changed, 144 insertions(+)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
>>>> index 99198b9..d42024d 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
>>>> @@ -880,6 +880,107 @@ int v4l2_async_register_subdev_sensor_common(struct v4l2_subdev *sd)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common);
>>>> +int v4l2_async_register_fwnode_subdev(
>>>> +	struct v4l2_subdev *sd, size_t asd_struct_size,
>>>> +	unsigned int *ports, unsigned int num_ports,
>>>> +	int (*parse_endpoint)(struct device *dev,
>>>> +			      struct v4l2_fwnode_endpoint *vep,
>>>> +			      struct v4l2_async_subdev *asd))
>>>> +{
>>>> +	struct v4l2_async_notifier *notifier;
>>>> +	struct device *dev = sd->dev;
>>>> +	struct fwnode_handle *fwnode;
>>>> +	unsigned int subdev_port;
>>>> +	bool is_port;
>>>> +	int ret;
>>>> +
>>>> +	if (WARN_ON(!dev))
>>>> +		return -ENODEV;
>>>> +
>>>> +	fwnode = dev_fwnode(dev);
>>>> +	if (!fwnode_device_is_available(fwnode))
>>>> +		return -ENODEV;
>>>> +
>>>> +	is_port = (is_of_node(fwnode) &&
>>>> +		   of_node_cmp(to_of_node(fwnode)->name, "port") == 0);
>>> What's the intent of this and the code below? You may not parse the graph
>>> data structure here, it should be done in the actual firmware
>>> implementation instead.
>> The i.MX6 CSI sub-device registers itself from a port fwnode, so
>> the intent of the is_port code below is to support the i.MX6 CSI.
>>
>> I can remove the is_port checks, but it means
>> v4l2_async_register_fwnode_subdev() won't be usable by the CSI
>> sub-device.
> This won't scale.

The vast majority of sub-devices register themselves as
port parent nodes. So for now at least, I think
v4l2_async_register_fwnode_subdev() could be useful to many
platforms.

>   Instead, I think we'd need to separate registering
> sub-devices (through async sub-devices) and binding them with the driver
> that registered the notifier. Or at least change how that process works: a
> single sub-device can well be bound to multiple notifiers,

Ok, that is certainly not the case now, a sub-device can only
be bound to a single notifier.

>   or multiple
> times to the same notifier while it may be registered only once.

Anyway, this is a future generalization if I understand you
correctly. Not something to deal with here.

>
>>> Also, sub-devices generally do not match ports.
>> Yes that's generally true, sub-devices generally match to port parent
>> nodes. But I do know of one other sub-device that buck that trend.
>> The ADV748x CSI-2 output sub-devices match against endpoint nodes.
> Endpoints, yes, but not ports.

Well, the imx CSI registers from a port node.

>
>>>    How sub-devices generally
>>> correspond to fwnodes is up to the device.
>> What do you think of adding a v4l2_async_register_port_fwnode_subdev(),
>> and a v4l2_async_register_endpoint_fwnode_subdev() to support such
>> sub-devices?
> The endpoint is more specific than a port, so why the port and not the
> endpoint?

Do you mean there should be a
v4l2_async_register_endpoint_fwnode_subdev() but not
v4l2_async_register_endpoint_port_subdev()?

Steve

Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers

[Sakari Ailus]

On Tue, May 08, 2018 at 08:55:04PM -0700, Steve Longerbeam wrote:
> 
> 
> On 05/08/2018 03:28 AM, Sakari Ailus wrote:
> > Hi Steve,
> > 
> > Again, sorry about the delay. This thread got buried in my inbox. :-(
> > Please see my reply below.
> > 
> > On Mon, Apr 23, 2018 at 11:00:22AM -0700, Steve Longerbeam wrote:
> > > 
> > > On 04/23/2018 12:14 AM, Sakari Ailus wrote:
> > > > Hi Steve,
> > > > 
> > > > On Tue, Mar 20, 2018 at 05:37:21PM -0700, Steve Longerbeam wrote:
> > > > > Adds v4l2_async_register_fwnode_subdev(), which is a convenience function
> > > > > for parsing a sub-device's fwnode port endpoints for connected remote
> > > > > sub-devices, registering a sub-device notifier, and then registering
> > > > > the sub-device itself.
> > > > > 
> > > > > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> > > > > ---
> > > > > Changes since v2:
> > > > > - fix error-out path in v4l2_async_register_fwnode_subdev() that forgot
> > > > >     to put device.
> > > > > Changes since v1:
> > > > > - add #include <media/v4l2-subdev.h> to v4l2-fwnode.h for
> > > > >     'struct v4l2_subdev' declaration.
> > > > > ---
> > > > >    drivers/media/v4l2-core/v4l2-fwnode.c | 101 ++++++++++++++++++++++++++++++++++
> > > > >    include/media/v4l2-fwnode.h           |  43 +++++++++++++++
> > > > >    2 files changed, 144 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > > > index 99198b9..d42024d 100644
> > > > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > > > @@ -880,6 +880,107 @@ int v4l2_async_register_subdev_sensor_common(struct v4l2_subdev *sd)
> > > > >    }
> > > > >    EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common);
> > > > > +int v4l2_async_register_fwnode_subdev(
> > > > > +	struct v4l2_subdev *sd, size_t asd_struct_size,
> > > > > +	unsigned int *ports, unsigned int num_ports,
> > > > > +	int (*parse_endpoint)(struct device *dev,
> > > > > +			      struct v4l2_fwnode_endpoint *vep,
> > > > > +			      struct v4l2_async_subdev *asd))
> > > > > +{
> > > > > +	struct v4l2_async_notifier *notifier;
> > > > > +	struct device *dev = sd->dev;
> > > > > +	struct fwnode_handle *fwnode;
> > > > > +	unsigned int subdev_port;
> > > > > +	bool is_port;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (WARN_ON(!dev))
> > > > > +		return -ENODEV;
> > > > > +
> > > > > +	fwnode = dev_fwnode(dev);
> > > > > +	if (!fwnode_device_is_available(fwnode))
> > > > > +		return -ENODEV;
> > > > > +
> > > > > +	is_port = (is_of_node(fwnode) &&
> > > > > +		   of_node_cmp(to_of_node(fwnode)->name, "port") == 0);
> > > > What's the intent of this and the code below? You may not parse the graph
> > > > data structure here, it should be done in the actual firmware
> > > > implementation instead.
> > > The i.MX6 CSI sub-device registers itself from a port fwnode, so
> > > the intent of the is_port code below is to support the i.MX6 CSI.
> > > 
> > > I can remove the is_port checks, but it means
> > > v4l2_async_register_fwnode_subdev() won't be usable by the CSI
> > > sub-device.
> > This won't scale.
> 
> The vast majority of sub-devices register themselves as
> port parent nodes. So for now at least, I think
> v4l2_async_register_fwnode_subdev() could be useful to many
> platforms.
> 
> >   Instead, I think we'd need to separate registering
> > sub-devices (through async sub-devices) and binding them with the driver
> > that registered the notifier. Or at least change how that process works: a
> > single sub-device can well be bound to multiple notifiers,
> 
> Ok, that is certainly not the case now, a sub-device can only
> be bound to a single notifier.
> 
> >   or multiple
> > times to the same notifier while it may be registered only once.
> 
> Anyway, this is a future generalization if I understand you
> correctly. Not something to deal with here.
> 
> > 
> > > > Also, sub-devices generally do not match ports.
> > > Yes that's generally true, sub-devices generally match to port parent
> > > nodes. But I do know of one other sub-device that buck that trend.
> > > The ADV748x CSI-2 output sub-devices match against endpoint nodes.
> > Endpoints, yes, but not ports.
> 
> Well, the imx CSI registers from a port node.

I looked at the imx driver and it appears to be parsing DT without much
help from the frameworks; graph or V4L2 fwnode. Is there a reason to do so,
other than it's a staging driver? :-)

Do you happen to have any DT source (or bindings) for this? It'd help to
understand what is the aim here.

> 
> > 
> > > >    How sub-devices generally
> > > > correspond to fwnodes is up to the device.
> > > What do you think of adding a v4l2_async_register_port_fwnode_subdev(),
> > > and a v4l2_async_register_endpoint_fwnode_subdev() to support such
> > > sub-devices?
> > The endpoint is more specific than a port, so why the port and not the
> > endpoint?
> 
> Do you mean there should be a
> v4l2_async_register_endpoint_fwnode_subdev() but not
> v4l2_async_register_endpoint_port_subdev()?
> 
> Steve
> 

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers

[Steve Longerbeam]

On 06/26/2018 12:45 AM, Sakari Ailus wrote:
> On Tue, May 08, 2018 at 08:55:04PM -0700, Steve Longerbeam wrote:
>>
>> On 05/08/2018 03:28 AM, Sakari Ailus wrote:
>>> Hi Steve,
>>>
>>> Again, sorry about the delay. This thread got buried in my inbox. :-(
>>> Please see my reply below.
>>>
>>> On Mon, Apr 23, 2018 at 11:00:22AM -0700, Steve Longerbeam wrote:
>>>> On 04/23/2018 12:14 AM, Sakari Ailus wrote:
>>>>> Hi Steve,
>>>>>
>>>>> On Tue, Mar 20, 2018 at 05:37:21PM -0700, Steve Longerbeam wrote:
>>>>>> Adds v4l2_async_register_fwnode_subdev(), which is a convenience function
>>>>>> for parsing a sub-device's fwnode port endpoints for connected remote
>>>>>> sub-devices, registering a sub-device notifier, and then registering
>>>>>> the sub-device itself.
>>>>>>
>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>> ---
>>>>>> Changes since v2:
>>>>>> - fix error-out path in v4l2_async_register_fwnode_subdev() that forgot
>>>>>>      to put device.
>>>>>> Changes since v1:
>>>>>> - add #include <media/v4l2-subdev.h> to v4l2-fwnode.h for
>>>>>>      'struct v4l2_subdev' declaration.
>>>>>> ---
>>>>>>     drivers/media/v4l2-core/v4l2-fwnode.c | 101 ++++++++++++++++++++++++++++++++++
>>>>>>     include/media/v4l2-fwnode.h           |  43 +++++++++++++++
>>>>>>     2 files changed, 144 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
>>>>>> index 99198b9..d42024d 100644
>>>>>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
>>>>>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
>>>>>> @@ -880,6 +880,107 @@ int v4l2_async_register_subdev_sensor_common(struct v4l2_subdev *sd)
>>>>>>     }
>>>>>>     EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common);
>>>>>> +int v4l2_async_register_fwnode_subdev(
>>>>>> +	struct v4l2_subdev *sd, size_t asd_struct_size,
>>>>>> +	unsigned int *ports, unsigned int num_ports,
>>>>>> +	int (*parse_endpoint)(struct device *dev,
>>>>>> +			      struct v4l2_fwnode_endpoint *vep,
>>>>>> +			      struct v4l2_async_subdev *asd))
>>>>>> +{
>>>>>> +	struct v4l2_async_notifier *notifier;
>>>>>> +	struct device *dev = sd->dev;
>>>>>> +	struct fwnode_handle *fwnode;
>>>>>> +	unsigned int subdev_port;
>>>>>> +	bool is_port;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	if (WARN_ON(!dev))
>>>>>> +		return -ENODEV;
>>>>>> +
>>>>>> +	fwnode = dev_fwnode(dev);
>>>>>> +	if (!fwnode_device_is_available(fwnode))
>>>>>> +		return -ENODEV;
>>>>>> +
>>>>>> +	is_port = (is_of_node(fwnode) &&
>>>>>> +		   of_node_cmp(to_of_node(fwnode)->name, "port") == 0);
>>>>> What's the intent of this and the code below? You may not parse the graph
>>>>> data structure here, it should be done in the actual firmware
>>>>> implementation instead.
>>>> The i.MX6 CSI sub-device registers itself from a port fwnode, so
>>>> the intent of the is_port code below is to support the i.MX6 CSI.
>>>>
>>>> I can remove the is_port checks, but it means
>>>> v4l2_async_register_fwnode_subdev() won't be usable by the CSI
>>>> sub-device.
>>> This won't scale.
>> The vast majority of sub-devices register themselves as
>> port parent nodes. So for now at least, I think
>> v4l2_async_register_fwnode_subdev() could be useful to many
>> platforms.
>>
>>>    Instead, I think we'd need to separate registering
>>> sub-devices (through async sub-devices) and binding them with the driver
>>> that registered the notifier. Or at least change how that process works: a
>>> single sub-device can well be bound to multiple notifiers,
>> Ok, that is certainly not the case now, a sub-device can only
>> be bound to a single notifier.
>>
>>>    or multiple
>>> times to the same notifier while it may be registered only once.
>> Anyway, this is a future generalization if I understand you
>> correctly. Not something to deal with here.
>>
>>>>> Also, sub-devices generally do not match ports.
>>>> Yes that's generally true, sub-devices generally match to port parent
>>>> nodes. But I do know of one other sub-device that buck that trend.
>>>> The ADV748x CSI-2 output sub-devices match against endpoint nodes.
>>> Endpoints, yes, but not ports.
>> Well, the imx CSI registers from a port node.
> I looked at the imx driver and it appears to be parsing DT without much
> help from the frameworks; graph or V4L2 fwnode. Is there a reason to do so,
> other than it's a staging driver? :-)

That's the whole point of this patchset. It gets rid of the code in imx
that discovers subdevices by recursively walking the graph, replacing
with the subdev notifier framework.


>
> Do you happen to have any DT source (or bindings) for this?

Documentation/devicetree/bindings/media/imx.txt.

For example DT source, it's all in the imx6 reference boards,
see imx6qdl-sabresd.dtsi for example.

>   It'd help to understand what is the aim here.

The aim of what? Of this specific commit? I'll reprint it here:

Adds v4l2_async_register_fwnode_subdev(), which is a convenience function
for parsing a sub-device's fwnode port endpoints for connected remote
sub-devices, registering a sub-device notifier, and then registering
the sub-device itself.


Steve

Re: [PATCH v3 05/13] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers

[Sakari Ailus]

Hi Steve,

On Tue, May 08, 2018 at 08:55:04PM -0700, Steve Longerbeam wrote:
> 
> 
> On 05/08/2018 03:28 AM, Sakari Ailus wrote:
> > Hi Steve,
> > 
> > Again, sorry about the delay. This thread got buried in my inbox. :-(
> > Please see my reply below.
> > 
> > On Mon, Apr 23, 2018 at 11:00:22AM -0700, Steve Longerbeam wrote:
> > > 
> > > On 04/23/2018 12:14 AM, Sakari Ailus wrote:
> > > > Hi Steve,
> > > > 
> > > > On Tue, Mar 20, 2018 at 05:37:21PM -0700, Steve Longerbeam wrote:
> > > > > Adds v4l2_async_register_fwnode_subdev(), which is a convenience function
> > > > > for parsing a sub-device's fwnode port endpoints for connected remote
> > > > > sub-devices, registering a sub-device notifier, and then registering
> > > > > the sub-device itself.
> > > > > 
> > > > > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> > > > > ---
> > > > > Changes since v2:
> > > > > - fix error-out path in v4l2_async_register_fwnode_subdev() that forgot
> > > > >     to put device.
> > > > > Changes since v1:
> > > > > - add #include <media/v4l2-subdev.h> to v4l2-fwnode.h for
> > > > >     'struct v4l2_subdev' declaration.
> > > > > ---
> > > > >    drivers/media/v4l2-core/v4l2-fwnode.c | 101 ++++++++++++++++++++++++++++++++++
> > > > >    include/media/v4l2-fwnode.h           |  43 +++++++++++++++
> > > > >    2 files changed, 144 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > > > index 99198b9..d42024d 100644
> > > > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > > > @@ -880,6 +880,107 @@ int v4l2_async_register_subdev_sensor_common(struct v4l2_subdev *sd)
> > > > >    }
> > > > >    EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common);
> > > > > +int v4l2_async_register_fwnode_subdev(
> > > > > +	struct v4l2_subdev *sd, size_t asd_struct_size,
> > > > > +	unsigned int *ports, unsigned int num_ports,
> > > > > +	int (*parse_endpoint)(struct device *dev,
> > > > > +			      struct v4l2_fwnode_endpoint *vep,
> > > > > +			      struct v4l2_async_subdev *asd))
> > > > > +{
> > > > > +	struct v4l2_async_notifier *notifier;
> > > > > +	struct device *dev = sd->dev;
> > > > > +	struct fwnode_handle *fwnode;
> > > > > +	unsigned int subdev_port;
> > > > > +	bool is_port;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (WARN_ON(!dev))
> > > > > +		return -ENODEV;
> > > > > +
> > > > > +	fwnode = dev_fwnode(dev);
> > > > > +	if (!fwnode_device_is_available(fwnode))
> > > > > +		return -ENODEV;
> > > > > +
> > > > > +	is_port = (is_of_node(fwnode) &&
> > > > > +		   of_node_cmp(to_of_node(fwnode)->name, "port") == 0);
> > > > What's the intent of this and the code below? You may not parse the graph
> > > > data structure here, it should be done in the actual firmware
> > > > implementation instead.
> > > The i.MX6 CSI sub-device registers itself from a port fwnode, so
> > > the intent of the is_port code below is to support the i.MX6 CSI.
> > > 
> > > I can remove the is_port checks, but it means
> > > v4l2_async_register_fwnode_subdev() won't be usable by the CSI
> > > sub-device.
> > This won't scale.
> 
> The vast majority of sub-devices register themselves as
> port parent nodes. So for now at least, I think
> v4l2_async_register_fwnode_subdev() could be useful to many
> platforms.

It's because the graph bindings define that the port nodes are sub-nodes of
a device node (see Documentation/devicetree/bindings/graph.txt). It's not
exactly the same as doing this in DT but still the kernel implementation
pretty much assumes the same.

> 
> >   Instead, I think we'd need to separate registering
> > sub-devices (through async sub-devices) and binding them with the driver
> > that registered the notifier. Or at least change how that process works: a
> > single sub-device can well be bound to multiple notifiers,
> 
> Ok, that is certainly not the case now, a sub-device can only
> be bound to a single notifier.
> 
> >   or multiple
> > times to the same notifier while it may be registered only once.
> 
> Anyway, this is a future generalization if I understand you
> correctly. Not something to deal with here.

Indeed; just FYI.

> 
> > 
> > > > Also, sub-devices generally do not match ports.
> > > Yes that's generally true, sub-devices generally match to port parent
> > > nodes. But I do know of one other sub-device that buck that trend.
> > > The ADV748x CSI-2 output sub-devices match against endpoint nodes.
> > Endpoints, yes, but not ports.
> 
> Well, the imx CSI registers from a port node.
> 
> > 
> > > >    How sub-devices generally
> > > > correspond to fwnodes is up to the device.
> > > What do you think of adding a v4l2_async_register_port_fwnode_subdev(),
> > > and a v4l2_async_register_endpoint_fwnode_subdev() to support such
> > > sub-devices?
> > The endpoint is more specific than a port, so why the port and not the
> > endpoint?
> 
> Do you mean there should be a
> v4l2_async_register_endpoint_fwnode_subdev() but not
> v4l2_async_register_endpoint_port_subdev()?
> 
> Steve
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

[PATCH v3 06/13] media: platform: video-mux: Register a subdev notifier

[Steve Longerbeam]

Parse neighbor remote devices on the video muxes input ports, add them to a
subdev notifier, and register the subdev notifier for the video mux, by
calling v4l2_async_register_fwnode_subdev().

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
Changes since v2:
- none
Changes since v1:
- add #include <linux/slab.h> for kcalloc() declaration.
---
 drivers/media/platform/video-mux.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
index ee89ad7..b93b0af 100644
--- a/drivers/media/platform/video-mux.c
+++ b/drivers/media/platform/video-mux.c
@@ -21,8 +21,10 @@
 #include <linux/of.h>
 #include <linux/of_graph.h>
 #include <linux/platform_device.h>
+#include <linux/slab.h>
 #include <media/v4l2-async.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
 struct video_mux {
@@ -193,6 +195,38 @@ static const struct v4l2_subdev_ops video_mux_subdev_ops = {
 	.video = &video_mux_subdev_video_ops,
 };
 
+static int video_mux_parse_endpoint(struct device *dev,
+				    struct v4l2_fwnode_endpoint *vep,
+				    struct v4l2_async_subdev *asd)
+{
+	/*
+	 * it's not an error if remote is missing on a video-mux
+	 * input port, return -ENOTCONN to skip this endpoint with
+	 * no error.
+	 */
+	return fwnode_device_is_available(asd->match.fwnode) ? 0 : -ENOTCONN;
+}
+
+static int video_mux_async_register(struct video_mux *vmux,
+				    unsigned int num_pads)
+{
+	unsigned int i, *ports;
+	int ret;
+
+	ports = kcalloc(num_pads - 1, sizeof(*ports), GFP_KERNEL);
+	if (!ports)
+		return -ENOMEM;
+	for (i = 0; i < num_pads - 1; i++)
+		ports[i] = i;
+
+	ret = v4l2_async_register_fwnode_subdev(
+		&vmux->subdev, sizeof(struct v4l2_async_subdev),
+		ports, num_pads - 1, video_mux_parse_endpoint);
+
+	kfree(ports);
+	return ret;
+}
+
 static int video_mux_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -258,7 +292,7 @@ static int video_mux_probe(struct platform_device *pdev)
 
 	vmux->subdev.entity.ops = &video_mux_ops;
 
-	return v4l2_async_register_subdev(&vmux->subdev);
+	return video_mux_async_register(vmux, num_pads);
 }
 
 static int video_mux_remove(struct platform_device *pdev)
-- 
2.7.4

Re: [PATCH v3 06/13] media: platform: video-mux: Register a subdev notifier

[Hans Verkuil]

On 03/21/18 01:37, Steve Longerbeam wrote:
> Parse neighbor remote devices on the video muxes input ports, add them to a
> subdev notifier, and register the subdev notifier for the video mux, by
> calling v4l2_async_register_fwnode_subdev().
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
> Changes since v2:
> - none
> Changes since v1:
> - add #include <linux/slab.h> for kcalloc() declaration.
> ---
>  drivers/media/platform/video-mux.c | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> index ee89ad7..b93b0af 100644
> --- a/drivers/media/platform/video-mux.c
> +++ b/drivers/media/platform/video-mux.c
> @@ -21,8 +21,10 @@
>  #include <linux/of.h>
>  #include <linux/of_graph.h>
>  #include <linux/platform_device.h>
> +#include <linux/slab.h>
>  #include <media/v4l2-async.h>
>  #include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
>  struct video_mux {
> @@ -193,6 +195,38 @@ static const struct v4l2_subdev_ops video_mux_subdev_ops = {
>  	.video = &video_mux_subdev_video_ops,
>  };
>  
> +static int video_mux_parse_endpoint(struct device *dev,
> +				    struct v4l2_fwnode_endpoint *vep,
> +				    struct v4l2_async_subdev *asd)
> +{
> +	/*
> +	 * it's not an error if remote is missing on a video-mux
> +	 * input port, return -ENOTCONN to skip this endpoint with
> +	 * no error.
> +	 */
> +	return fwnode_device_is_available(asd->match.fwnode) ? 0 : -ENOTCONN;
> +}
> +
> +static int video_mux_async_register(struct video_mux *vmux,
> +				    unsigned int num_pads)
> +{
> +	unsigned int i, *ports;
> +	int ret;
> +
> +	ports = kcalloc(num_pads - 1, sizeof(*ports), GFP_KERNEL);
> +	if (!ports)
> +		return -ENOMEM;
> +	for (i = 0; i < num_pads - 1; i++)
> +		ports[i] = i;
> +
> +	ret = v4l2_async_register_fwnode_subdev(
> +		&vmux->subdev, sizeof(struct v4l2_async_subdev),
> +		ports, num_pads - 1, video_mux_parse_endpoint);
> +
> +	kfree(ports);
> +	return ret;
> +}
> +
>  static int video_mux_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> @@ -258,7 +292,7 @@ static int video_mux_probe(struct platform_device *pdev)
>  
>  	vmux->subdev.entity.ops = &video_mux_ops;
>  
> -	return v4l2_async_register_subdev(&vmux->subdev);
> +	return video_mux_async_register(vmux, num_pads);

I would prefer to change the last argument to 'num_pads - 1' and drop the ' - 1'
part in the video_mux_async_register() function. Perhaps renaming the num_pads
argument there to num_input_pads.

Regards,

	Hans

>  }
>  
>  static int video_mux_remove(struct platform_device *pdev)
> 

Re: [PATCH v3 06/13] media: platform: video-mux: Register a subdev notifier

[Steve Longerbeam]

Hi Hans,


On 04/20/2018 05:28 AM, Hans Verkuil wrote:
> On 03/21/18 01:37, Steve Longerbeam wrote:
>> Parse neighbor remote devices on the video muxes input ports, add them to a
>> subdev notifier, and register the subdev notifier for the video mux, by
>> calling v4l2_async_register_fwnode_subdev().
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> ---
>> Changes since v2:
>> - none
>> Changes since v1:
>> - add #include <linux/slab.h> for kcalloc() declaration.
>> ---
>>   drivers/media/platform/video-mux.c | 36 +++++++++++++++++++++++++++++++++++-
>>   1 file changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
>> index ee89ad7..b93b0af 100644
>> --- a/drivers/media/platform/video-mux.c
>> +++ b/drivers/media/platform/video-mux.c
>> @@ -21,8 +21,10 @@
>>   #include <linux/of.h>
>>   #include <linux/of_graph.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/slab.h>
>>   #include <media/v4l2-async.h>
>>   #include <media/v4l2-device.h>
>> +#include <media/v4l2-fwnode.h>
>>   #include <media/v4l2-subdev.h>
>>   
>>   struct video_mux {
>> @@ -193,6 +195,38 @@ static const struct v4l2_subdev_ops video_mux_subdev_ops = {
>>   	.video = &video_mux_subdev_video_ops,
>>   };
>>   
>> +static int video_mux_parse_endpoint(struct device *dev,
>> +				    struct v4l2_fwnode_endpoint *vep,
>> +				    struct v4l2_async_subdev *asd)
>> +{
>> +	/*
>> +	 * it's not an error if remote is missing on a video-mux
>> +	 * input port, return -ENOTCONN to skip this endpoint with
>> +	 * no error.
>> +	 */
>> +	return fwnode_device_is_available(asd->match.fwnode) ? 0 : -ENOTCONN;
>> +}
>> +
>> +static int video_mux_async_register(struct video_mux *vmux,
>> +				    unsigned int num_pads)
>> +{
>> +	unsigned int i, *ports;
>> +	int ret;
>> +
>> +	ports = kcalloc(num_pads - 1, sizeof(*ports), GFP_KERNEL);
>> +	if (!ports)
>> +		return -ENOMEM;
>> +	for (i = 0; i < num_pads - 1; i++)
>> +		ports[i] = i;
>> +
>> +	ret = v4l2_async_register_fwnode_subdev(
>> +		&vmux->subdev, sizeof(struct v4l2_async_subdev),
>> +		ports, num_pads - 1, video_mux_parse_endpoint);
>> +
>> +	kfree(ports);
>> +	return ret;
>> +}
>> +
>>   static int video_mux_probe(struct platform_device *pdev)
>>   {
>>   	struct device_node *np = pdev->dev.of_node;
>> @@ -258,7 +292,7 @@ static int video_mux_probe(struct platform_device *pdev)
>>   
>>   	vmux->subdev.entity.ops = &video_mux_ops;
>>   
>> -	return v4l2_async_register_subdev(&vmux->subdev);
>> +	return video_mux_async_register(vmux, num_pads);
> I would prefer to change the last argument to 'num_pads - 1' and drop the ' - 1'
> part in the video_mux_async_register() function. Perhaps renaming the num_pads
> argument there to num_input_pads.

Yes makes sense. I will do that.

Steve

[PATCH v3 07/13] media: imx: csi: Register a subdev notifier

[Steve Longerbeam]

Parse neighbor remote devices on the CSI port, add them to a subdev
notifier, and register the subdev notifier for the CSI, by calling
v4l2_async_register_fwnode_subdev().

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/staging/media/imx/imx-media-csi.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 5a195f8..87cf277 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1736,6 +1736,13 @@ static const struct v4l2_subdev_internal_ops csi_internal_ops = {
 	.unregistered = csi_unregistered,
 };
 
+static int imx_csi_parse_endpoint(struct device *dev,
+				  struct v4l2_fwnode_endpoint *vep,
+				  struct v4l2_async_subdev *asd)
+{
+	return fwnode_device_is_available(asd->match.fwnode) ? 0 : -EINVAL;
+}
+
 static int imx_csi_probe(struct platform_device *pdev)
 {
 	struct ipu_client_platformdata *pdata;
@@ -1802,7 +1809,9 @@ static int imx_csi_probe(struct platform_device *pdev)
 		goto free;
 	}
 
-	ret = v4l2_async_register_subdev(&priv->sd);
+	ret = v4l2_async_register_fwnode_subdev(
+		&priv->sd, sizeof(struct v4l2_async_subdev),
+		NULL, 0, imx_csi_parse_endpoint);
 	if (ret)
 		goto free;
 
-- 
2.7.4

[PATCH v3 08/13] media: imx: mipi csi-2: Register a subdev notifier

[Steve Longerbeam]

Parse neighbor remote devices on the MIPI CSI-2 input port, add
them to a subdev notifier, and register the subdev notifier for the
MIPI CSI-2 receiver, by calling v4l2_async_register_fwnode_subdev().

csi2_parse_endpoints() is modified to be the parse_endpoint callback.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/staging/media/imx/imx6-mipi-csi2.c | 31 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
index ceeeb30..94eb9a1 100644
--- a/drivers/staging/media/imx/imx6-mipi-csi2.c
+++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
@@ -551,35 +551,34 @@ static const struct v4l2_subdev_internal_ops csi2_internal_ops = {
 	.registered = csi2_registered,
 };
 
-static int csi2_parse_endpoints(struct csi2_dev *csi2)
+static int csi2_parse_endpoint(struct device *dev,
+			       struct v4l2_fwnode_endpoint *vep,
+			       struct v4l2_async_subdev *asd)
 {
-	struct device_node *node = csi2->dev->of_node;
-	struct device_node *epnode;
-	struct v4l2_fwnode_endpoint ep;
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct csi2_dev *csi2 = sd_to_dev(sd);
 
-	epnode = of_graph_get_endpoint_by_regs(node, 0, -1);
-	if (!epnode) {
-		v4l2_err(&csi2->sd, "failed to get sink endpoint node\n");
+	if (!fwnode_device_is_available(asd->match.fwnode)) {
+		v4l2_err(&csi2->sd, "remote is not available\n");
 		return -EINVAL;
 	}
 
-	v4l2_fwnode_endpoint_parse(of_fwnode_handle(epnode), &ep);
-	of_node_put(epnode);
-
-	if (ep.bus_type != V4L2_MBUS_CSI2) {
+	if (vep->bus_type != V4L2_MBUS_CSI2) {
 		v4l2_err(&csi2->sd, "invalid bus type, must be MIPI CSI2\n");
 		return -EINVAL;
 	}
 
-	csi2->bus = ep.bus.mipi_csi2;
+	csi2->bus = vep->bus.mipi_csi2;
 
 	dev_dbg(csi2->dev, "data lanes: %d\n", csi2->bus.num_data_lanes);
 	dev_dbg(csi2->dev, "flags: 0x%08x\n", csi2->bus.flags);
+
 	return 0;
 }
 
 static int csi2_probe(struct platform_device *pdev)
 {
+	unsigned int sink_port = 0;
 	struct csi2_dev *csi2;
 	struct resource *res;
 	int ret;
@@ -601,10 +600,6 @@ static int csi2_probe(struct platform_device *pdev)
 	csi2->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
 	csi2->sd.grp_id = IMX_MEDIA_GRP_ID_CSI2;
 
-	ret = csi2_parse_endpoints(csi2);
-	if (ret)
-		return ret;
-
 	csi2->pllref_clk = devm_clk_get(&pdev->dev, "ref");
 	if (IS_ERR(csi2->pllref_clk)) {
 		v4l2_err(&csi2->sd, "failed to get pll reference clock\n");
@@ -654,7 +649,9 @@ static int csi2_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, &csi2->sd);
 
-	ret = v4l2_async_register_subdev(&csi2->sd);
+	ret = v4l2_async_register_fwnode_subdev(
+		&csi2->sd, sizeof(struct v4l2_async_subdev),
+		&sink_port, 1, csi2_parse_endpoint);
 	if (ret)
 		goto dphy_off;
 
-- 
2.7.4

[PATCH v3 09/13] media: staging/imx: of: Remove recursive graph walk

[Steve Longerbeam]

After moving to subdev notifiers, it's no longer necessary to recursively
walk the OF graph, because the subdev notifiers will discover and add
devices from the graph for us.

So the recursive of_parse_subdev() function is gone, replaced with
of_add_csi() which adds only the CSI port fwnodes to the imx-media
root notifier.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/staging/media/imx/imx-media-of.c | 106 +++----------------------------
 1 file changed, 8 insertions(+), 98 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-of.c b/drivers/staging/media/imx/imx-media-of.c
index acde372..1c91754 100644
--- a/drivers/staging/media/imx/imx-media-of.c
+++ b/drivers/staging/media/imx/imx-media-of.c
@@ -20,74 +20,19 @@
 #include <video/imx-ipu-v3.h>
 #include "imx-media.h"
 
-static int of_get_port_count(const struct device_node *np)
+static int of_add_csi(struct imx_media_dev *imxmd, struct device_node *csi_np)
 {
-	struct device_node *ports, *child;
-	int num = 0;
-
-	/* check if this node has a ports subnode */
-	ports = of_get_child_by_name(np, "ports");
-	if (ports)
-		np = ports;
-
-	for_each_child_of_node(np, child)
-		if (of_node_cmp(child->name, "port") == 0)
-			num++;
-
-	of_node_put(ports);
-	return num;
-}
-
-/*
- * find the remote device node given local endpoint node
- */
-static bool of_get_remote(struct device_node *epnode,
-			  struct device_node **remote_node)
-{
-	struct device_node *rp, *rpp;
-	struct device_node *remote;
-	bool is_csi_port;
-
-	rp = of_graph_get_remote_port(epnode);
-	rpp = of_graph_get_remote_port_parent(epnode);
-
-	if (of_device_is_compatible(rpp, "fsl,imx6q-ipu")) {
-		/* the remote is one of the CSI ports */
-		remote = rp;
-		of_node_put(rpp);
-		is_csi_port = true;
-	} else {
-		remote = rpp;
-		of_node_put(rp);
-		is_csi_port = false;
-	}
-
-	if (!of_device_is_available(remote)) {
-		of_node_put(remote);
-		*remote_node = NULL;
-	} else {
-		*remote_node = remote;
-	}
-
-	return is_csi_port;
-}
-
-static int
-of_parse_subdev(struct imx_media_dev *imxmd, struct device_node *sd_np,
-		bool is_csi_port)
-{
-	int i, num_ports, ret;
+	int ret;
 
-	if (!of_device_is_available(sd_np)) {
+	if (!of_device_is_available(csi_np)) {
 		dev_dbg(imxmd->md.dev, "%s: %s not enabled\n", __func__,
-			sd_np->name);
+			csi_np->name);
 		/* unavailable is not an error */
 		return 0;
 	}
 
-	/* register this subdev with async notifier */
-	ret = imx_media_add_async_subdev(imxmd, of_fwnode_handle(sd_np),
-					 NULL);
+	/* add CSI fwnode to async notifier */
+	ret = imx_media_add_async_subdev(imxmd, of_fwnode_handle(csi_np), NULL);
 	if (ret) {
 		if (ret == -EEXIST) {
 			/* already added, everything is fine */
@@ -98,42 +43,7 @@ of_parse_subdev(struct imx_media_dev *imxmd, struct device_node *sd_np,
 		return ret;
 	}
 
-	/*
-	 * the ipu-csi has one sink port. The source pads are not
-	 * represented in the device tree by port nodes, but are
-	 * described by the internal pads and links later.
-	 */
-	num_ports = is_csi_port ? 1 : of_get_port_count(sd_np);
-
-	for (i = 0; i < num_ports; i++) {
-		struct device_node *epnode = NULL, *port, *remote_np;
-
-		port = is_csi_port ? sd_np : of_graph_get_port_by_id(sd_np, i);
-		if (!port)
-			continue;
-
-		for_each_child_of_node(port, epnode) {
-			bool remote_is_csi;
-
-			remote_is_csi = of_get_remote(epnode, &remote_np);
-			if (!remote_np)
-				continue;
-
-			ret = of_parse_subdev(imxmd, remote_np, remote_is_csi);
-			of_node_put(remote_np);
-			if (ret)
-				break;
-		}
-
-		if (port != sd_np)
-			of_node_put(port);
-		if (ret) {
-			of_node_put(epnode);
-			break;
-		}
-	}
-
-	return ret;
+	return 0;
 }
 
 int imx_media_add_of_subdevs(struct imx_media_dev *imxmd,
@@ -147,7 +57,7 @@ int imx_media_add_of_subdevs(struct imx_media_dev *imxmd,
 		if (!csi_np)
 			break;
 
-		ret = of_parse_subdev(imxmd, csi_np, true);
+		ret = of_add_csi(imxmd, csi_np);
 		of_node_put(csi_np);
 		if (ret)
 			return ret;
-- 
2.7.4

[PATCH v3 10/13] media: staging/imx: Loop through all registered subdevs for media links

[Steve Longerbeam]

The root imx-media notifier no longer sees all bound subdevices because
some of them will be bound to subdev notifiers. So imx_media_create_links()
now needs to loop through all subdevices registered with the v4l2-device,
not just the ones in the root notifier's done list. This should be safe
because imx_media_create_of_links() checks if a fwnode link already
exists before creating.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/staging/media/imx/imx-media-dev.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
index 289d775..4d00ed3 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -175,7 +175,7 @@ static int imx_media_subdev_bound(struct v4l2_async_notifier *notifier,
 }
 
 /*
- * create the media links for all subdevs that registered async.
+ * Create the media links for all subdevs that registered.
  * Called after all async subdevs have bound.
  */
 static int imx_media_create_links(struct v4l2_async_notifier *notifier)
@@ -184,14 +184,7 @@ static int imx_media_create_links(struct v4l2_async_notifier *notifier)
 	struct v4l2_subdev *sd;
 	int ret;
 
-	/*
-	 * Only links are created between subdevices that are known
-	 * to the async notifier. If there are other non-async subdevices,
-	 * they were created internally by some subdevice (smiapp is one
-	 * example). In those cases it is expected the subdevice is
-	 * responsible for creating those internal links.
-	 */
-	list_for_each_entry(sd, &notifier->done, async_list) {
+	list_for_each_entry(sd, &imxmd->v4l2_dev.subdevs, list) {
 		switch (sd->grp_id) {
 		case IMX_MEDIA_GRP_ID_VDIC:
 		case IMX_MEDIA_GRP_ID_IC_PRP:
@@ -211,7 +204,10 @@ static int imx_media_create_links(struct v4l2_async_notifier *notifier)
 				imx_media_create_csi_of_links(imxmd, sd);
 			break;
 		default:
-			/* this is an external fwnode subdev */
+			/*
+			 * if this subdev has fwnode links, create media
+			 * links for them.
+			 */
 			imx_media_create_of_links(imxmd, sd);
 			break;
 		}
-- 
2.7.4

[PATCH v3 11/13] media: staging/imx: Rename root notifier

[Steve Longerbeam]

Rename the imx-media root async notifier from "subdev_notifier" to
simply "notifier", so as not to confuse it with true subdev notifiers.
No functional changes.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/staging/media/imx/imx-media-dev.c | 14 +++++++-------
 drivers/staging/media/imx/imx-media.h     |  2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
index 4d00ed3..dd4702a 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -29,7 +29,7 @@
 
 static inline struct imx_media_dev *notifier2dev(struct v4l2_async_notifier *n)
 {
-	return container_of(n, struct imx_media_dev, subdev_notifier);
+	return container_of(n, struct imx_media_dev, notifier);
 }
 
 /*
@@ -113,7 +113,7 @@ int imx_media_add_async_subdev(struct imx_media_dev *imxmd,
 
 	list_add_tail(&imxasd->list, &imxmd->asd_list);
 
-	imxmd->subdev_notifier.num_subdevs++;
+	imxmd->notifier.num_subdevs++;
 
 	dev_dbg(imxmd->md.dev, "%s: added %s, match type %s\n",
 		__func__, np ? np->name : devname, np ? "FWNODE" : "DEVNAME");
@@ -532,7 +532,7 @@ static int imx_media_probe(struct platform_device *pdev)
 		goto unreg_dev;
 	}
 
-	num_subdevs = imxmd->subdev_notifier.num_subdevs;
+	num_subdevs = imxmd->notifier.num_subdevs;
 
 	/* no subdevs? just bail */
 	if (num_subdevs == 0) {
@@ -552,10 +552,10 @@ static int imx_media_probe(struct platform_device *pdev)
 		subdevs[i++] = &imxasd->asd;
 
 	/* prepare the async subdev notifier and register it */
-	imxmd->subdev_notifier.subdevs = subdevs;
-	imxmd->subdev_notifier.ops = &imx_media_subdev_ops;
+	imxmd->notifier.subdevs = subdevs;
+	imxmd->notifier.ops = &imx_media_subdev_ops;
 	ret = v4l2_async_notifier_register(&imxmd->v4l2_dev,
-					   &imxmd->subdev_notifier);
+					   &imxmd->notifier);
 	if (ret) {
 		v4l2_err(&imxmd->v4l2_dev,
 			 "v4l2_async_notifier_register failed with %d\n", ret);
@@ -580,7 +580,7 @@ static int imx_media_remove(struct platform_device *pdev)
 
 	v4l2_info(&imxmd->v4l2_dev, "Removing imx-media\n");
 
-	v4l2_async_notifier_unregister(&imxmd->subdev_notifier);
+	v4l2_async_notifier_unregister(&imxmd->notifier);
 	imx_media_remove_internal_subdevs(imxmd);
 	v4l2_device_unregister(&imxmd->v4l2_dev);
 	media_device_unregister(&imxmd->md);
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index e945e0e..7edb18a 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -148,7 +148,7 @@ struct imx_media_dev {
 
 	/* for async subdev registration */
 	struct list_head asd_list;
-	struct v4l2_async_notifier subdev_notifier;
+	struct v4l2_async_notifier notifier;
 };
 
 enum codespace_sel {
-- 
2.7.4

[PATCH v3 12/13] media: staging/imx: Switch to v4l2_async_notifier_add_subdev

[Steve Longerbeam]

Switch to v4l2_async_notifier_add_subdev() when adding async subdevs
to the imx-media root notifier. This removes the need to check for
an already added asd, since v4l2_async_notifier_add_subdev() does this
check. Also no need to allocate a subdevs array when registering the
root notifier, or keeping an internal master asd_list, since this is
moved to the notifier's asd_list.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/staging/media/imx/imx-media-dev.c         | 110 ++++++----------------
 drivers/staging/media/imx/imx-media-internal-sd.c |   5 +-
 drivers/staging/media/imx/imx-media.h             |   4 +-
 3 files changed, 32 insertions(+), 87 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
index dd4702a..f67ec8e 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -33,43 +33,10 @@ static inline struct imx_media_dev *notifier2dev(struct v4l2_async_notifier *n)
 }
 
 /*
- * Find an asd by fwnode or device name. This is called during
- * driver load to form the async subdev list and bind them.
- */
-static struct v4l2_async_subdev *
-find_async_subdev(struct imx_media_dev *imxmd,
-		  struct fwnode_handle *fwnode,
-		  const char *devname)
-{
-	struct imx_media_async_subdev *imxasd;
-	struct v4l2_async_subdev *asd;
-
-	list_for_each_entry(imxasd, &imxmd->asd_list, list) {
-		asd = &imxasd->asd;
-		switch (asd->match_type) {
-		case V4L2_ASYNC_MATCH_FWNODE:
-			if (fwnode && asd->match.fwnode == fwnode)
-				return asd;
-			break;
-		case V4L2_ASYNC_MATCH_DEVNAME:
-			if (devname && !strcmp(asd->match.device_name,
-					       devname))
-				return asd;
-			break;
-		default:
-			break;
-		}
-	}
-
-	return NULL;
-}
-
-
-/*
- * Adds a subdev to the async subdev list. If fwnode is non-NULL, adds
- * the async as a V4L2_ASYNC_MATCH_FWNODE match type, otherwise as
- * a V4L2_ASYNC_MATCH_DEVNAME match type using the dev_name of the
- * given platform_device. This is called during driver load when
+ * Adds a subdev to the root notifier's async subdev list. If fwnode is
+ * non-NULL, adds the async as a V4L2_ASYNC_MATCH_FWNODE match type,
+ * otherwise as a V4L2_ASYNC_MATCH_DEVNAME match type using the dev_name
+ * of the given platform_device. This is called during driver load when
  * forming the async subdev list.
  */
 int imx_media_add_async_subdev(struct imx_media_dev *imxmd,
@@ -80,28 +47,17 @@ int imx_media_add_async_subdev(struct imx_media_dev *imxmd,
 	struct imx_media_async_subdev *imxasd;
 	struct v4l2_async_subdev *asd;
 	const char *devname = NULL;
-	int ret = 0;
+	int ret;
 
-	mutex_lock(&imxmd->mutex);
+	imxasd = kzalloc(sizeof(*imxasd), GFP_KERNEL);
+	if (!imxasd)
+		return -ENOMEM;
+
+	asd = &imxasd->asd;
 
 	if (pdev)
 		devname = dev_name(&pdev->dev);
 
-	/* return -EEXIST if this asd already added */
-	if (find_async_subdev(imxmd, fwnode, devname)) {
-		dev_dbg(imxmd->md.dev, "%s: already added %s\n",
-			__func__, np ? np->name : devname);
-		ret = -EEXIST;
-		goto out;
-	}
-
-	imxasd = devm_kzalloc(imxmd->md.dev, sizeof(*imxasd), GFP_KERNEL);
-	if (!imxasd) {
-		ret = -ENOMEM;
-		goto out;
-	}
-	asd = &imxasd->asd;
-
 	if (fwnode) {
 		asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
 		asd->match.fwnode = fwnode;
@@ -111,16 +67,19 @@ int imx_media_add_async_subdev(struct imx_media_dev *imxmd,
 		imxasd->pdev = pdev;
 	}
 
-	list_add_tail(&imxasd->list, &imxmd->asd_list);
-
-	imxmd->notifier.num_subdevs++;
+	ret = v4l2_async_notifier_add_subdev(&imxmd->notifier, asd);
+	if (ret < 0) {
+		if (ret == -EEXIST)
+			dev_dbg(imxmd->md.dev, "%s: already added %s\n",
+				__func__, np ? np->name : devname);
+		kfree(imxasd);
+		return ret;
+	}
 
 	dev_dbg(imxmd->md.dev, "%s: added %s, match type %s\n",
 		__func__, np ? np->name : devname, np ? "FWNODE" : "DEVNAME");
 
-out:
-	mutex_unlock(&imxmd->mutex);
-	return ret;
+	return 0;
 }
 
 /*
@@ -483,10 +442,8 @@ static int imx_media_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *node = dev->of_node;
-	struct imx_media_async_subdev *imxasd;
-	struct v4l2_async_subdev **subdevs;
 	struct imx_media_dev *imxmd;
-	int num_subdevs, i, ret;
+	int ret;
 
 	imxmd = devm_kzalloc(dev, sizeof(*imxmd), GFP_KERNEL);
 	if (!imxmd)
@@ -515,44 +472,29 @@ static int imx_media_probe(struct platform_device *pdev)
 
 	dev_set_drvdata(imxmd->v4l2_dev.dev, imxmd);
 
-	INIT_LIST_HEAD(&imxmd->asd_list);
 	INIT_LIST_HEAD(&imxmd->vdev_list);
 
 	ret = imx_media_add_of_subdevs(imxmd, node);
 	if (ret) {
 		v4l2_err(&imxmd->v4l2_dev,
 			 "add_of_subdevs failed with %d\n", ret);
-		goto unreg_dev;
+		goto notifier_cleanup;
 	}
 
 	ret = imx_media_add_internal_subdevs(imxmd);
 	if (ret) {
 		v4l2_err(&imxmd->v4l2_dev,
 			 "add_internal_subdevs failed with %d\n", ret);
-		goto unreg_dev;
+		goto notifier_cleanup;
 	}
 
-	num_subdevs = imxmd->notifier.num_subdevs;
-
 	/* no subdevs? just bail */
-	if (num_subdevs == 0) {
+	if (imxmd->notifier.num_subdevs == 0) {
 		ret = -ENODEV;
-		goto unreg_dev;
+		goto notifier_cleanup;
 	}
 
-	subdevs = devm_kzalloc(imxmd->md.dev, sizeof(*subdevs) * num_subdevs,
-			       GFP_KERNEL);
-	if (!subdevs) {
-		ret = -ENOMEM;
-		goto unreg_dev;
-	}
-
-	i = 0;
-	list_for_each_entry(imxasd, &imxmd->asd_list, list)
-		subdevs[i++] = &imxasd->asd;
-
 	/* prepare the async subdev notifier and register it */
-	imxmd->notifier.subdevs = subdevs;
 	imxmd->notifier.ops = &imx_media_subdev_ops;
 	ret = v4l2_async_notifier_register(&imxmd->v4l2_dev,
 					   &imxmd->notifier);
@@ -566,7 +508,8 @@ static int imx_media_probe(struct platform_device *pdev)
 
 del_int:
 	imx_media_remove_internal_subdevs(imxmd);
-unreg_dev:
+notifier_cleanup:
+	v4l2_async_notifier_cleanup(&imxmd->notifier);
 	v4l2_device_unregister(&imxmd->v4l2_dev);
 cleanup:
 	media_device_cleanup(&imxmd->md);
@@ -582,6 +525,7 @@ static int imx_media_remove(struct platform_device *pdev)
 
 	v4l2_async_notifier_unregister(&imxmd->notifier);
 	imx_media_remove_internal_subdevs(imxmd);
+	v4l2_async_notifier_cleanup(&imxmd->notifier);
 	v4l2_device_unregister(&imxmd->v4l2_dev);
 	media_device_unregister(&imxmd->md);
 	media_device_cleanup(&imxmd->md);
diff --git a/drivers/staging/media/imx/imx-media-internal-sd.c b/drivers/staging/media/imx/imx-media-internal-sd.c
index daf66c2..0fdc45d 100644
--- a/drivers/staging/media/imx/imx-media-internal-sd.c
+++ b/drivers/staging/media/imx/imx-media-internal-sd.c
@@ -350,8 +350,11 @@ int imx_media_add_internal_subdevs(struct imx_media_dev *imxmd)
 void imx_media_remove_internal_subdevs(struct imx_media_dev *imxmd)
 {
 	struct imx_media_async_subdev *imxasd;
+	struct v4l2_async_subdev *asd;
+
+	list_for_each_entry(asd, &imxmd->notifier.asd_list, asd_list) {
+		imxasd = to_imx_media_asd(asd);
 
-	list_for_each_entry(imxasd, &imxmd->asd_list, list) {
 		if (!imxasd->pdev)
 			continue;
 
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index 7edb18a..44532cd 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -117,12 +117,11 @@ struct imx_media_internal_sd_platformdata {
 	int ipu_id;
 };
 
-
 struct imx_media_async_subdev {
+	/* the base asd - must be first in this struct */
 	struct v4l2_async_subdev asd;
 	/* the platform device of IPU-internal subdevs */
 	struct platform_device *pdev;
-	struct list_head list;
 };
 
 static inline struct imx_media_async_subdev *
@@ -147,7 +146,6 @@ struct imx_media_dev {
 	struct ipu_soc *ipu[2];
 
 	/* for async subdev registration */
-	struct list_head asd_list;
 	struct v4l2_async_notifier notifier;
 };
 
-- 
2.7.4

[PATCH v3 13/13] media: staging/imx: TODO: Remove one assumption about OF graph parsing

[Steve Longerbeam]

The move to subdev notifiers fixes one assumption of OF graph parsing.
If a subdevice has non-video related ports, the subdev driver knows not
to follow those ports when adding remote devices to its subdev notifier.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/staging/media/imx/TODO | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/media/imx/TODO b/drivers/staging/media/imx/TODO
index 9eb7326..aeeb154 100644
--- a/drivers/staging/media/imx/TODO
+++ b/drivers/staging/media/imx/TODO
@@ -17,29 +17,15 @@
   decided whether this feature is useful enough to make it generally
   available by exporting to v4l2-core.
 
-- The OF graph is walked at probe time to form the list of fwnodes to
-  be passed to v4l2_async_notifier_register(), starting from the IPU
-  CSI ports. And after all async subdevices have been bound,
-  v4l2_fwnode_parse_link() is used to form the media links between
-  the entities discovered by walking the OF graph.
+- After all async subdevices have been bound, v4l2_fwnode_parse_link()
+  is used to form the media links between the devices discovered in
+  the OF graph.
 
   While this approach allows support for arbitrary OF graphs, there
   are some assumptions for this to work:
 
-  1. All port parent nodes reachable in the graph from the IPU CSI
-     ports bind to V4L2 async subdevice drivers.
-
-     If a device has mixed-use ports such as video plus audio, the
-     endpoints from the audio ports are followed to devices that must
-     bind to V4L2 subdevice drivers, and not for example, to an ALSA
-     driver or a non-V4L2 media driver. If the device were bound to
-     such a driver, imx-media would never get an async completion
-     notification because the device fwnode was added to the async
-     list, but the driver does not interface with the V4L2 async
-     framework.
-
-  2. Every port reachable in the graph is treated as a media pad,
-     owned by the V4L2 subdevice that is bound to the port's parent.
+  1. If a port owned by a device in the graph has endpoint nodes, the
+     port is treated as a media pad.
 
      This presents problems for devices that don't make this port = pad
      assumption. Examples are SMIAPP compatible cameras which define only
@@ -54,9 +40,8 @@
      possible long-term solution is to implement a subdev API that
      maps a port id to a media pad index.
 
-  3. Every endpoint of a port reachable in the graph is treated as
-     a media link, between V4L2 subdevices that are bound to the
-     port parents of the local and remote endpoints.
+  2. Every endpoint of a port owned by a device in the graph is treated
+     as a media link.
 
      Which means a port must not contain mixed-use endpoints, they
      must all refer to media links between V4L2 subdevices.
-- 
2.7.4

Re: [PATCH v3 00/13] media: imx: Switch to subdev notifiers

[Steve Longerbeam]

Hi Sakari, Hans, Mauro,

What is the status on this patch-set?

Thanks,
Steve

On 03/20/2018 05:37 PM, Steve Longerbeam wrote:
> This patchset converts the imx-media driver and its dependent
> subdevs to use subdev notifiers.
>
> There are a couple shortcomings in v4l2-core that prevented
> subdev notifiers from working correctly in imx-media:
>
> 1. v4l2_async_notifier_fwnode_parse_endpoint() treats a fwnode
>     endpoint that is not connected to a remote device as an error.
>     But in the case of the video-mux subdev, this is not an error,
>     it is OK if some of the mux inputs have no connection. Also,
>     Documentation/devicetree/bindings/media/video-interfaces.txt explicitly
>     states that the 'remote-endpoint' property is optional. So the first
>     patch is a small modification to ignore empty endpoints in
>     v4l2_async_notifier_fwnode_parse_endpoint() and allow
>     __v4l2_async_notifier_parse_fwnode_endpoints() to continue to
>     parse the remaining port endpoints of the device.
>
> 2. In the imx-media graph, multiple subdevs will encounter the same
>     upstream subdev (such as the imx6-mipi-csi2 receiver), and so
>     v4l2_async_notifier_parse_fwnode_endpoints() will add imx6-mipi-csi2
>     multiple times. This is treated as an error by
>     v4l2_async_notifier_register() later.
>
>     To get around this problem, add an v4l2_async_notifier_add_subdev()
>     which first verifies the provided asd does not already exist in the
>     given notifier asd list or in other registered notifiers. If the asd
>     exists, the function returns -EEXIST and it's up to the caller to
>     decide if that is an error (in imx-media case it is never an error).
>
>     Patches 2-4 deal with adding that support.
>
> 3. Patch 5 adds v4l2_async_register_fwnode_subdev(), which is a
>     convenience function for parsing a subdev's fwnode port endpoints
>     for connected remote subdevs, registering a subdev notifier, and
>     then registering the sub-device itself.
>
> The remaining patches update the subdev drivers to register a
> subdev notifier with endpoint parsing, and the changes to imx-media
> to support that.
>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> Acked-by: Philipp Zabel <p.zabel@pengutronix.de>
>
> History:
> v3:
> - code optimization in asd_equal(), and remove unneeded braces,
>    suggested by Sakari Ailus.
> - add a NULL asd pointer check to v4l2_async_notifier_asd_valid().
> - fix an error-out path in v4l2_async_register_fwnode_subdev() that
>    forgot to put device.
>
> v2:
> - don't pass an empty endpoint to the parse_endpoint callback,
>    v4l2_async_notifier_fwnode_parse_endpoint() now just ignores them
>    and returns success.
> - Fix a couple compile warnings and errors seen in i386 and sh archs.
>
>
> Steve Longerbeam (13):
>    media: v4l2-fwnode: ignore endpoints that have no remote port parent
>    media: v4l2: async: Allow searching for asd of any type
>    media: v4l2: async: Add v4l2_async_notifier_add_subdev
>    media: v4l2-fwnode: Switch to v4l2_async_notifier_add_subdev
>    media: v4l2-fwnode: Add a convenience function for registering subdevs
>      with notifiers
>    media: platform: video-mux: Register a subdev notifier
>    media: imx: csi: Register a subdev notifier
>    media: imx: mipi csi-2: Register a subdev notifier
>    media: staging/imx: of: Remove recursive graph walk
>    media: staging/imx: Loop through all registered subdevs for media
>      links
>    media: staging/imx: Rename root notifier
>    media: staging/imx: Switch to v4l2_async_notifier_add_subdev
>    media: staging/imx: TODO: Remove one assumption about OF graph parsing
>
>   drivers/media/pci/intel/ipu3/ipu3-cio2.c          |  10 +-
>   drivers/media/platform/video-mux.c                |  36 ++-
>   drivers/media/v4l2-core/v4l2-async.c              | 268 ++++++++++++++++------
>   drivers/media/v4l2-core/v4l2-fwnode.c             | 231 +++++++++++--------
>   drivers/staging/media/imx/TODO                    |  29 +--
>   drivers/staging/media/imx/imx-media-csi.c         |  11 +-
>   drivers/staging/media/imx/imx-media-dev.c         | 134 +++--------
>   drivers/staging/media/imx/imx-media-internal-sd.c |   5 +-
>   drivers/staging/media/imx/imx-media-of.c          | 106 +--------
>   drivers/staging/media/imx/imx-media.h             |   6 +-
>   drivers/staging/media/imx/imx6-mipi-csi2.c        |  31 ++-
>   include/media/v4l2-async.h                        |  24 +-
>   include/media/v4l2-fwnode.h                       |  65 +++++-
>   13 files changed, 534 insertions(+), 422 deletions(-)
>

Re: [PATCH v3 00/13] media: imx: Switch to subdev notifiers

[Hans Verkuil]

Steve, Sakari,

Any progress on this? The imx7 patch series depends on this so that can't
move forward until this is merged.

Regards,

	Hans

On 21/03/18 01:37, Steve Longerbeam wrote:
> This patchset converts the imx-media driver and its dependent
> subdevs to use subdev notifiers.
> 
> There are a couple shortcomings in v4l2-core that prevented
> subdev notifiers from working correctly in imx-media:
> 
> 1. v4l2_async_notifier_fwnode_parse_endpoint() treats a fwnode
>    endpoint that is not connected to a remote device as an error.
>    But in the case of the video-mux subdev, this is not an error,
>    it is OK if some of the mux inputs have no connection. Also,
>    Documentation/devicetree/bindings/media/video-interfaces.txt explicitly
>    states that the 'remote-endpoint' property is optional. So the first
>    patch is a small modification to ignore empty endpoints in
>    v4l2_async_notifier_fwnode_parse_endpoint() and allow
>    __v4l2_async_notifier_parse_fwnode_endpoints() to continue to
>    parse the remaining port endpoints of the device.
> 
> 2. In the imx-media graph, multiple subdevs will encounter the same
>    upstream subdev (such as the imx6-mipi-csi2 receiver), and so
>    v4l2_async_notifier_parse_fwnode_endpoints() will add imx6-mipi-csi2
>    multiple times. This is treated as an error by
>    v4l2_async_notifier_register() later.
> 
>    To get around this problem, add an v4l2_async_notifier_add_subdev()
>    which first verifies the provided asd does not already exist in the
>    given notifier asd list or in other registered notifiers. If the asd
>    exists, the function returns -EEXIST and it's up to the caller to
>    decide if that is an error (in imx-media case it is never an error).
> 
>    Patches 2-4 deal with adding that support.
> 
> 3. Patch 5 adds v4l2_async_register_fwnode_subdev(), which is a
>    convenience function for parsing a subdev's fwnode port endpoints
>    for connected remote subdevs, registering a subdev notifier, and
>    then registering the sub-device itself.
> 
> The remaining patches update the subdev drivers to register a
> subdev notifier with endpoint parsing, and the changes to imx-media
> to support that.
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> Acked-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> History:
> v3:
> - code optimization in asd_equal(), and remove unneeded braces,
>   suggested by Sakari Ailus.
> - add a NULL asd pointer check to v4l2_async_notifier_asd_valid().
> - fix an error-out path in v4l2_async_register_fwnode_subdev() that
>   forgot to put device.
> 
> v2:
> - don't pass an empty endpoint to the parse_endpoint callback, 
>   v4l2_async_notifier_fwnode_parse_endpoint() now just ignores them
>   and returns success.
> - Fix a couple compile warnings and errors seen in i386 and sh archs.
> 
> 
> Steve Longerbeam (13):
>   media: v4l2-fwnode: ignore endpoints that have no remote port parent
>   media: v4l2: async: Allow searching for asd of any type
>   media: v4l2: async: Add v4l2_async_notifier_add_subdev
>   media: v4l2-fwnode: Switch to v4l2_async_notifier_add_subdev
>   media: v4l2-fwnode: Add a convenience function for registering subdevs
>     with notifiers
>   media: platform: video-mux: Register a subdev notifier
>   media: imx: csi: Register a subdev notifier
>   media: imx: mipi csi-2: Register a subdev notifier
>   media: staging/imx: of: Remove recursive graph walk
>   media: staging/imx: Loop through all registered subdevs for media
>     links
>   media: staging/imx: Rename root notifier
>   media: staging/imx: Switch to v4l2_async_notifier_add_subdev
>   media: staging/imx: TODO: Remove one assumption about OF graph parsing
> 
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c          |  10 +-
>  drivers/media/platform/video-mux.c                |  36 ++-
>  drivers/media/v4l2-core/v4l2-async.c              | 268 ++++++++++++++++------
>  drivers/media/v4l2-core/v4l2-fwnode.c             | 231 +++++++++++--------
>  drivers/staging/media/imx/TODO                    |  29 +--
>  drivers/staging/media/imx/imx-media-csi.c         |  11 +-
>  drivers/staging/media/imx/imx-media-dev.c         | 134 +++--------
>  drivers/staging/media/imx/imx-media-internal-sd.c |   5 +-
>  drivers/staging/media/imx/imx-media-of.c          | 106 +--------
>  drivers/staging/media/imx/imx-media.h             |   6 +-
>  drivers/staging/media/imx/imx6-mipi-csi2.c        |  31 ++-
>  include/media/v4l2-async.h                        |  24 +-
>  include/media/v4l2-fwnode.h                       |  65 +++++-
>  13 files changed, 534 insertions(+), 422 deletions(-)
> 

Re: [PATCH v3 00/13] media: imx: Switch to subdev notifiers

[Steve Longerbeam]

On 05/07/2018 07:20 AM, Hans Verkuil wrote:
> Steve, Sakari,
>
> Any progress on this? The imx7 patch series depends on this so that can't
> move forward until this is merged.

Hi Hans, I've been working on the updates and will submit v4 today.

Steve


>
>
> On 21/03/18 01:37, Steve Longerbeam wrote:
>> This patchset converts the imx-media driver and its dependent
>> subdevs to use subdev notifiers.
>>
>> There are a couple shortcomings in v4l2-core that prevented
>> subdev notifiers from working correctly in imx-media:
>>
>> 1. v4l2_async_notifier_fwnode_parse_endpoint() treats a fwnode
>>     endpoint that is not connected to a remote device as an error.
>>     But in the case of the video-mux subdev, this is not an error,
>>     it is OK if some of the mux inputs have no connection. Also,
>>     Documentation/devicetree/bindings/media/video-interfaces.txt explicitly
>>     states that the 'remote-endpoint' property is optional. So the first
>>     patch is a small modification to ignore empty endpoints in
>>     v4l2_async_notifier_fwnode_parse_endpoint() and allow
>>     __v4l2_async_notifier_parse_fwnode_endpoints() to continue to
>>     parse the remaining port endpoints of the device.
>>
>> 2. In the imx-media graph, multiple subdevs will encounter the same
>>     upstream subdev (such as the imx6-mipi-csi2 receiver), and so
>>     v4l2_async_notifier_parse_fwnode_endpoints() will add imx6-mipi-csi2
>>     multiple times. This is treated as an error by
>>     v4l2_async_notifier_register() later.
>>
>>     To get around this problem, add an v4l2_async_notifier_add_subdev()
>>     which first verifies the provided asd does not already exist in the
>>     given notifier asd list or in other registered notifiers. If the asd
>>     exists, the function returns -EEXIST and it's up to the caller to
>>     decide if that is an error (in imx-media case it is never an error).
>>
>>     Patches 2-4 deal with adding that support.
>>
>> 3. Patch 5 adds v4l2_async_register_fwnode_subdev(), which is a
>>     convenience function for parsing a subdev's fwnode port endpoints
>>     for connected remote subdevs, registering a subdev notifier, and
>>     then registering the sub-device itself.
>>
>> The remaining patches update the subdev drivers to register a
>> subdev notifier with endpoint parsing, and the changes to imx-media
>> to support that.
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> Acked-by: Philipp Zabel <p.zabel@pengutronix.de>
>>
>> History:
>> v3:
>> - code optimization in asd_equal(), and remove unneeded braces,
>>    suggested by Sakari Ailus.
>> - add a NULL asd pointer check to v4l2_async_notifier_asd_valid().
>> - fix an error-out path in v4l2_async_register_fwnode_subdev() that
>>    forgot to put device.
>>
>> v2:
>> - don't pass an empty endpoint to the parse_endpoint callback,
>>    v4l2_async_notifier_fwnode_parse_endpoint() now just ignores them
>>    and returns success.
>> - Fix a couple compile warnings and errors seen in i386 and sh archs.
>>
>>
>> Steve Longerbeam (13):
>>    media: v4l2-fwnode: ignore endpoints that have no remote port parent
>>    media: v4l2: async: Allow searching for asd of any type
>>    media: v4l2: async: Add v4l2_async_notifier_add_subdev
>>    media: v4l2-fwnode: Switch to v4l2_async_notifier_add_subdev
>>    media: v4l2-fwnode: Add a convenience function for registering subdevs
>>      with notifiers
>>    media: platform: video-mux: Register a subdev notifier
>>    media: imx: csi: Register a subdev notifier
>>    media: imx: mipi csi-2: Register a subdev notifier
>>    media: staging/imx: of: Remove recursive graph walk
>>    media: staging/imx: Loop through all registered subdevs for media
>>      links
>>    media: staging/imx: Rename root notifier
>>    media: staging/imx: Switch to v4l2_async_notifier_add_subdev
>>    media: staging/imx: TODO: Remove one assumption about OF graph parsing
>>
>>   drivers/media/pci/intel/ipu3/ipu3-cio2.c          |  10 +-
>>   drivers/media/platform/video-mux.c                |  36 ++-
>>   drivers/media/v4l2-core/v4l2-async.c              | 268 ++++++++++++++++------
>>   drivers/media/v4l2-core/v4l2-fwnode.c             | 231 +++++++++++--------
>>   drivers/staging/media/imx/TODO                    |  29 +--
>>   drivers/staging/media/imx/imx-media-csi.c         |  11 +-
>>   drivers/staging/media/imx/imx-media-dev.c         | 134 +++--------
>>   drivers/staging/media/imx/imx-media-internal-sd.c |   5 +-
>>   drivers/staging/media/imx/imx-media-of.c          | 106 +--------
>>   drivers/staging/media/imx/imx-media.h             |   6 +-
>>   drivers/staging/media/imx/imx6-mipi-csi2.c        |  31 ++-
>>   include/media/v4l2-async.h                        |  24 +-
>>   include/media/v4l2-fwnode.h                       |  65 +++++-
>>   13 files changed, 534 insertions(+), 422 deletions(-)
>>