On 09/11/2017 02:28 PM, Sakari Ailus wrote:
> 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@xxxxxxxxxxxxxxx>
>>> ---
>>> 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.
Which async changes? Since the fwnode_handle that's returned is refcounted
I wonder if 'get' isn't the right name in this case.
>
>>
>>> +{
>>> + 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 think so, yes. I noticed you like complex for-loops :-)
>
>>
>> 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.
Ideally an ACPI and an equivalent DT example would be nice to have, but I might
be asking too much. I'm not that familiar with ACPI, so for me a DT example
is easier.
>
>>
>>> + }
>>> +
>>> + 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@xxxxxxxxxxxxxxx>");
>>> MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>");
>>>
>
Regards,
Hans
