[PATCH v5 0/4] v4l2-async: add subnotifier registration for subdevices

[PATCH v5 0/4] v4l2-async: add subnotifier registration for subdevices

[Niklas Söderlund]

Hi,

I know Sakari have posted a series '[RFC 00/19] Async sub-notifiers and 
how to use them' which address similar problems as this series. This is 
not intended to compete whit his work and Sakari includes one of my v3 
patches in his series. Never the less I post this updated series since 
it fixes some issues in my v3 implementation and contains some other 
fixes for the v4l2-async framework which are not addressed in Sakaris 
patches. I think the correct solution to the problems we both try to fix 
is a mix of our two series, would you agree Sakari?

This series enables incremental async find and bind of subdevices,
please se patch 4/4 for a more detailed description. This is a rewrite 
of the feature since v3, see changelog in this cover letter for the 
differences to v3. The two primary reasons for a new implementation 
where:

1. Hans expressed an interest having the async complete() callbacks to
   happen only once all notifiers in the pipeline where complete. To do
   this a stronger connection between the notifiers where needed, hence
   the subnotifier is now embedded in struct v4l2_subdev.

   Whit this change it is possible to check all notifiers in a pipeline
   is complete before calling any of them.

2. There where concerns that the v3 solution was a bit to complex and
   hard to refactor in the future if other issues in the v4l2-async
   framework where to be addressed. By hiding the notifier in the struct
   v4l2_subdev and adding a new function to set that structure the
   interface towards drivers are minimized while everything else happens
   in the v4l2-async framework. This leaves the interface in a good
   position for possible changes in v4l2-async.

This is tested on Renesas H3 and M3-W together with the Renesas CSI-2
and VIN Gen3 driver (posted separately). It is based on top of the media-tree.

* Changes since v4
- Add patch which aborts v4l2_async_notifier_unregister() if the memory 
  allocation for the device cache fails instead of trying to do as much 
  as possible but still leave the system in a semi good state.
- Rename v4l2_async_subdev_register_notifier() to 
  v4l2_async_subdev_notifier_register().
- Add more error checks in v4l2_async_subdev_notifier_register().
- Fix code style issues pointed out by Hans.
- Fix spelling mistakes pointed out by Hans.

* Changes since v3
- Almost a complete rewrite, so drop all Ack-ed by tags.
- Do not add new functions to register/unregister subnotifiers from 
  callbacks. Instead have have the subdevice drivers populate the
  subnotifer list at probe time and have the v4l2-async framework handle
  the (un)registration of the notifiers.
- Synchronize the call off the complete() callbacks. They will now all 
  happens once all notifiers in a pipeline are all complete and from the
  edge towards the root device.
- Add a new function v4l2_async_subdev_register_notifier() to hide the 
  setup of the subnotifier internals to ease improvements later.

* Changes since v2
- Fixed lots of spelling mistakes, thanks Hans!
- Used a goto instead if state variable when restarting iteration over
  subdev list as suggested by Sakari. Thank you it's much easier read
  now.
- Added Acked-by from Sakari and Hans, thanks!
- Rebased to latest media-tree.

* Changes since v1:
- Added a pre-patch which adds an error check which was previously in
  the new incremental async code but is more useful on its own.
- Added documentation to Documentation/media/kapi/v4l2-subdev.rst.
- Fixed data type of bool variable.
- Added call to lockdep_assert_held(), thanks Sakari.
- Fixed commit messages typo, thanks Sakari.

Niklas Söderlund (4):
  v4l: async: fix unbind error in v4l2_async_notifier_unregister()
  v4l: async: abort if memory allocation fails when unregistering
    notifiers
  v4l: async: do not hold list_lock when re-probing devices
  v4l: async: add subnotifier to subdevices

 Documentation/media/kapi/v4l2-subdev.rst |  12 +++
 drivers/media/v4l2-core/v4l2-async.c     | 170 ++++++++++++++++++++++++-------
 include/media/v4l2-async.h               |  25 +++++
 include/media/v4l2-subdev.h              |   5 +
 4 files changed, 178 insertions(+), 34 deletions(-)

-- 
2.13.1

[PATCH v5 1/4] v4l: async: fix unbind error in v4l2_async_notifier_unregister()

[Niklas Söderlund]

