Re: [PATCH v12 18/26] v4l: fwnode: Add a helper function for parsing generic references

On 09/13/17 12:07, Sakari Ailus wrote:

Hi Hans,

On Wed, Sep 13, 2017 at 11:28:44AM +0200, Hans Verkuil wrote:

quoted

On 09/13/17 11:24, Sakari Ailus wrote:

quoted

Hi Hans,

Thanks for the review!

On Wed, Sep 13, 2017 at 09:27:34AM +0200, Hans Verkuil wrote:

quoted

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

quoted

Add function v4l2_fwnode_reference_count() for counting external

???? There is no function v4l2_fwnode_reference_count()?!

It got removed during the revisions but the commit message was not changed
accordingly, I do that now.

quoted

quoted

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.

You forgot to remove this outdated paragraph.

Oops. Removed it now.

quoted

quoted

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

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 44ee35f6aad5..a32473f95be1 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c

@@ -498,6 +498,75 @@ int v4l2_async_notifier_parse_fwnode_endpoints_by_port(
 }
 EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints_by_port);
 
+/*
+ * v4l2_fwnode_reference_parse - parse references for async sub-devices
+ * @dev: the device node the properties of which are parsed for references
+ * @notifier: the async notifier where the async subdevs will be added
+ * @prop: the name of the property
+ *
+ * Return: 0 on success
+ *	   -ENOENT if no entries were found
+ *	   -ENOMEM if memory allocation failed
+ *	   -EINVAL if property parsing failed
+ */
+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;
+	int ret;
+
+	for (index = 0;
+	     !(ret = fwnode_property_get_reference_args(
+		       dev_fwnode(dev), prop, NULL, 0, index, &args));
+	     index++)
+		fwnode_handle_put(args.fwnode);
+
+	if (!index)
+		return -ENOENT;
+
+	/*
+	 * To-do: handle -ENODATA when "device property: Align return
+	 * codes of acpi_fwnode_get_reference_with_args" is merged.
+	 */
+	if (ret != -ENOENT && ret != -ENODATA)

So while that patch referenced in the To-do above is not merged yet,
what does fwnode_property_get_reference_args return? ENOENT or ENODATA?
Or ENOENT now and ENODATA later? Or vice versa?

I can't tell based on that information whether this code is correct or not.

The comment needs to explain this a bit better.

I'll add this:

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index a32473f95be1..74fcc3ba9ebd 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c

@@ -529,6 +529,9 @@ static int v4l2_fwnode_reference_parse(
 	/*
 	 * To-do: handle -ENODATA when "device property: Align return
 	 * codes of acpi_fwnode_get_reference_with_args" is merged.

So after this patch referred to in the To-do is applied it will only
return ENODATA?

In that case, change 'handle' to 'handle only'.

That depends a bit in which form the patch will be eventually accepted. The
underlying issue there is that different error codes are used to signal
conditions for out-of-bounds access and missing entry. After aligning them
the code here can be updated.

Ah. In that case I'd drop the 'To-do' sentence.

quoted

quoted

+	 * Right now, both -ENODATA and -ENOENT signal the end of
+	 * references where only a single error code should be used
+	 * for the purpose.

And add something like: "This might change in the future, in which
case this code should be updated."

quoted

quoted

 	 */
 	if (ret != -ENOENT && ret != -ENODATA)
 		return ret;

Regards,

	Hans