Hi Hans,
Thank you for the review.
On Tue, Sep 19, 2017 at 10:31:41AM +0200, Hans Verkuil wrote:
> Hi Sakari,
>
> I'm slowly starting to understand this. The example helped a lot. But I still have
> some questions, see below.
>
> On 09/15/2017 04:17 PM, Sakari Ailus wrote:
> > v4l2_fwnode_reference_parse_int_prop() will find an fwnode such that under
> > the device's own fwnode, it will follow child fwnodes with the given
> > property-value pair and return the resulting fwnode.
>
> I think both the subject, commit log, function comment and function name should
> reflect the fact that this function is for an ACPI reference.
>
> It's only called for ACPI (from patch 19):
>
> + if (props[i].props && is_acpi_node(dev_fwnode(dev)))
> + ret = v4l2_fwnode_reference_parse_int_props(
>
> So renaming it to v4l2_fwnode_acpi_reference_parse_int_props or something similar
> would clarify this fact.
I don't think we'll see many like this one. I presume we won't use it on DT
albeit there are no direct references to ACPI in the code itself.
How about v4l2_fwnode_parse_acpi_reference (+ "s" for the one below)?
>
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > drivers/media/v4l2-core/v4l2-fwnode.c | 201 ++++++++++++++++++++++++++++++++++
> > 1 file changed, 201 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index 65e84ea1cc35..968a345a288f 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -567,6 +567,207 @@ static int v4l2_fwnode_reference_parse(
> > return ret;
> > }
> >
> > +/*
> > + * v4l2_fwnode_reference_get_int_prop - parse a reference with integer
> > + * arguments
> > + * @dev: struct device pointer
> > + * @notifier: notifier for @dev
> > + * @prop: the name of the property
> > + * @index: the index of the reference to get
> > + * @props: the array of integer property names
> > + * @nprops: the number of integer property names in @nprops
>
> You mean 'in @props'?
Yes, I'll fix that.
>
> One thing that is not clear to me is when you would use an nprops value > 1.
> What's the use-case for that? It only makes sense (I think) if you would have
> property names that are all aliases of one another.
There may be several flash LEDs related to a sensor. That's the use case,
for instance.
>
> > + *
> > + * Find fwnodes referred to by a property @prop, then under that
> > + * iteratively, @nprops times, follow each child node which has a
> > + * property in @props array at a given child index the value of which
> > + * matches the integer argument at an index.
> > + *
> > + * For example, if this function was called with arguments and values
> > + * @dev corresponding to device "SEN", @prop == "flash-leds", @index
> > + * == 1, @props == { "led" }, @nprops == 1, with the ASL snippet below
> > + * it would return the node marked with THISONE. The @dev argument in
> > + * the ASL below.
>
> That last sentence about the @dev seems incomplete. I'm not sure what is
> meant by it.
I think it was meant to convey some information but it got added to the
previous sentence. I'll remove it.
>
> > + *
> > + * Device (LED)
> > + * {
> > + * Name (_DSD, Package () {
> > + * ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> > + * Package () {
> > + * Package () { "led0", "LED0" },
> > + * Package () { "led1", "LED1" },
> > + * }
> > + * })
> > + * Name (LED0, Package () {
> > + * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > + * Package () {
> > + * Package () { "led", 0 },
> > + * }
> > + * })
> > + * Name (LED1, Package () {
> > + * // THISONE
> > + * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > + * Package () {
> > + * Package () { "led", 1 },
> > + * }
> > + * })
> > + * }
> > + *
> > + * Device (SEN)
> > + * {
> > + * Name (_DSD, Package () {
> > + * ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > + * Package () {
> > + * Package () {
> > + * "flash-leds",
> > + * Package () { ^LED, 0, ^LED, 1 },
> > + * }
> > + * }
> > + * })
> > + * }
> > + *
> > + * where
> > + *
> > + * LED LED driver device
> > + * LED0 First LED
> > + * LED1 Second LED
> > + * SEN Camera sensor device (or another device the LED is
> > + * related to)
> > + *
> > + * Return: 0 on success
> > + * -ENOENT if no entries (or the property itself) were found
> > + * -EINVAL if property parsing otherwise failed
> > + * -ENOMEM if memory allocation failed
> > + */
> > +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)
> > +{
> > + struct fwnode_reference_args fwnode_args;
> > + unsigned int *args = fwnode_args.args;
> > + struct fwnode_handle *child;
> > + int ret;
> > +
> > + /*
> > + * Obtain remote fwnode as well as the integer arguments.
> > + *
> > + * Note that right now both -ENODATA and -ENOENT may signal
> > + * out-of-bounds access. Return -ENOENT in that case.
> > + */
> > + ret = fwnode_property_get_reference_args(fwnode, prop, NULL, nprops,
> > + index, &fwnode_args);
> > + if (ret)
> > + return ERR_PTR(ret == -ENODATA ? -ENOENT : ret);
> > +
> > + /*
> > + * Find a node in the tree under the referred fwnode corresponding the
> > + * integer arguments.
> > + */
> > + fwnode = fwnode_args.fwnode;
>
> So given the example above, fwnode would point to the LED device?
>
> If correct, then mention that in the comment.
It could be a LED driver device, but it could be something else as well.
Like a lens VCM, depending on the property being parsed. That's why I
didn't put it in the comments. But this is a device node, not a
hierarchical data extension node, for instance. That's what I think I
should add.
>
> > + while (nprops--) {
> > + u32 val;
> > +
> > + /* Loop over all child nodes under fwnode. */
>
> And here you check if the LED device has child nodes that have a *props
> property with a value matching the index.
>
> So given the example above it is looking for a child with property "led"
> and value 1.
>
> It's useful if that is mentioned in the comment as well.
But should I? This isn't specific to LEDs.
>
> > + fwnode_for_each_child_node(fwnode, child) {
> > + if (fwnode_property_read_u32(child, *props, &val))
> > + continue;
> > +
> > + /* Found property, see if its value matches. */
> > + if (val == *args)
> > + break;
> > + }
> > +
> > + fwnode_handle_put(fwnode);
> > +
> > + /* No property found; return an error here. */
> > + if (!child) {
> > + fwnode = ERR_PTR(-ENOENT);
> > + break;
> > + }
> > +
> > + props++;
> > + args++;
> > + fwnode = child;
> > + }
> > +
> > + return fwnode;
> > +}
> > +
> > +/*
> > + * v4l2_fwnode_reference_parse_int_props - parse references for async sub-devices
> > + * @dev: struct device pointer
> > + * @notifier: notifier for @dev
> > + * @prop: the name of the property
> > + * @props: the array of integer property names
> > + * @nprops: the number of integer properties
> > + *
> > + * Use v4l2_fwnode_reference_get_int_prop to find fwnodes through reference in
> > + * property @prop with integer arguments with child nodes matching in properties
> > + * @props. Then, set up V4L2 async sub-devices for those fwnodes in the notifier
> > + * accordingly.
> > + *
> > + * While it is technically possible to use this function on DT, it is only
> > + * meaningful on ACPI. On Device tree you can refer to any node in the tree but
> > + * on ACPI the references are limited to devices.
> > + *
> > + * Return: 0 on success
> > + * -ENOENT if no entries (or the property itself) were found
> > + * -EINVAL if property parsing otherwisefailed
> > + * -ENOMEM if memory allocation failed
> > + */
> > +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)
> > +{
> > + struct fwnode_handle *fwnode;
> > + unsigned int index;
> > + int ret;
> > +
> > + for (index = 0; !IS_ERR((fwnode = v4l2_fwnode_reference_get_int_prop(
> > + dev_fwnode(dev), prop, index, props,
> > + nprops))); index++)
> > + fwnode_handle_put(fwnode);
> > +
> > + /*
> > + * Note that right now both -ENODATA and -ENOENT may signal
> > + * out-of-bounds access. Return the error in cases other than that.
> > + */
> > + if (PTR_ERR(fwnode) != -ENOENT && PTR_ERR(fwnode) != -ENODATA)
> > + return PTR_ERR(fwnode);
> > +
> > + 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))); index++) {
> > + 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++;
> > + }
> > +
> > + 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>");
--
Kind regards,
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
