Re: [PATCH v12 15/26] v4l: async: Allow binding notifiers to sub-devices

On 09/13/17 10:29, Sakari Ailus wrote:

Hi Hans,

On Wed, Sep 13, 2017 at 09:17:08AM +0200, Hans Verkuil wrote:

quoted

On 09/12/2017 03:41 PM, Sakari Ailus wrote:

quoted

Registering a notifier has required the knowledge of struct v4l2_device
for the reason that sub-devices generally are registered to the
v4l2_device (as well as the media device, also available through
v4l2_device).

This information is not available for sub-device drivers at probe time.

What this patch does is that it allows registering notifiers without
having v4l2_device around. Instead the sub-device pointer is stored in the
notifier. Once the sub-device of the driver that registered the notifier
is registered, the notifier will gain the knowledge of the v4l2_device,
and the binding of async sub-devices from the sub-device driver's notifier
may proceed.

The root notifier's complete callback is only called when all sub-device
notifiers are completed.

Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Just two small comments (see below).

After changing those (the first is up to you) you can add my:

Acked-by: Hans Verkuil <redacted>

Thanks; please see my comments below.

...

quoted

quoted

+/* Return true if all sub-device notifiers are complete, false otherwise. */
+static bool v4l2_async_subdev_notifiers_complete(
+	struct v4l2_async_notifier *notifier)
+{
+	struct v4l2_subdev *sd;
+
+	if (!list_empty(&notifier->waiting))
+		return false;
+
+	list_for_each_entry(sd, &notifier->done, async_list) {
+		struct v4l2_async_notifier *subdev_notifier =
+			v4l2_async_find_subdev_notifier(sd);
+
+		if (!subdev_notifier)
+			continue;
+
+		if (!v4l2_async_subdev_notifiers_complete(subdev_notifier))

I think it is better to change this to:

		if (subdev_notifier &&
		    !v4l2_async_subdev_notifiers_complete(subdev_notifier))

and drop this if..continue above. That's a bit overkill in this simple case.

It's up to you, though.

Yes, makes sense.

...

quoted

quoted

+/* Try completing a notifier. */
+static int v4l2_async_notifier_try_complete(
+	struct v4l2_async_notifier *notifier)
+{
+	do {
+		int ret;
+
+		/* Any local async sub-devices left? */
+		if (!list_empty(&notifier->waiting))
+			return 0;
+
+		/*
+		 * Any sub-device notifiers waiting for async subdevs
+		 * to be bound?
+		 */
+		if (!v4l2_async_subdev_notifiers_complete(notifier))
+			return 0;
+
+		/* Proceed completing the notifier */
+		ret = v4l2_async_notifier_call_complete(notifier);
+		if (ret < 0)
+			return ret;
+
+		/*
+		 * Obtain notifier's parent. If there is one, repeat
+		 * the process, otherwise we're done here.
+		 */
+	} while ((notifier = notifier->parent));

I'd change this to:

		notifier = notifier->parent;
	} while (notifier);

Assignment in a condition is frowned upon, and there is no need to do that
here.

Wouldn't that equally apply to any statement with side effects? In other
words, what you're suggesting for patch 19? :-)

I don't like it there either, but rewriting that would make the code quite a
bit longer and you enter a gray area between 'no side-effects' and 'readability'.
In cases like that I tend to accept the preference of the author of the code.

In the case of this do...while the 'no side-effects' version is just as readable
if not more so than the 'side-effect' version.

At least, that's my reasoning.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html