Re: [PATCH v10 18/24] v4l: fwnode: Add a helper function to obtain device / interger references

Typo in subject: interger -> integer

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

v4l2_fwnode_reference_parse_int_prop() will find an fwnode such that under
the device's own fwnode, 

Sorry, you lost me here. Which device are we talking about?

it will follow child fwnodes with the given
property -- value pair and return the resulting fwnode.

property-value pair (easier readable that way).

You only describe v4l2_fwnode_reference_parse_int_prop(), not
v4l2_fwnode_reference_parse_int_props().

Need comments describing what this does.

+{
+	struct fwnode_reference_args fwnode_args;
+	unsigned int *args = fwnode_args.args;
+	struct fwnode_handle *child;
+	int ret;
+
+	ret = fwnode_property_get_reference_args(fwnode, prop, NULL, nprops,
+						 index, &fwnode_args);
+	if (ret)
+		return ERR_PTR(ret == -EINVAL ? -ENOENT : ret);

Why map EINVAL to ENOENT? Needs a comment, either here or in the function description.

+
+	for (fwnode = fwnode_args.fwnode;
+	     nprops; nprops--, fwnode = child, props++, args++) {

I think you cram too much in this for-loop: fwnode, nprops, fwnode, props, args...
It's hard to parse.

I would make this a 'while (nprops)' and write out all the other assignments,
increments and decrements.

+		u32 val;
+
+		fwnode_for_each_child_node(fwnode, child) {
+			if (fwnode_property_read_u32(child, *props, &val))
+				continue;
+
+			if (val == *args)
+				break;

I'm lost. This really needs comments and perhaps even an DT or ACPI example
so you can see what exactly it is we're doing here.

+		}
+
+		fwnode_handle_put(fwnode);
+
+		if (!child) {
+			fwnode = ERR_PTR(-ENOENT);
+			break;
+		}
+	}
+
+	return fwnode;
+}
+
+static int v4l2_fwnode_reference_parse_int_props(
+	struct device *dev, struct v4l2_async_notifier *notifier,
+	const char *prop, const char **props, unsigned int nprops)

Needs comments describing what this does.

+{
+	struct fwnode_handle *fwnode;
+	unsigned int index = 0;
+	int ret;
+
+	while (!IS_ERR((fwnode = v4l2_fwnode_reference_get_int_prop(
+				dev_fwnode(dev), prop, index, props,
+				nprops)))) {
+		fwnode_handle_put(fwnode);
+		index++;
+	}
+
+	if (PTR_ERR(fwnode) != -ENOENT)
+		return PTR_ERR(fwnode);

Missing 'if (index == 0)'?

+
+	ret = v4l2_async_notifier_realloc(notifier,
+					  notifier->num_subdevs + index);
+	if (ret)
+		return -ENOMEM;
+
+	for (index = 0; !IS_ERR((fwnode = v4l2_fwnode_reference_get_int_prop(
+					 dev_fwnode(dev), prop, index, props,
+					 nprops))); ) {

I'd add 'index++' in this for-loop. It's weird that it is missing.

+		struct v4l2_async_subdev *asd;
+
+		if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) {
+			ret = -EINVAL;
+			goto error;
+		}
+
+		asd = kzalloc(sizeof(struct v4l2_async_subdev), GFP_KERNEL);
+		if (!asd) {
+			ret = -ENOMEM;
+			goto error;
+		}
+
+		notifier->subdevs[notifier->num_subdevs] = asd;
+		asd->match.fwnode.fwnode = fwnode;
+		asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
+		notifier->num_subdevs++;
+
+		fwnode_handle_put(fwnode);
+
+		index++;
+	}
+
+	return PTR_ERR(fwnode) == -ENOENT ? 0 : PTR_ERR(fwnode);
+
+error:
+	fwnode_handle_put(fwnode);
+	return ret;
+}
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Sakari Ailus [off-list ref]");
 MODULE_AUTHOR("Sylwester Nawrocki [off-list ref]");

Regards,

	Hans