The call to v4l2_async_cleanup() will set sd->asd to NULL so passing it
to notifier->unbind() have no effect and leaves the notifier confused.
Call the unbind() callback prior to cleaning up the subdevice to avoid
this.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/v4l2-core/v4l2-async.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 851f128eba2219ad..0acf288d7227ba97 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -226,14 +226,14 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 
 		d = get_device(sd->dev);
 
+		if (notifier->unbind)
+			notifier->unbind(notifier, sd, sd->asd);
+
 		v4l2_async_cleanup(sd);
 
 		/* If we handled USB devices, we'd have to lock the parent too */
 		device_release_driver(d);
 
-		if (notifier->unbind)
-			notifier->unbind(notifier, sd, sd->asd);
-
 		/*
 		 * Store device at the device cache, in order to call
 		 * put_device() on the final step
-- 
2.13.1

[PATCH v5 2/4] v4l: async: abort if memory allocation fails when unregistering notifiers

[Niklas Söderlund]

Instead of trying to cope with the failed memory allocation and still
leaving the kernel in a semi-broken state (the subdevices will be
released but never re-probed) simply abort. The kernel have already
printed a warning about allocation failure but keep the error printout
to ease pinpointing the problem if it happens.

By doing this we can increase the readability of this complex function
which puts it in a better state to separate the v4l2 housekeeping tasks
from the re-probing of devices. It also serves to prepare for adding
subnotifers.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/v4l2-core/v4l2-async.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 0acf288d7227ba97..67852f0f2d3000c9 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -215,6 +215,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 	if (!dev) {
 		dev_err(notifier->v4l2_dev->dev,
 			"Failed to allocate device cache!\n");
+		return;
 	}
 
 	mutex_lock(&list_lock);
@@ -234,23 +235,13 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 		/* If we handled USB devices, we'd have to lock the parent too */
 		device_release_driver(d);
 
-		/*
-		 * Store device at the device cache, in order to call
-		 * put_device() on the final step
-		 */
-		if (dev)
-			dev[i++] = d;
-		else
-			put_device(d);
+		dev[i++] = d;
 	}
 
 	mutex_unlock(&list_lock);
 
 	/*
 	 * Call device_attach() to reprobe devices
-	 *
-	 * NOTE: If dev allocation fails, i is 0, and the whole loop won't be
-	 * executed.
 	 */
 	while (i--) {
 		struct device *d = dev[i];
-- 
2.13.1

[PATCH v5 3/4] v4l: async: do not hold list_lock when re-probing devices

[Niklas Söderlund]

There is no good reason to hold the list_lock when re-probing the
devices and it prevents a clean implementation of subdevice notifiers.
Move the actual release of the devices outside of the loop which
requires the lock to be held.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/v4l2-core/v4l2-async.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 67852f0f2d3000c9..d91ff0a33fd3eaff 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -206,7 +206,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 	unsigned int notif_n_subdev = notifier->num_subdevs;
 	unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS);
 	struct device **dev;
-	int i = 0;
+	int i, count = 0;
 
 	if (!notifier->v4l2_dev)
 		return;
@@ -223,27 +223,26 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 	list_del(&notifier->list);
 
 	list_for_each_entry_safe(sd, tmp, &notifier->done, async_list) {
-		struct device *d;
-
-		d = get_device(sd->dev);
+		dev[count] = get_device(sd->dev);
+		count++;
 
 		if (notifier->unbind)
 			notifier->unbind(notifier, sd, sd->asd);
 
 		v4l2_async_cleanup(sd);
-
-		/* If we handled USB devices, we'd have to lock the parent too */
-		device_release_driver(d);
-
-		dev[i++] = d;
 	}
 
 	mutex_unlock(&list_lock);
 
+	for (i = 0; i < count; i++) {
+		/* If we handled USB devices, we'd have to lock the parent too */
+		device_release_driver(dev[i]);
+	}
+
 	/*
 	 * Call device_attach() to reprobe devices
 	 */
