Hi Rafael,
Thank you for the review.
On Tue, Mar 26, 2019 at 11:53:25PM +0100, Rafael J. Wysocki wrote:
> On Sat, Mar 16, 2019 at 12:36 PM Sakari Ailus
> <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> >
> > fwnode_graph_get_endpoint_by_id() is intended for obtaining local
> > endpoints by a given local port. fwnode_graph_get_endpoint_by_id() is
> > slightly different from its OF counterpart is
> > of_graph_get_endpoint_by_regs(): instead of using -1 as a value to signify
> > that a port or an endpoint number does not matter, it uses flags to look
> > for equal or greater endpoint. The port number is always fixed. It also
> > returns only remote endpoints that belong to an available device, a
> > behaviour that can be turned off with a flag.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > since v1:
> >
> > - Remove the PORT_NEXT flag.
> >
> > - Replace the ENDPOINT_AVAILABLE flag with DEVICE_DISABLED flag and
> > so effectively inverting its functionality.
> >
> > - Rework the loop iterating over endpoint to find the best one. It's more
> > simple and better commented now.
> >
> > - Fixes in indentation and documentation (e.g. fwnode_node_put ->
> > fwnode_handle_put).
> >
> > drivers/base/property.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/property.h | 19 ++++++++++++
> > 2 files changed, 100 insertions(+)
> >
> > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > index 8b91ab380d14..7b908cadbdd5 100644
> > --- a/drivers/base/property.c
> > +++ b/drivers/base/property.c
> > @@ -984,6 +984,87 @@ fwnode_graph_get_remote_node(const struct fwnode_handle *fwnode, u32 port_id,
> > EXPORT_SYMBOL_GPL(fwnode_graph_get_remote_node);
> >
> > /**
> > + * fwnode_graph_get_endpoint_by_id - get endpoint node by port and endpoint
> > + * numbers
>
> IMO you can drop "node" from the description line and then it will fit
> under 80 chars.
Done.
>
> > + * @fwnode: pointer to parent fwnode_handle containing the graph
>
> "pointer to" is redundant here IMO.
Fixed.
>
> > + * @port: identifier of the port node
> > + * @endpoint: identifier of the endpoint node under the port node
> > + * @flags: fwnode graph flags
>
> s/graph/lookup/ ?
Yes.
>
> > + *
> > + * Returns the fwnode handle to the local endpoint corresponding the port and
>
> s/to/of/ ?
Yes.
>
> Also there should be "corresponding to the port ..."
I'd say "to" doesn't belong there. I guess neither of us is a native
speaker so I'd hope to get a comment from one. :-)
>
> And I would say "Return ..."
Fixed.
>
> > + * endpoint IDs or NULL if not found.
>
> I would say here "If FWNODE_GRAPH_ENDPOINT_NEXT is passed in @flags
> and the specified endpoint has not been found, look for the closest
> endpoint ID greater than the specified one and return the endpoint
> that corresponds to it, if present".
>
> Also "Do not return endpoints that belong to disabled devices, unless
> FWNODE_GRAPH_DEVICE_DISABLED is passed in @flags".
I replaced the original with this as such.
>
> > + *
> > + * Flags may be set in order to obtain the endpoint instead of just returning
> > + * the specified one or none at all, or to only return endpoints that belong to
> > + * a device that is available.
> > + *
> > + * Use fwnode_handle_put() on the endpoint fwnode handle when done using it.
>
> I would say "The returned endpoint needs to be released by calling
> fwnode_handle_put() on it when it is not necessary any more."
How about s/necessary/needed/ ?
>
> > + */
> > +struct fwnode_handle *
> > +fwnode_graph_get_endpoint_by_id(const struct fwnode_handle *fwnode,
> > + u32 port, u32 endpoint,
> > + enum fwnode_graph_get_endpoint_flags flags)
> > +{
> > + struct fwnode_handle *ep = NULL, *best_ep = NULL;
> > + unsigned int best_ep_id = 0;
> > + bool endpoint_next = flags & FWNODE_GRAPH_ENDPOINT_NEXT;
>
> I tend to apply !! to things like the right-hand side of the above to
> get a proper bool value.
It's not necessary: casting any non-zero value to bool is true. I couldn't
find an authoritative reference tut it's not hard to find examples around
the kernel, including arch/x86.
>
> > + bool disabled = flags & FWNODE_GRAPH_DEVICE_DISABLED;
>
> I would call this "enabled_only" and reverse its meaning.
Makes sense, that makes the code below easier to follow.
>
> > +
> > + while ((ep = fwnode_graph_get_next_endpoint(fwnode, ep))) {
> > + struct fwnode_endpoint fwnode_ep = { 0 };
> > + int ret;
> > +
> > + /*
> > + * Check the device is available unless we're explicitly told
> > + * not to.
> > + */
> > + if (!disabled) {
> > + struct fwnode_handle *dev;
>
> s/dev/dev_node/
>
> bool available;
>
> > +
> > + dev = fwnode_graph_get_remote_port_parent(ep);
>
> available = fwnode_device_is_available(dev_node);
> fwnode_handle_put(dev_node);
> if (!available)
> continue;
Yes, makes sense.
>
> > +
> > + if (!fwnode_device_is_available(dev)) {
> > + fwnode_handle_put(dev);
> > + continue;
> > + }
> > +
> > + fwnode_handle_put(dev);
> > + }
> > +
> > + ret = fwnode_graph_parse_endpoint(ep, &fwnode_ep);
> > + if (ret < 0)
> > + continue;
> > +
> > + /* Check we have the right port. */
> > + if (fwnode_ep.port != port)
> > + continue;
> > +
> > + /* Is this an exact match? If so, return it immediately. */
> > + if (fwnode_ep.id == endpoint)
> > + return ep;
>
> This appears to mean that the endpoint has been reference-counted, but
> then the reference should be dropped before the "continue" above,
> shouldn't it?
This is handled by fwnode_graph_get_next_endpoint() which puts the previous
endpoint on every iteration.
>
> > +
> > + /* Is an exact match needed? If so, skip this one. */
> > + if (!endpoint_next)
> > + continue;
>
> And here?
>
> Besides, the three comments above are redundant IMO (they don't
> explain anything, but just repeat what the code does).
Well, true; I thought they might be useful to understand what the function
does. I have no problem removing them.
>
> > +
> > + /*
> > + * Is this endpoint better than we already had?
> > + */
>
> I would say in this comment "If the endpoint that has just been found
> is not the first matching one and the ID of the one found previously
> is closer to the requested endpoint ID, skip it."
Yes.
>
> > + if (fwnode_ep.id < endpoint ||
> > + (best_ep && best_ep_id < fwnode_ep.id))
>
> Drop the fwnode_ep reference?
fwnode_graph_parse_endpoint() takes no reference.
>
> > + continue;
> > +
> > + /* Replace the one we had with the newly found one. */
>
> Redundant comment.
Removed.
>
> > + fwnode_handle_put(best_ep);
>
> Does this always work if best_ep is NULL?
Yes, the fwnode API functions (mostly?) are safe to call with NULL fwnode;
at least this one is.
>
> > + best_ep = fwnode_handle_get(ep);
> > + best_ep_id = fwnode_ep.id;
> > + }
> > +
> > + return best_ep;
> > +}
> > +EXPORT_SYMBOL_GPL(fwnode_graph_get_endpoint_by_id);
> > +
> > +/**
> > * fwnode_graph_parse_endpoint - parse common endpoint node properties
> > * @fwnode: pointer to endpoint fwnode_handle
> > * @endpoint: pointer to the fwnode endpoint data structure
> > diff --git a/include/linux/property.h b/include/linux/property.h
> > index 3789ec755fb6..f3d924092890 100644
> > --- a/include/linux/property.h
> > +++ b/include/linux/property.h
> > @@ -304,6 +304,25 @@ struct fwnode_handle *
> > fwnode_graph_get_remote_node(const struct fwnode_handle *fwnode, u32 port,
> > u32 endpoint);
> >
> > +/**
> > + * enum fwnode_graph_get_endpoint_flags - Flags for finding an endpoint
> > + *
> > + * @FWNODE_GRAPH_ENDPOINT_NEXT: if no specified endpoint is found, obtain the
> > + * smallest endpoint number greater than specified
>
> "In the no exact match case, look for the closest endpoint ID greater
> than the specified one."
How about: "In the case of no exact match, ..."?
>
> > + * @FWNODE_GRAPH_DEVICE_DISABLED that the device to which the remote
>
> Missing colon.
Fixed.
>
> > + * endpoint of the given endpoint belongs to,
> > + * may be disabled
>
> "Also return endpoints that belong to disabled devices."
>
> > + */
> > +enum fwnode_graph_get_endpoint_flags {
> > + FWNODE_GRAPH_ENDPOINT_NEXT = 0x00000001,
>
> BIT(1) ?
BIT(0), BIT(1) below.
>
> > + FWNODE_GRAPH_DEVICE_DISABLED = 0x00000002,
>
> BIT(2) ?
>
> > +};
> > +
> > +struct fwnode_handle *
> > +fwnode_graph_get_endpoint_by_id(const struct fwnode_handle *fwnode,
> > + u32 port, u32 endpoint,
> > + enum fwnode_graph_get_endpoint_flags flags);
> > +
> > #define fwnode_graph_for_each_endpoint(fwnode, child) \
> > for (child = NULL; \
> > (child = fwnode_graph_get_next_endpoint(fwnode, child)); )
> > --
--
Kind regards,
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
