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.
>
> > + * 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;
> >
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