-	while (i--) {
+	for (i = 0; i < count; i++) {
 		struct device *d = dev[i];
 
 		if (d && device_attach(d) < 0) {
-- 
2.13.1

[PATCH v5 4/4] v4l: async: add subnotifier to subdevices

[Niklas Söderlund]

Add a subdevice specific notifier which can be used by a subdevice
driver to complement the master device notifier to extend the subdevice
discovery.

The master device registers the subdevices closest to itself in its
notifier while the subdevice(s) register notifiers for their closest
neighboring devices. Subdevice drivers configure a notifier at probe
time which is registered by the v4l2-async framework once the subdevice
itself is registered, since it's only at this point the v4l2_dev is
available to the subnotifier.

Using this incremental approach two problems can be solved:

1. The master device no longer has to care how many devices exist in
   the pipeline. It only needs to care about its closest subdevice and
   arbitrary long pipelines can be created without having to adapt the
   master device for each case.

2. Subdevices which are represented as a single DT node but register
   more than one subdevice can use this to improve the pipeline
   discovery, since the subdevice driver is the only one who knows which
   of its subdevices is linked with which subdevice of a neighboring DT
   node.

To allow subdevices to provide their own list of subdevices to the
v4l2-async framework v4l2_async_subdev_notifier_register() is added.
This new function must be called before the subdevice itself is
registered with the v4l2-async framework using
v4l2_async_register_subdev().

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 Documentation/media/kapi/v4l2-subdev.rst |  12 +++
 drivers/media/v4l2-core/v4l2-async.c     | 142 +++++++++++++++++++++++++++----
 include/media/v4l2-async.h               |  25 ++++++
 include/media/v4l2-subdev.h              |   5 ++
 4 files changed, 169 insertions(+), 15 deletions(-)

diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst
index e1f0b726e438f963..ec9e7b7fb78f226d 100644
--- a/Documentation/media/kapi/v4l2-subdev.rst
+++ b/Documentation/media/kapi/v4l2-subdev.rst
@@ -262,6 +262,18 @@ is called. After all subdevices have been located the .complete() callback is
 called. When a subdevice is removed from the system the .unbind() method is
 called. All three callbacks are optional.
 
+Subdevice drivers might in turn register subnotifier objects with an
+array of other subdevice descriptors that the subdevice needs for its
+own operation. Subnotifiers are an extension of the bridge drivers
+notifier to allow for a incremental registering and matching of
+subdevices. This is useful when a driver only has information about
+which subdevice is closest to itself and would require knowledge from the
+driver of that subdevice to know which other subdevice(s) lie beyond.
+By registering subnotifiers drivers can incrementally move the subdevice
+matching down the chain of drivers. This is performed using the
+:c:func:`v4l2_async_subdev_notifier_register` call which must be performed
+before registering the subdevice using :c:func:`v4l2_async_register_subdev`.
+
 V4L2 sub-device userspace API
 -----------------------------
 
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index d91ff0a33fd3eaff..0cafb13025b5cbb6 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -100,6 +100,60 @@ static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *
 	return NULL;
 }
 
+static int v4l2_async_notifier_complete(struct v4l2_async_notifier *notifier)
+{
+	struct v4l2_subdev *sd, *tmp;
+
+	if (!notifier->num_subdevs)
+		return 0;
+
+	list_for_each_entry_safe(sd, tmp, &notifier->done, async_list)
+		v4l2_async_notifier_complete(&sd->subnotifier);
+
+	if (notifier->complete)
+		return notifier->complete(notifier);
+
+	return 0;
+}
+
+static bool
+v4l2_async_is_notifier_complete(struct v4l2_async_notifier *notifier)
+{
+	struct v4l2_subdev *sd, *tmp;
+
+	if (!list_empty(&notifier->waiting))
+		return false;
+
+	list_for_each_entry_safe(sd, tmp, &notifier->done, async_list) {
+		/* Don't consider empty subnotifiers */
+		if (!sd->subnotifier.num_subdevs)
+			continue;
+
+		if (!v4l2_async_is_notifier_complete(&sd->subnotifier))
+			return false;
+	}
+
+	return true;
+}
+
+static int
+v4l2_async_try_complete_notifier(struct v4l2_async_notifier *notifier)
+{
+	struct v4l2_async_notifier *root = notifier;
+
+	while (root->subnotifier) {
+		root = subnotifier_to_v4l2_subdev(root)->notifier;
+		/* No root notifier can be found at this time */
+		if (!root)
+			return 0;
+	}
+
+	if (v4l2_async_is_notifier_complete(root))
+		return v4l2_async_notifier_complete(root);
+
+	return 0;
+}
+
 static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
 				  struct v4l2_subdev *sd,
 				  struct v4l2_async_subdev *asd)
@@ -119,6 +173,17 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
 		return ret;
 	}
 
