On 09/13/17 12:07, Sakari Ailus wrote:
> Hi Hans,
>
> On Wed, Sep 13, 2017 at 11:28:44AM +0200, Hans Verkuil wrote:
>> On 09/13/17 11:24, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> Thanks for the review!
>>>
>>> On Wed, Sep 13, 2017 at 09:27:34AM +0200, Hans Verkuil wrote:
>>>> On 09/12/2017 03:41 PM, Sakari Ailus wrote:
>>>>> 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.
>>>
>>>>
>>>>> 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.
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Sakari Ailus <sakari.ai...@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.
>
>>
>>> + * 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."
>>> */
>>> if (ret != -ENOENT && ret != -ENODATA)
>>> return ret;
>>>
>
Regards,
Hans
