On 09/19/17 10:45, Sakari Ailus wrote:

Hi Hans,

Thank you for the review.

On Tue, Sep 19, 2017 at 10:31:41AM +0200, Hans Verkuil wrote:

quoted

Hi Sakari,

I'm slowly starting to understand this. The example helped a lot. But I still have
some questions, see below.

On 09/15/2017 04:17 PM, Sakari Ailus wrote:

quoted

v4l2_fwnode_reference_parse_int_prop() will find an fwnode such that under
the device's own fwnode, it will follow child fwnodes with the given
property-value pair and return the resulting fwnode.

I think both the subject, commit log, function comment and function name should
reflect the fact that this function is for an ACPI reference.

It's only called for ACPI (from patch 19):

+		if (props[i].props && is_acpi_node(dev_fwnode(dev)))
+			ret = v4l2_fwnode_reference_parse_int_props(

So renaming it to v4l2_fwnode_acpi_reference_parse_int_props or something similar
would clarify this fact.

I don't think we'll see many like this one. I presume we won't use it on DT
albeit there are no direct references to ACPI in the code itself.

How about v4l2_fwnode_parse_acpi_reference (+ "s" for the one below)?

Sounds good.

quoted

quoted

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

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 65e84ea1cc35..968a345a288f 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c

@@ -567,6 +567,207 @@ static int v4l2_fwnode_reference_parse(
 	return ret;
 }
 
+/*
+ * v4l2_fwnode_reference_get_int_prop - parse a reference with integer
+ *					arguments
+ * @dev: struct device pointer
+ * @notifier: notifier for @dev
+ * @prop: the name of the property
+ * @index: the index of the reference to get
+ * @props: the array of integer property names
+ * @nprops: the number of integer property names in @nprops

You mean 'in @props'?

Yes, I'll fix that.

quoted

One thing that is not clear to me is when you would use an nprops value > 1.
What's the use-case for that? It only makes sense (I think) if you would have
property names that are all aliases of one another.

There may be several flash LEDs related to a sensor. That's the use case,
for instance.

I think it would be helpful if the example shows two LEDs related to a
sensor. Part of the problem I have in understanding this code is that I
have zero experience with ACPI (and that is probably true for most other
developers), so I don't know how this is encoded. With a good example it
is much easier to understand.

quoted

quoted

+ *
+ * Find fwnodes referred to by a property @prop, then under that
+ * iteratively, @nprops times, follow each child node which has a
+ * property in @props array at a given child index the value of which
+ * matches the integer argument at an index.
+ *
+ * For example, if this function was called with arguments and values
+ * @dev corresponding to device "SEN", @prop == "flash-leds", @index
+ * == 1, @props == { "led" }, @nprops == 1, with the ASL snippet below
+ * it would return the node marked with THISONE. The @dev argument in
+ * the ASL below.

That last sentence about the @dev seems incomplete. I'm not sure what is
meant by it.

I think it was meant to convey some information but it got added to the
previous sentence. I'll remove it.

quoted

quoted

+ *
+ *	Device (LED)
+ *	{
+ *		Name (_DSD, Package () {
+ *			ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
+ *			Package () {
+ *				Package () { "led0", "LED0" },
+ *				Package () { "led1", "LED1" },
+ *			}
+ *		})
+ *		Name (LED0, Package () {
+ *			ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ *			Package () {
+ *				Package () { "led", 0 },
+ *			}
+ *		})
+ *		Name (LED1, Package () {
+ *			// THISONE
+ *			ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ *			Package () {
+ *				Package () { "led", 1 },
+ *			}
+ *		})
+ *	}
+ *
+ *	Device (SEN)
+ *	{
+ *		Name (_DSD, Package () {
+ *			ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ *			Package () {
+ *				Package () {
+ *					"flash-leds",
+ *					Package () { ^LED, 0, ^LED, 1 },
+ *				}
+ *			}
+ *		})
+ *	}
+ *
+ * where
+ *
+ *	LED	LED driver device
+ *	LED0	First LED
+ *	LED1	Second LED
+ *	SEN	Camera sensor device (or another device the LED is
+ *		related to)
+ *
+ * Return: 0 on success
+ *	   -ENOENT if no entries (or the property itself) were found
+ *	   -EINVAL if property parsing otherwise failed
+ *	   -ENOMEM if memory allocation failed
+ */
+static struct fwnode_handle *v4l2_fwnode_reference_get_int_prop(
+	struct fwnode_handle *fwnode, const char *prop, unsigned int index,
+	const char **props, unsigned int nprops)
+{
+	struct fwnode_reference_args fwnode_args;
+	unsigned int *args = fwnode_args.args;
+	struct fwnode_handle *child;
+	int ret;
+
+	/*
+	 * Obtain remote fwnode as well as the integer arguments.
+	 *
+	 * Note that right now both -ENODATA and -ENOENT may signal
+	 * out-of-bounds access. Return -ENOENT in that case.
+	 */
+	ret = fwnode_property_get_reference_args(fwnode, prop, NULL, nprops,
+						 index, &fwnode_args);
+	if (ret)
+		return ERR_PTR(ret == -ENODATA ? -ENOENT : ret);
+
+	/*
+	 * Find a node in the tree under the referred fwnode corresponding the
+	 * integer arguments.
+	 */
+	fwnode = fwnode_args.fwnode;

So given the example above, fwnode would point to the LED device?

If correct, then mention that in the comment.

It could be a LED driver device, but it could be something else as well.
Like a lens VCM, depending on the property being parsed. That's why I
didn't put it in the comments. But this is a device node, not a
hierarchical data extension node, for instance. That's what I think I
should add.

I think that will help.

quoted

quoted

+	while (nprops--) {
+		u32 val;
+
+		/* Loop over all child nodes under fwnode. */

And here you check if the LED device has child nodes that have a *props
property with a value matching the index.

So given the example above it is looking for a child with property "led"
and value 1.

It's useful if that is mentioned in the comment as well.

But should I? This isn't specific to LEDs.

Ignore this comment for now. I'll take another look when I see v14.

Regards,

	Hans