Hi Hans,

Thanks for the review!

On Mon, Sep 11, 2017 at 11:14:03AM +0200, Hans Verkuil wrote:

On 09/11/2017 10:00 AM, Sakari Ailus wrote:

quoted

Add function v4l2_fwnode_reference_count() for counting external
references and v4l2_fwnode_reference_parse() for parsing them as async
sub-devices.

This can be done on e.g. flash or lens async sub-devices that are not part
of but are associated with a sensor.

struct v4l2_async_notifier.max_subdevs field is added to contain the
maximum number of sub-devices in a notifier to reflect the memory
allocated for the subdevs array.

This paragraph appears to be out-of-date.

Will remove.

quoted

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-fwnode.c | 47 +++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index d978f2d714ca..4821c4989119 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c

@@ -449,6 +449,53 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
 }
 EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
 
+static int v4l2_fwnode_reference_parse(
+	struct device *dev, struct v4l2_async_notifier *notifier,
+	const char *prop)
+{
+	struct fwnode_reference_args args;
+	unsigned int index = 0;
+	int ret;
+
+	for (; !fwnode_property_get_reference_args(
+		     dev_fwnode(dev), prop, NULL, 0, index, &args); index++)
+		fwnode_handle_put(args.fwnode);
+

If nothing is found (i.e. index == 0), shouldn't you just return here?

You could. yes. It would actually simplify the code, I could just return
-ENOENT here.

quoted

+	ret = v4l2_async_notifier_realloc(notifier,
+					  notifier->num_subdevs + index);
+	if (ret)
+		return -ENOMEM;
+
+	for (ret = -ENOENT, index = 0;

There is no reason for the 'ret = -ENOENT' to be in the for(), just set it before
the 'for' statement.

quoted

+	     !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.fwnode = args.fwnode;
+		asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
+		notifier->num_subdevs++;
+	}

If the loop doesn't find anything, then it still returns 0, not -ENOENT.
So why set ret to ENOENT? Something weird going on here.

:-)

I think -ENOENT would make sense indeed if there are no entries found.

I think you should also add a comment explaining this function.

Yes. I had that in the header when it was exported, I'll add the same
comments here.

quoted

+
+	return 0;
+
+error:
+	fwnode_handle_put(args.fwnode);
+	return ret;
+}
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Sakari Ailus [off-list ref]");
 MODULE_AUTHOR("Sylwester Nawrocki [off-list ref]");

-- 
Regards,

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