+	/* Register the subnotifier if it's not empty */
+	if (sd->subnotifier.num_subdevs) {
+		ret = v4l2_async_notifier_register(sd->v4l2_dev,
+						   &sd->subnotifier);
+		if (ret) {
+			if (notifier->unbind)
+				notifier->unbind(notifier, sd, asd);
+			v4l2_device_unregister_subdev(sd);
+			return ret;
+		}
+	}
 	/* Remove from the waiting list */
 	list_del(&asd->list);
 	sd->asd = asd;
@@ -127,10 +192,7 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
 	/* Move from the global subdevice list to notifier's done */
 	list_move(&sd->async_list, &notifier->done);
 
-	if (list_empty(&notifier->waiting) && notifier->complete)
-		return notifier->complete(notifier);
-
-	return 0;
+	return v4l2_async_try_complete_notifier(notifier);
 }
 
 static void v4l2_async_cleanup(struct v4l2_subdev *sd)
@@ -140,6 +202,7 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd)
 	list_del_init(&sd->async_list);
 	sd->asd = NULL;
 	sd->dev = NULL;
+	sd->notifier = NULL;
 }
 
 int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
@@ -175,8 +238,17 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
 		list_add_tail(&asd->list, &notifier->waiting);
 	}
 
-	mutex_lock(&list_lock);
+	if (notifier->subnotifier)
+		lockdep_assert_held(&list_lock);
+	else
+		mutex_lock(&list_lock);
 
+	/*
+	 * This function can be called recursively so the list
+	 * might be modified in a recursive call. Start from the
+	 * top of the list each iteration.
+	 */
+again:
 	list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
 		int ret;
 
@@ -186,15 +258,18 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
 
 		ret = v4l2_async_test_notify(notifier, sd, asd);
 		if (ret < 0) {
-			mutex_unlock(&list_lock);
+			if (!notifier->subnotifier)
+				mutex_unlock(&list_lock);
 			return ret;
 		}
+		goto again;
 	}
 
 	/* Keep also completed notifiers on the list */
 	list_add(&notifier->list, &notifier_list);
 
-	mutex_unlock(&list_lock);
+	if (!notifier->subnotifier)
+		mutex_unlock(&list_lock);
 
 	return 0;
 }
@@ -205,14 +280,17 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 	struct v4l2_subdev *sd, *tmp;
 	unsigned int notif_n_subdev = notifier->num_subdevs;
 	unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS);
-	struct device **dev;
 	int i, count = 0;
+	struct {
+		struct device *dev;
+		struct v4l2_subdev *sd;
+	} *cache;
 
 	if (!notifier->v4l2_dev)
 		return;
 
