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.ailus@xxxxxxxxxxxxxxx>
>>> ---
>>> 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'.
> + * Right now, both -ENODATA and -ENOENT signal the end of
> + * references where only a single error code should be used
> + * for the purpose.
> */
> if (ret != -ENOENT && ret != -ENODATA)
> return ret;
>
Regards,
Hans
