Hi Hans,
On Tue, Sep 19, 2017 at 10:40:14AM +0200, Hans Verkuil wrote:
> On 09/19/2017 10:20 AM, Sakari Ailus wrote:
> > Hi Hans,
> >
> > Thank you for the review.
> >
> > On Tue, Sep 19, 2017 at 10:03:27AM +0200, Hans Verkuil wrote:
> >> On 09/15/2017 04:17 PM, Sakari Ailus wrote:
...
> >>> +static int __v4l2_async_notifier_parse_fwnode_endpoints(
> >>> + struct device *dev, struct v4l2_async_notifier *notifier,
> >>> + size_t asd_struct_size, unsigned int port, bool has_port,
> >>> + int (*parse_endpoint)(struct device *dev,
> >>> + struct v4l2_fwnode_endpoint *vep,
> >>> + struct v4l2_async_subdev *asd))
> >>> +{
> >>> + struct fwnode_handle *fwnode = NULL;
> >>> + unsigned int max_subdevs = notifier->max_subdevs;
> >>> + int ret;
> >>> +
> >>> + if (WARN_ON(asd_struct_size < sizeof(struct v4l2_async_subdev)))
> >>> + return -EINVAL;
> >>> +
> >>> + for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> >>> + dev_fwnode(dev), fwnode)); ) {
> >>
> >> You can replace this by:
> >>
> >> while ((fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev), fwnode))) {
> >>
> >>> + if (!fwnode_device_is_available(
> >>> + fwnode_graph_get_port_parent(fwnode)))
> >>> + continue;
> >>> +
> >>> + if (has_port) {
> >>> + struct fwnode_endpoint ep;
> >>> +
> >>> + ret = fwnode_graph_parse_endpoint(fwnode, &ep);
> >>> + if (ret) {
> >>> + fwnode_handle_put(fwnode);
> >>> + return ret;
> >>> + }
> >>> +
> >>> + if (ep.port != port)
> >>> + continue;
> >>> + }
> >>> + max_subdevs++;
> >>> + }
> >>> +
> >>> + /* No subdevs to add? Return here. */
> >>> + if (max_subdevs == notifier->max_subdevs)
> >>> + return 0;
> >>> +
> >>> + ret = v4l2_async_notifier_realloc(notifier, max_subdevs);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> >>> + dev_fwnode(dev), fwnode)); ) {
> >>
> >> Same here: this can be a 'while'.
> >
> > The fwnode = NULL assignment still needs to be done. A for loop has a
> > natural initialiser for the loop, I think it's cleaner than using while
> > here.
>
> After the previous while fwnode is NULL again (since that's when the while
> stops).
>
> >
> > The macro would be implemented this way as well.
> >
> > For the loop above this one, I'd use for for consistency: it's the same
> > loop after all.
> >
> > This reminds me --- I'll send the patch for the macro.
>
> If this is going to be replaced by a macro, then disregard my comment.
Yes. I just sent that to linux-acpi (as well as devicetree and to you).
...
> >>> +/**
> >>> + * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode endpoints in a
> >>> + * device node
> >>> + * @dev: the device the endpoints of which are to be parsed
> >>> + * @notifier: notifier for @dev
> >>> + * @asd_struct_size: size of the driver's async sub-device struct, including
> >>> + * sizeof(struct v4l2_async_subdev). The &struct
> >>> + * v4l2_async_subdev shall be the first member of
> >>> + * the driver's async sub-device struct, i.e. both
> >>> + * begin at the same memory address.
> >>> + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
> >>> + * endpoint. Optional.
> >>> + * Return: %0 on success
> >>> + * %-ENOTCONN if the endpoint is to be skipped but this
> >>> + * should not be considered as an error
> >>> + * %-EINVAL if the endpoint configuration is invalid
> >>> + *
> >>> + * Parse the fwnode endpoints of the @dev device and populate the async sub-
> >>> + * devices array of the notifier. The @parse_endpoint callback function is
> >>> + * called for each endpoint with the corresponding async sub-device pointer to
> >>> + * let the caller initialize the driver-specific part of the async sub-device
> >>> + * structure.
> >>> + *
> >>> + * The notifier memory shall be zeroed before this function is called on the
> >>> + * notifier.
> >>> + *
> >>> + * This function may not be called on a registered notifier and may be called on
> >>> + * a notifier only once.
> >>> + *
> >>> + * Do not change the notifier's subdevs array, take references to the subdevs
> >>> + * array itself or change the notifier's num_subdevs field. This is because this
> >>> + * function allocates and reallocates the subdevs array based on parsing
> >>> + * endpoints.
> >>> + *
> >>> + * The @struct v4l2_fwnode_endpoint passed to the callback function
> >>> + * @parse_endpoint is released once the function is finished. If there is a need
> >>> + * to retain that configuration, the user needs to allocate memory for it.
> >>> + *
> >>> + * Any notifier populated using this function must be released with a call to
> >>> + * v4l2_async_notifier_release() after it has been unregistered and the async
> >>> + * sub-devices are no longer in use, even if the function returned an error.
> >>> + *
> >>> + * Return: %0 on success, including when no async sub-devices are found
> >>> + * %-ENOMEM if memory allocation failed
> >>> + * %-EINVAL if graph or endpoint parsing failed
> >>> + * Other error codes as returned by @parse_endpoint
> >>> + */
> >>> +int v4l2_async_notifier_parse_fwnode_endpoints(
> >>> + struct device *dev, struct v4l2_async_notifier *notifier,
> >>> + size_t asd_struct_size,
> >>> + int (*parse_endpoint)(struct device *dev,
> >>> + struct v4l2_fwnode_endpoint *vep,
> >>> + struct v4l2_async_subdev *asd));
> >>> +
> >>> +/**
> >>> + * v4l2_async_notifier_parse_fwnode_endpoints_by_port - Parse V4L2 fwnode
> >>> + * endpoints of a port in a
> >>> + * device node
> >>> + * @dev: the device the endpoints of which are to be parsed
> >>> + * @notifier: notifier for @dev
> >>> + * @asd_struct_size: size of the driver's async sub-device struct, including
> >>> + * sizeof(struct v4l2_async_subdev). The &struct
> >>> + * v4l2_async_subdev shall be the first member of
> >>> + * the driver's async sub-device struct, i.e. both
> >>> + * begin at the same memory address.
> >>> + * @port: port number where endpoints are to be parsed
> >>> + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
> >>> + * endpoint. Optional.
> >>> + * Return: %0 on success
> >>> + * %-ENOTCONN if the endpoint is to be skipped but this
> >>> + * should not be considered as an error
> >>> + * %-EINVAL if the endpoint configuration is invalid
> >>> + *
> >>> + * This function is just like @v4l2_async_notifier_parse_fwnode_endpoints with
> >>> + * the exception that it only parses endpoints in a given port. This is useful
> >>> + * on devices that have both sinks and sources: the async sub-devices connected
> >>
> >> on -> for
> >>
> >>> + * to sources have already been set up by another driver (on capture devices).
> >>
> >> on -> for
> >
> > Agreed on both.
> >
> >>
> >> So if I understand this correctly for devices with both sinks and sources you use
> >> this function to just parse the sink ports. And you have to give explicit port
> >> numbers since you can't tell from parsing the device tree if a port is a sink or
> >> source port, right? Only the driver knows this.
> >
> > Correct. The graph data structure in DT isn't directed, so this is only
> > known by the driver.
>
> I think this should be clarified.
>
> I wonder if there is any way around it. I don't have time to dig into this, but
> isn't it possible to tell that the source ports are already configured?
Well, this is essentially what the documentation is attempting to convey.
:-)
I can add this / change the existing wording, if you think it could help.
--
Regards,
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