-	dev = kvmalloc_array(n_subdev, sizeof(*dev), GFP_KERNEL);
-	if (!dev) {
+	cache = kvmalloc_array(n_subdev, sizeof(*cache), GFP_KERNEL);
+	if (!cache) {
 		dev_err(notifier->v4l2_dev->dev,
 			"Failed to allocate device cache!\n");
 		return;
@@ -223,7 +301,8 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 	list_del(&notifier->list);
 
 	list_for_each_entry_safe(sd, tmp, &notifier->done, async_list) {
-		dev[count] = get_device(sd->dev);
+		cache[count].dev = get_device(sd->dev);
+		cache[count].sd = sd;
 		count++;
 
 		if (notifier->unbind)
@@ -235,20 +314,24 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 	mutex_unlock(&list_lock);
 
 	for (i = 0; i < count; i++) {
+		sd = cache[i].sd;
+		/* If the subdev have a notifier unregister it */
+		if (sd->subnotifier.num_subdevs)
+			v4l2_async_notifier_unregister(&sd->subnotifier);
+
 		/* If we handled USB devices, we'd have to lock the parent too */
-		device_release_driver(dev[i]);
+		device_release_driver(cache[i].dev);
 	}
 
 	/*
 	 * Call device_attach() to reprobe devices
 	 */
 	for (i = 0; i < count; i++) {
-		struct device *d = dev[i];
+		struct device *d = cache[i].dev;
 
 		if (d && device_attach(d) < 0) {
 			const char *name = "(none)";
 			int lock = device_trylock(d);
-
 			if (lock && d->driver)
 				name = d->driver->name;
 			dev_err(d, "Failed to re-probe to %s\n", name);
@@ -257,7 +340,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 		}
 		put_device(d);
 	}
-	kvfree(dev);
+	kvfree(cache);
 
 	notifier->v4l2_dev = NULL;
 
@@ -312,6 +395,9 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
 		return;
 	}
 
+	if (sd->subnotifier.num_subdevs)
+		v4l2_async_notifier_unregister(&sd->subnotifier);
+
 	mutex_lock(&list_lock);
 
 	list_add(&sd->asd->list, &notifier->waiting);
@@ -324,3 +410,29 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
 	mutex_unlock(&list_lock);
 }
 EXPORT_SYMBOL(v4l2_async_unregister_subdev);
+
+int v4l2_async_subdev_notifier_register(
+		struct v4l2_subdev *sd,
+		unsigned int num_subdevs,
+		struct v4l2_async_subdev **subdevs,
+		int (*bound)(struct v4l2_async_notifier *notifier,
+			     struct v4l2_subdev *subdev,
+			     struct v4l2_async_subdev *asd),
+		int (*complete)(struct v4l2_async_notifier *notifier),
+		void (*unbind)(struct v4l2_async_notifier *notifier,
+			       struct v4l2_subdev *subdev,
+			       struct v4l2_async_subdev *asd))
+{
+	if (!sd || !num_subdevs || !subdevs)
+		return -EINVAL;
+
+	sd->subnotifier.subnotifier = true;
+	sd->subnotifier.num_subdevs = num_subdevs;
+	sd->subnotifier.subdevs = subdevs;
+	sd->subnotifier.bound = bound;
+	sd->subnotifier.complete = complete;
+	sd->subnotifier.unbind = unbind;
+
+	return 0;
+}
+EXPORT_SYMBOL(v4l2_async_subdev_notifier_register);
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index c69d8c8a66d0093a..91f02d2b03b88135 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -78,6 +78,7 @@ struct v4l2_async_subdev {
 /**
  * struct v4l2_async_notifier - v4l2_device notifier data
  *
+ * @subnotifier: flag if this notifier is part of a v4l2_subdev
  * @num_subdevs: number of subdevices
  * @subdevs:	array of pointers to subdevice descriptors
  * @v4l2_dev:	pointer to struct v4l2_device
@@ -89,6 +90,7 @@ struct v4l2_async_subdev {
  * @unbind:	a subdevice is leaving
  */
 struct v4l2_async_notifier {
+	bool subnotifier;
 	unsigned int num_subdevs;
 	struct v4l2_async_subdev **subdevs;
 	struct v4l2_device *v4l2_dev;
@@ -135,4 +137,27 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd);
  * @sd: pointer to &struct v4l2_subdev
  */
 void v4l2_async_unregister_subdev(struct v4l2_subdev *sd);
+
+/**
+ * v4l2_async_subdev_notifier_register - registers a subdevice asynchronous
+ *	subnotifier
+ *
+ * @sd: pointer to &struct v4l2_subdev
+ * @num_subdevs: number of subdevices
+ * @subdevs: array of pointers to subdevice descriptors
+ * @bound: a subdevice driver has successfully probed one of subdevices
+ * @complete: all subdevices have been probed successfully
+ * @unbind: a subdevice is leaving
+ */
+int v4l2_async_subdev_notifier_register(
+		struct v4l2_subdev *sd,
+		unsigned int num_subdevs,
+		struct v4l2_async_subdev **subdevs,
+		int (*bound)(struct v4l2_async_notifier *notifier,
+			     struct v4l2_subdev *subdev,
+			     struct v4l2_async_subdev *asd),
+		int (*complete)(struct v4l2_async_notifier *notifier),
+		void (*unbind)(struct v4l2_async_notifier *notifier,
+			       struct v4l2_subdev *subdev,
+			       struct v4l2_async_subdev *asd));
 #endif
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 0f92ebd2d7101acf..13a04af16a627394 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -793,6 +793,7 @@ struct v4l2_subdev_platform_data {
  *	list.
  * @asd: Pointer to respective &struct v4l2_async_subdev.
  * @notifier: Pointer to the managing notifier.
+ * @subnotifier: Notifier for devices the subdevice depends on
  * @pdata: common part of subdevice platform data
  *
  * Each instance of a subdev driver should create this struct, either
@@ -823,6 +824,7 @@ struct v4l2_subdev {
 	struct list_head async_list;
 	struct v4l2_async_subdev *asd;
 	struct v4l2_async_notifier *notifier;
+	struct v4l2_async_notifier subnotifier;
 	struct v4l2_subdev_platform_data *pdata;
 };
 
@@ -838,6 +840,9 @@ struct v4l2_subdev {
 #define vdev_to_v4l2_subdev(vdev) \
 	((struct v4l2_subdev *)video_get_drvdata(vdev))
 
+#define subnotifier_to_v4l2_subdev(sub) \
+	container_of(sub, struct v4l2_subdev, subnotifier)
+
 /**
  * struct v4l2_subdev_fh - Used for storing subdev information per file handle
  *
-- 
2.13.1

Re: [PATCH v5 0/4] v4l2-async: add subnotifier registration for subdevices

[Hans Verkuil]

On 19/07/17 12:49, Niklas Söderlund wrote:
> * Changes since v4
> - Add patch which aborts v4l2_async_notifier_unregister() if the memory 
>   allocation for the device cache fails instead of trying to do as much 
>   as possible but still leave the system in a semi good state.

Since you are working with this code I would very much appreciate it if you
can make another patch that adds comments to this reprobing stuff, including
why device_reprobe() cannot be used here.

That will help in the future.

Regards,

	Hans

Re: [PATCH v5 0/4] v4l2-async: add subnotifier registration for subdevices

[Niklas Söderlund]

Hi Hans,

Thanks for your feedback.

On 2017-07-19 13:02:14 +0200, Hans Verkuil wrote:
> On 19/07/17 12:49, Niklas Söderlund wrote:
> > * Changes since v4
> > - Add patch which aborts v4l2_async_notifier_unregister() if the memory 
> >   allocation for the device cache fails instead of trying to do as much 
> >   as possible but still leave the system in a semi good state.
> 
> Since you are working with this code I would very much appreciate it if you
> can make another patch that adds comments to this reprobing stuff, including
> why device_reprobe() cannot be used here.

I could do that.

I think it makes most sens to break out patch 1-3 plus this new patch to 
a new series 'v4l2-async: clean up v4l2_async_notifier_unregister()' or 
something similar and post separate from the subnotifer work as me and 
Sakari are still bashing out a common solution to that.

Do this make sens to you or do you wish for me to keep these patches 
together in this series?

> 
> That will help in the future.
> 
> Regards,
> 
> 	Hans

-- 
Regards,
Niklas Söderlund

Re: [PATCH v5 0/4] v4l2-async: add subnotifier registration for subdevices

[Niklas Söderlund]

Hi Hans,

Thanks for your feedback.

On 2017-07-19 13:02:14 +0200, Hans Verkuil wrote:
> On 19/07/17 12:49, Niklas S�derlund wrote:
> > * Changes since v4
> > - Add patch which aborts v4l2_async_notifier_unregister() if the memory 
> >   allocation for the device cache fails instead of trying to do as much 
> >   as possible but still leave the system in a semi good state.
> 
> Since you are working with this code I would very much appreciate it if you
> can make another patch that adds comments to this reprobing stuff, including
> why device_reprobe() cannot be used here.

I could do that.

I think it makes most sens to break out patch 1-3 plus this new patch to 
a new series 'v4l2-async: clean up v4l2_async_notifier_unregister()' or 
something similar and post separate from the subnotifer work as me and 
Sakari are still bashing out a common solution to that.

Do this make sens to you or do you wish for me to keep these patches 
together in this series?

> 
> That will help in the future.
> 
> Regards,
> 
> 	Hans

-- 
Regards,
Niklas S�derlund

Re: [PATCH v5 0/4] v4l2-async: add subnotifier registration for subdevices

[Hans Verkuil]

On 19/07/17 13:54, Niklas Söderlund wrote:
> Hi Hans,
> 
> Thanks for your feedback.
> 
> On 2017-07-19 13:02:14 +0200, Hans Verkuil wrote:
>> On 19/07/17 12:49, Niklas Söderlund wrote:
>>> * Changes since v4
>>> - Add patch which aborts v4l2_async_notifier_unregister() if the memory 
>>>   allocation for the device cache fails instead of trying to do as much 
>>>   as possible but still leave the system in a semi good state.
>>
>> Since you are working with this code I would very much appreciate it if you
>> can make another patch that adds comments to this reprobing stuff, including
>> why device_reprobe() cannot be used here.
> 
> I could do that.
> 
> I think it makes most sens to break out patch 1-3 plus this new patch to 
> a new series 'v4l2-async: clean up v4l2_async_notifier_unregister()' or 
> something similar and post separate from the subnotifer work as me and 
> Sakari are still bashing out a common solution to that.
> 
> Do this make sens to you or do you wish for me to keep these patches 
> together in this series?

That makes sense. It's good to have those fixes in.

Regards,

	Hans

Re: [PATCH v5 0/4] v4l2-async: add subnotifier registration for subdevices

[Hans Verkuil]

On 19/07/17 13:54, Niklas S�derlund wrote:
> Hi Hans,
> 
> Thanks for your feedback.
> 
> On 2017-07-19 13:02:14 +0200, Hans Verkuil wrote:
>> On 19/07/17 12:49, Niklas S�derlund wrote:
>>> * Changes since v4
>>> - Add patch which aborts v4l2_async_notifier_unregister() if the memory 
>>>   allocation for the device cache fails instead of trying to do as much 
>>>   as possible but still leave the system in a semi good state.
>>
>> Since you are working with this code I would very much appreciate it if you
>> can make another patch that adds comments to this reprobing stuff, including
>> why device_reprobe() cannot be used here.
> 
> I could do that.
> 
> I think it makes most sens to break out patch 1-3 plus this new patch to 
> a new series 'v4l2-async: clean up v4l2_async_notifier_unregister()' or 
> something similar and post separate from the subnotifer work as me and 
> Sakari are still bashing out a common solution to that.
> 
> Do this make sens to you or do you wish for me to keep these patches 
> together in this series?

That makes sense. It's good to have those fixes in.

Regards,

	Hans

Re: [PATCH v5 0/4] v4l2-async: add subnotifier registration for subdevices

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2322 bytes --]

Hi Niklas,

On Wed, Jul 19, 2017 at 12:49:42PM +0200, Niklas Söderlund wrote:
> Hi,
> 
> I know Sakari have posted a series '[RFC 00/19] Async sub-notifiers and 
> how to use them' which address similar problems as this series. This is 
> not intended to compete whit his work and Sakari includes one of my v3 
> patches in his series. Never the less I post this updated series since 
> it fixes some issues in my v3 implementation and contains some other 
> fixes for the v4l2-async framework which are not addressed in Sakaris 
> patches. I think the correct solution to the problems we both try to fix 
> is a mix of our two series, would you agree Sakari?
> 
> This series enables incremental async find and bind of subdevices,
> please se patch 4/4 for a more detailed description. This is a rewrite 
> of the feature since v3, see changelog in this cover letter for the 
> differences to v3. The two primary reasons for a new implementation 
> where:
> 
> 1. Hans expressed an interest having the async complete() callbacks to
>    happen only once all notifiers in the pipeline where complete. To do
>    this a stronger connection between the notifiers where needed, hence
>    the subnotifier is now embedded in struct v4l2_subdev.
> 
>    Whit this change it is possible to check all notifiers in a pipeline
>    is complete before calling any of them.
> 
> 2. There where concerns that the v3 solution was a bit to complex and
>    hard to refactor in the future if other issues in the v4l2-async
>    framework where to be addressed. By hiding the notifier in the struct
>    v4l2_subdev and adding a new function to set that structure the
>    interface towards drivers are minimized while everything else happens
>    in the v4l2-async framework. This leaves the interface in a good
>    position for possible changes in v4l2-async.
> 
> This is tested on Renesas H3 and M3-W together with the Renesas CSI-2
> and VIN Gen3 driver (posted separately). It is based on top of the media-tree.

Thanks for the new iteration, you can add my
Tested-by: Maxime Ripard <maxime.ripard@free-electrons.com>

On the cadence CSI2 RX driver I sent earlier.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]