| Message ID | 20190316113605.12928-1-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,1/1] device property: Add fwnode_graph_get_endpoint_by_id | expand |
On Sat, Mar 16, 2019 at 12:36 PM Sakari Ailus <sakari.ailus@linux.intel.com> 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@linux.intel.com> > --- > 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. > + * @fwnode: pointer to parent fwnode_handle containing the graph "pointer to" is redundant here IMO. > + * @port: identifier of the port node > + * @endpoint: identifier of the endpoint node under the port node > + * @flags: fwnode graph flags s/graph/lookup/ ? > + * > + * Returns the fwnode handle to the local endpoint corresponding the port and s/to/of/ ? Also there should be "corresponding to the port ..." And I would say "Return ..." > + * 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". > + * > + * 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." > + */ > +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. > + bool disabled = flags & FWNODE_GRAPH_DEVICE_DISABLED; I would call this "enabled_only" and reverse its meaning. > + > + 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; > + > + 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? > + > + /* 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). > + > + /* > + * 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." > + if (fwnode_ep.id < endpoint || > + (best_ep && best_ep_id < fwnode_ep.id)) Drop the fwnode_ep reference? > + continue; > + > + /* Replace the one we had with the newly found one. */ Redundant comment. > + fwnode_handle_put(best_ep); Does this always work if best_ep is NULL? > + 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." > + * @FWNODE_GRAPH_DEVICE_DISABLED that the device to which the remote Missing colon. > + * 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) ? > + 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)); ) > --
On Tue, Mar 26, 2019 at 11:53 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Sat, Mar 16, 2019 at 12:36 PM Sakari Ailus > <sakari.ailus@linux.intel.com> 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@linux.intel.com> > > --- > > 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. > > > + * @fwnode: pointer to parent fwnode_handle containing the graph > > "pointer to" is redundant here IMO. > > > + * @port: identifier of the port node > > + * @endpoint: identifier of the endpoint node under the port node > > + * @flags: fwnode graph flags > > s/graph/lookup/ ? > > > + * > > + * Returns the fwnode handle to the local endpoint corresponding the port and > > s/to/of/ ? > > Also there should be "corresponding to the port ..." > > And I would say "Return ..." > > > + * 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". > > > + * > > + * 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." > > > + */ > > +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. > > > + bool disabled = flags & FWNODE_GRAPH_DEVICE_DISABLED; > > I would call this "enabled_only" and reverse its meaning. > > > + > > + 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; > > > + > > + 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? > > > + > > + /* 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). > > > + > > + /* > > + * 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." > > > + if (fwnode_ep.id < endpoint || > > + (best_ep && best_ep_id < fwnode_ep.id)) > > Drop the fwnode_ep reference? > > > + continue; > > + > > + /* Replace the one we had with the newly found one. */ > > Redundant comment. > > > + fwnode_handle_put(best_ep); > > Does this always work if best_ep is NULL? > > > + 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." > > > + * @FWNODE_GRAPH_DEVICE_DISABLED that the device to which the remote > > Missing colon. > > > + * 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) ? > > > + FWNODE_GRAPH_DEVICE_DISABLED = 0x00000002, > > BIT(2) ? > > > +}; Actually, enum types are not particularly suitable for flags in my view, because something like FWNODE_GRAPH_ENDPOINT_NEXT | FWNODE_GRAPH_DEVICE_DISABLED is not really defined within the enum type, for example. It's better to #define the flags IMO and use unsigned int as the type.
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@linux.intel.com> 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@linux.intel.com> > > --- > > 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)); ) > > --
Hi Rafael, On Wed, Mar 27, 2019 at 10:42:23AM +0100, Rafael J. Wysocki wrote: ... > > > + */ > > > +enum fwnode_graph_get_endpoint_flags { > > > + FWNODE_GRAPH_ENDPOINT_NEXT = 0x00000001, > > > > BIT(1) ? > > > > > + FWNODE_GRAPH_DEVICE_DISABLED = 0x00000002, > > > > BIT(2) ? > > > > > +}; > > Actually, enum types are not particularly suitable for flags in my > view, because something like > > FWNODE_GRAPH_ENDPOINT_NEXT | FWNODE_GRAPH_DEVICE_DISABLED > > is not really defined within the enum type, for example. > > It's better to #define the flags IMO and use unsigned int as the type. Enums can have better kerneldoc documentation; that's been the reasoning to make flags fields enums in e.g. V4L2 kAPI. In C it's still valid use of enums even if the value isn't any value listen in an enum as such. Anyway, I'll switch to #define and make the comment just a regular comment if you prefer that.
On Wed, Mar 27, 2019 at 10:56 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Rafael, > > Thank you for the review. No problem. That's my job after all ... [cut] > > > > > + * > > > + * 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/ ? That works too. > > > > > + */ > > > +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. Every nonzero value is regarded as "true" in C because of the lack of a bool type in the language, but since we pretend to have one ... > 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. OK > > > > > + > > > + /* 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. Yes, please drop 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. OK > > > > > + 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. OK > > > > > + 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, ..."? Sure. > > > > > + * @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. Right, sorry.
On Wed, Mar 27, 2019 at 11:02 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Rafael, > > On Wed, Mar 27, 2019 at 10:42:23AM +0100, Rafael J. Wysocki wrote: > ... > > > > + */ > > > > +enum fwnode_graph_get_endpoint_flags { > > > > + FWNODE_GRAPH_ENDPOINT_NEXT = 0x00000001, > > > > > > BIT(1) ? > > > > > > > + FWNODE_GRAPH_DEVICE_DISABLED = 0x00000002, > > > > > > BIT(2) ? > > > > > > > +}; > > > > Actually, enum types are not particularly suitable for flags in my > > view, because something like > > > > FWNODE_GRAPH_ENDPOINT_NEXT | FWNODE_GRAPH_DEVICE_DISABLED > > > > is not really defined within the enum type, for example. > > > > It's better to #define the flags IMO and use unsigned int as the type. > > Enums can have better kerneldoc documentation; that's been the reasoning to > make flags fields enums in e.g. V4L2 kAPI. In C it's still valid use of > enums even if the value isn't any value listen in an enum as such. > > Anyway, I'll switch to #define and make the comment just a regular comment > if you prefer that. I do. I did something like that with device links flags, for example.
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 + * @fwnode: pointer to parent fwnode_handle containing the graph + * @port: identifier of the port node + * @endpoint: identifier of the endpoint node under the port node + * @flags: fwnode graph flags + * + * Returns the fwnode handle to the local endpoint corresponding the port and + * endpoint IDs or NULL if not found. + * + * 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. + */ +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; + bool disabled = flags & FWNODE_GRAPH_DEVICE_DISABLED; + + 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; + + dev = fwnode_graph_get_remote_port_parent(ep); + + 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; + + /* Is an exact match needed? If so, skip this one. */ + if (!endpoint_next) + continue; + + /* + * Is this endpoint better than we already had? + */ + if (fwnode_ep.id < endpoint || + (best_ep && best_ep_id < fwnode_ep.id)) + continue; + + /* Replace the one we had with the newly found one. */ + fwnode_handle_put(best_ep); + 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 + * @FWNODE_GRAPH_DEVICE_DISABLED that the device to which the remote + * endpoint of the given endpoint belongs to, + * may be disabled + */ +enum fwnode_graph_get_endpoint_flags { + FWNODE_GRAPH_ENDPOINT_NEXT = 0x00000001, + FWNODE_GRAPH_DEVICE_DISABLED = 0x00000002, +}; + +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)); )
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@linux.intel.com> --- 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(+)