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

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Hans,

Thanks for the review.

On Mon, Sep 11, 2017 at 11:38:58AM +0200, Hans Verkuil wrote:
> 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?

The fwnode related a struct device, in other words what dev_fwnode(dev)
gives you. This is either struct device.fwnode or struct
device.of_node.fwnode, depending on which firmware interface was used to
create the device.

I'll add a note of this.

> 
> > 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().

Yes, I think I changed the naming but forgot to update the commit. I'll do
that now.

> 
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-fwnode.c | 93 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 93 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index 4821c4989119..56eee5bbd3b5 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -496,6 +496,99 @@ static int v4l2_fwnode_reference_parse(
> >  	return ret;
> >  }
> >  
> > +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)
> 
> Need comments describing what this does.

Yes. I'll also rename it (get -> read) for consistency with the async
changes.

> 
> > +{
> > +	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.

fwnode_property_get_reference_args() returns currently a little bit
different error codes in ACPI / DT. This is worth documenting there and
fixing as well.

> 
> > +
> > +	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.

Hmm. I'm not sure if that really helps; the function is just handling each
entry in the array and related array pointers are changed accordingly. The
fwnode = child assignment is there to move to the child node. I.e. what you
need for handling the loop itself.

I can change this though if you think it really makes a difference for
better.

> 
> 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.

I'll add comments to the code. A good example will be ACPI documentation
for LEDs, see 17th patch in v9. That will go through the linux-pm tree so
it won't be available in the same tree for a while.

> 
> > +		}
> > +
> > +		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.

Will add.

> 
> > +{
> > +	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)'?

Yes, will add.

> 
> > +
> > +	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.

Agreed, I'll move it there.

> 
> > +		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 <sakari.ailus@linux.intel.com>");
> >  MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> > 

-- 
Regards,

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