| Message ID | 20220208031944.3444-2-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | typec: mux: Introduce support for multiple TypeC muxes | expand |
On Mon, Feb 07, 2022 at 07:19:39PM -0800, Bjorn Andersson wrote: > In some cases multiple connections with the same connection id > needs to be resolved from a fwnode graph. > > One such example is when separate hardware is used for performing muxing > and/or orientation switching of the SuperSpeed and SBU lines in a USB-C USB Type-C ? > connector. In this case the connector needs to belong to a graph with > multiple matching remote endpoints, and the TypeC controller needs to be Type-C ? > able to resolve them both. > > Add a new API that allows this kind of lookup. > > Given that the match() callback returns an opaque reference to something > provided by the client it's not possible for the implementation to > release the returned object and as such it's not possible to handle > errors, which in turn means that it's not possible to query the number > of elements or dynamically grow the results array. It's however expected > that the number of matches will be reasonably low and that the worst > case is known by the caller before hand. ... > + fwnode_graph_for_each_endpoint(fwnode, ep) { > + if (count >= matches_len) { > + fwnode_handle_put(ep); > + return count; > + } > + > + node = fwnode_graph_get_remote_port_parent(ep); > + if (!fwnode_device_is_available(node)) { > + fwnode_handle_put(node); > + continue; > + } > + > + ret = match(node, con_id, data); > + fwnode_handle_put(node); > + Redundant blank line (it seems the current style w/o this). Ditto for the below function. > + if (ret) > + matches[count++] = ret; > + } ... > +/** > + * fwnode_connection_find_matches - Find connections from a device node > + * @fwnode: Device node with the connection > + * @con_id: Identifier for the connection > + * @data: Data for the match function > + * @match: Function to check and convert the connection description > + * @matches: Array of pointers to fill with matches > + * @matches_len: Length of @matches > + * > + * Find up to @matches_len connections with unique identifier @con_id between > + * @fwnode and other device nodes. @match will be used to convert the > + * connection description to data the caller is expecting to be returned > + * through the @matches array. > + * > + * Return: Number of matches resolved, of negative errno. s/of/or/ ? > + */ > +int fwnode_connection_find_matches(struct fwnode_handle *fwnode, > + const char *con_id, void *data, > + devcon_match_fn_t match, > + void **matches, unsigned int matches_len) > +{ > + unsigned int count; > + > + if (!fwnode || !match || !matches) !matches case may be still useful to get the count and allocate memory by caller. Please, consider this case. > + return -EINVAL; > + > + count = fwnode_graph_devcon_matches(fwnode, con_id, data, match, > + matches, matches_len); > + > + return count + fwnode_devcon_matches(fwnode, con_id, data, match, > + matches + count, > + matches_len - count); I haven't found any explanation what the difference between two counts. Also can you define two count variables with distinct names and do something like count_A = ... matches += count; matches_len -= count; count_B = ... return count_A + count_B; ? > +}
On Wed 09 Feb 04:30 PST 2022, Andy Shevchenko wrote: > On Mon, Feb 07, 2022 at 07:19:39PM -0800, Bjorn Andersson wrote: > > In some cases multiple connections with the same connection id > > needs to be resolved from a fwnode graph. > > > > One such example is when separate hardware is used for performing muxing > > and/or orientation switching of the SuperSpeed and SBU lines in a USB-C > > USB Type-C ? > > > connector. In this case the connector needs to belong to a graph with > > multiple matching remote endpoints, and the TypeC controller needs to be > > Type-C ? > > > able to resolve them both. > > > > Add a new API that allows this kind of lookup. > > > > Given that the match() callback returns an opaque reference to something > > provided by the client it's not possible for the implementation to > > release the returned object and as such it's not possible to handle > > errors, which in turn means that it's not possible to query the number > > of elements or dynamically grow the results array. It's however expected > > that the number of matches will be reasonably low and that the worst > > case is known by the caller before hand. > > ... > > > + fwnode_graph_for_each_endpoint(fwnode, ep) { > > + if (count >= matches_len) { > > + fwnode_handle_put(ep); > > + return count; > > + } > > + > > + node = fwnode_graph_get_remote_port_parent(ep); > > + if (!fwnode_device_is_available(node)) { > > + fwnode_handle_put(node); > > + continue; > > + } > > + > > + ret = match(node, con_id, data); > > + fwnode_handle_put(node); > > > + > > Redundant blank line (it seems the current style w/o this). > Ditto for the below function. > > > + if (ret) > > + matches[count++] = ret; > > + } > > ... > > > +/** > > + * fwnode_connection_find_matches - Find connections from a device node > > + * @fwnode: Device node with the connection > > + * @con_id: Identifier for the connection > > + * @data: Data for the match function > > + * @match: Function to check and convert the connection description > > + * @matches: Array of pointers to fill with matches > > + * @matches_len: Length of @matches > > + * > > + * Find up to @matches_len connections with unique identifier @con_id between > > + * @fwnode and other device nodes. @match will be used to convert the > > + * connection description to data the caller is expecting to be returned > > + * through the @matches array. > > + * > > + * Return: Number of matches resolved, of negative errno. > > s/of/or/ ? > > > + */ > > +int fwnode_connection_find_matches(struct fwnode_handle *fwnode, > > + const char *con_id, void *data, > > + devcon_match_fn_t match, > > + void **matches, unsigned int matches_len) > > +{ > > + unsigned int count; > > + > > + if (!fwnode || !match || !matches) > > !matches case may be still useful to get the count and allocate memory by > caller. Please, consider this case. > As discussed in previous version, and described in the commit message, the returned value of "match" is a opaque pointer to something which has to be passed back to the caller in order to be cleaned up. E.g. the typec mux code returns a pointer to a typec_mux/switch object with a refcounted struct device within, or an ERR_PTR(). So unfortunately we can must gather the results into matches and pass it back to the caller to take consume or clean up. Thanks for your review, I'll update the patches according to the other feedback. Regards, Bjorn > > + return -EINVAL; > > + > > + count = fwnode_graph_devcon_matches(fwnode, con_id, data, match, > > + matches, matches_len); > > + > > + return count + fwnode_devcon_matches(fwnode, con_id, data, match, > > + matches + count, > > + matches_len - count); > > I haven't found any explanation what the difference between two counts. Also > can you define two count variables with distinct names and do something like > > count_A = ... > > matches += count; > matches_len -= count; > > count_B = ... > > return count_A + count_B; > > ? > > > +} > > -- > With Best Regards, > Andy Shevchenko > >
On Fri, Feb 18, 2022 at 11:00:45AM -0800, Bjorn Andersson wrote: > On Wed 09 Feb 04:30 PST 2022, Andy Shevchenko wrote: > > On Mon, Feb 07, 2022 at 07:19:39PM -0800, Bjorn Andersson wrote: ... > > > +int fwnode_connection_find_matches(struct fwnode_handle *fwnode, > > > + const char *con_id, void *data, > > > + devcon_match_fn_t match, > > > + void **matches, unsigned int matches_len) > > > +{ > > > + unsigned int count; > > > + > > > + if (!fwnode || !match || !matches) > > > > !matches case may be still useful to get the count and allocate memory by > > caller. Please, consider this case. > > > > As discussed in previous version, and described in the commit message, > the returned value of "match" is a opaque pointer to something which > has to be passed back to the caller in order to be cleaned up. > > E.g. the typec mux code returns a pointer to a typec_mux/switch object > with a refcounted struct device within, or an ERR_PTR(). > > So unfortunately we can must gather the results into matches and pass it > back to the caller to take consume or clean up. It's fine. You have **matches, means pointer of an opaque pointer. What I'm talking about is memory allocation for and array of _pointers_. That's what caller very much aware of and can allocate on heap. So, please consider this case.
On Sun 20 Feb 03:16 PST 2022, Andy Shevchenko wrote: > On Fri, Feb 18, 2022 at 11:00:45AM -0800, Bjorn Andersson wrote: > > On Wed 09 Feb 04:30 PST 2022, Andy Shevchenko wrote: > > > On Mon, Feb 07, 2022 at 07:19:39PM -0800, Bjorn Andersson wrote: > > ... > > > > > +int fwnode_connection_find_matches(struct fwnode_handle *fwnode, > > > > + const char *con_id, void *data, > > > > + devcon_match_fn_t match, > > > > + void **matches, unsigned int matches_len) > > > > +{ > > > > + unsigned int count; > > > > + > > > > + if (!fwnode || !match || !matches) > > > > > > !matches case may be still useful to get the count and allocate memory by > > > caller. Please, consider this case. > > > > > > > As discussed in previous version, and described in the commit message, > > the returned value of "match" is a opaque pointer to something which > > has to be passed back to the caller in order to be cleaned up. > > > > E.g. the typec mux code returns a pointer to a typec_mux/switch object > > with a refcounted struct device within, or an ERR_PTR(). > > > > So unfortunately we can must gather the results into matches and pass it > > back to the caller to take consume or clean up. > > > It's fine. You have **matches, means pointer of an opaque pointer. > What I'm talking about is memory allocation for and array of _pointers_. > That's what caller very much aware of and can allocate on heap. So, please > consider this case. > I'm sorry, but I'm not sure what you're looking for. I still interpret your comment as that it would be nice to be able to do something like: count = fwnode_connection_find_matches(fwnode, "orientation-switch", NULL, typec_switch_match, NULL, 0); based on the returned value the caller could allocate an array of "count" pointers and then call the function again to actually fill out the count elements. The problem with this is that, typec_switch_match() does: void *typec_switch_match(fwnode, id, data) { struct device *dev = find_struct_device(fwnode, id); if (!dev) return NULL; get_device(dev); return container_of(dev, struct typec_switch, dev); } So if we call the match function and if that finds a "dev" it will return a struct typec_switch with a refcounted struct device within. We can see if that's NULL or not and will be able to return a "count", but we have no way of releasing the reference acquired - we must return the void pointer back to the client, so that it can release it. My claim is that this is not a problem, because this works fine with any reasonable size of fwnode graphs we might run into - and the client will in general have a sense of the worst case number of matches (in this series its 3, as there's 3 types of lanes that can be switched/muxed coming out of a USB connector). But that's perhaps not what you're referring to? Or perhaps I'm missing something else? Regards, Bjorn
On Sun, Feb 20, 2022 at 08:55:10PM -0800, Bjorn Andersson wrote: > On Sun 20 Feb 03:16 PST 2022, Andy Shevchenko wrote: > > On Fri, Feb 18, 2022 at 11:00:45AM -0800, Bjorn Andersson wrote: > > > On Wed 09 Feb 04:30 PST 2022, Andy Shevchenko wrote: > > > > On Mon, Feb 07, 2022 at 07:19:39PM -0800, Bjorn Andersson wrote: ... > > > > > +int fwnode_connection_find_matches(struct fwnode_handle *fwnode, > > > > > + const char *con_id, void *data, > > > > > + devcon_match_fn_t match, > > > > > + void **matches, unsigned int matches_len) > > > > > +{ > > > > > + unsigned int count; > > > > > + > > > > > + if (!fwnode || !match || !matches) > > > > > > > > !matches case may be still useful to get the count and allocate memory by > > > > caller. Please, consider this case. > > > > > > As discussed in previous version, and described in the commit message, > > > the returned value of "match" is a opaque pointer to something which > > > has to be passed back to the caller in order to be cleaned up. > > > > > > E.g. the typec mux code returns a pointer to a typec_mux/switch object > > > with a refcounted struct device within, or an ERR_PTR(). > > > > > > So unfortunately we can must gather the results into matches and pass it > > > back to the caller to take consume or clean up. > > > > It's fine. You have **matches, means pointer of an opaque pointer. > > What I'm talking about is memory allocation for and array of _pointers_. > > That's what caller very much aware of and can allocate on heap. So, please > > consider this case. > > I'm sorry, but I'm not sure what you're looking for. > > > I still interpret your comment as that it would be nice to be able to do > something like: > > count = fwnode_connection_find_matches(fwnode, "orientation-switch", > NULL, typec_switch_match, NULL, 0); > > based on the returned value the caller could allocate an array of > "count" pointers and then call the function again to actually fill out > the count elements. Yes, that's what I want from the generic fwnode APIs. (Keyword: generic) > The problem with this is that, typec_switch_match() does: As you stated, the problem is in the typec_switch_match(). So, it's not related to the fwnode, but how you are using it. > void *typec_switch_match(fwnode, id, data) { > struct device *dev = find_struct_device(fwnode, id); > if (!dev) > return NULL; > get_device(dev); > return container_of(dev, struct typec_switch, dev); > } > > So if we call the match function and if that finds a "dev" it will > return a struct typec_switch with a refcounted struct device within. fwnode (as being an abstraction on top of the others) has no knowledge about this. And more important should not know that. > We can see if that's NULL or not and will be able to return a "count", > but we have no way of releasing the reference acquired - we must return > the void pointer back to the client, so that it can release it. The caller (if it wants to!) may create different callbacks for count and real matching, no? > My claim is that this is not a problem, because this works fine with any > reasonable size of fwnode graphs we might run into - and the client will > in general have a sense of the worst case number of matches (in this > series its 3, as there's 3 types of lanes that can be switched/muxed > coming out of a USB connector).
On Mon 21 Feb 09:19 PST 2022, Andy Shevchenko wrote: > On Sun, Feb 20, 2022 at 08:55:10PM -0800, Bjorn Andersson wrote: > > On Sun 20 Feb 03:16 PST 2022, Andy Shevchenko wrote: > > > On Fri, Feb 18, 2022 at 11:00:45AM -0800, Bjorn Andersson wrote: > > > > On Wed 09 Feb 04:30 PST 2022, Andy Shevchenko wrote: > > > > > On Mon, Feb 07, 2022 at 07:19:39PM -0800, Bjorn Andersson wrote: > > ... > > > > > > > +int fwnode_connection_find_matches(struct fwnode_handle *fwnode, > > > > > > + const char *con_id, void *data, > > > > > > + devcon_match_fn_t match, > > > > > > + void **matches, unsigned int matches_len) > > > > > > +{ > > > > > > + unsigned int count; > > > > > > + > > > > > > + if (!fwnode || !match || !matches) > > > > > > > > > > !matches case may be still useful to get the count and allocate memory by > > > > > caller. Please, consider this case. > > > > > > > > As discussed in previous version, and described in the commit message, > > > > the returned value of "match" is a opaque pointer to something which > > > > has to be passed back to the caller in order to be cleaned up. > > > > > > > > E.g. the typec mux code returns a pointer to a typec_mux/switch object > > > > with a refcounted struct device within, or an ERR_PTR(). > > > > > > > > So unfortunately we can must gather the results into matches and pass it > > > > back to the caller to take consume or clean up. > > > > > > It's fine. You have **matches, means pointer of an opaque pointer. > > > What I'm talking about is memory allocation for and array of _pointers_. > > > That's what caller very much aware of and can allocate on heap. So, please > > > consider this case. > > > > I'm sorry, but I'm not sure what you're looking for. > > > > > > I still interpret your comment as that it would be nice to be able to do > > something like: > > > > count = fwnode_connection_find_matches(fwnode, "orientation-switch", > > NULL, typec_switch_match, NULL, 0); > > > > based on the returned value the caller could allocate an array of > > "count" pointers and then call the function again to actually fill out > > the count elements. > > Yes, that's what I want from the generic fwnode APIs. > (Keyword: generic) > > > The problem with this is that, typec_switch_match() does: > > As you stated, the problem is in the typec_switch_match(). So, it's not related > to the fwnode, but how you are using it. > > > void *typec_switch_match(fwnode, id, data) { > > struct device *dev = find_struct_device(fwnode, id); > > if (!dev) > > return NULL; > > get_device(dev); > > return container_of(dev, struct typec_switch, dev); > > } > > > > So if we call the match function and if that finds a "dev" it will > > return a struct typec_switch with a refcounted struct device within. > > fwnode (as being an abstraction on top of the others) has no knowledge > about this. And more important should not know that. > > > We can see if that's NULL or not and will be able to return a "count", > > but we have no way of releasing the reference acquired - we must return > > the void pointer back to the client, so that it can release it. > > The caller (if it wants to!) may create different callbacks for count and real > matching, no? > Ahh, yeah you're right, we can shift this responsibility onto the caller and thereby allow them to implement the count as well. Makes sense! Thanks, Bjorn > > My claim is that this is not a problem, because this works fine with any > > reasonable size of fwnode graphs we might run into - and the client will > > in general have a sense of the worst case number of matches (in this > > series its 3, as there's 3 types of lanes that can be switched/muxed > > coming out of a USB connector). > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/base/property.c b/drivers/base/property.c index e6497f6877ee..5230ff5c8d48 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -1189,6 +1189,38 @@ fwnode_graph_devcon_match(struct fwnode_handle *fwnode, const char *con_id, return NULL; } +static unsigned int fwnode_graph_devcon_matches(struct fwnode_handle *fwnode, + const char *con_id, void *data, + devcon_match_fn_t match, + void **matches, + unsigned int matches_len) +{ + struct fwnode_handle *node; + struct fwnode_handle *ep; + unsigned int count = 0; + void *ret; + + fwnode_graph_for_each_endpoint(fwnode, ep) { + if (count >= matches_len) { + fwnode_handle_put(ep); + return count; + } + + node = fwnode_graph_get_remote_port_parent(ep); + if (!fwnode_device_is_available(node)) { + fwnode_handle_put(node); + continue; + } + + ret = match(node, con_id, data); + fwnode_handle_put(node); + + if (ret) + matches[count++] = ret; + } + return count; +} + static void * fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id, void *data, devcon_match_fn_t match) @@ -1211,6 +1243,35 @@ fwnode_devcon_match(struct fwnode_handle *fwnode, const char *con_id, return NULL; } +static unsigned int fwnode_devcon_matches(struct fwnode_handle *fwnode, + const char *con_id, void *data, + devcon_match_fn_t match, + void **matches, + unsigned int matches_len) +{ + struct fwnode_handle *node; + unsigned int count = 0; + unsigned int i; + void *ret; + + for (i = 0; ; i++) { + if (count >= matches_len) + return count; + + node = fwnode_find_reference(fwnode, con_id, i); + if (IS_ERR(node)) + break; + + ret = match(node, NULL, data); + fwnode_handle_put(node); + + if (ret) + matches[count++] = ret; + } + + return count; +} + /** * fwnode_connection_find_match - Find connection from a device node * @fwnode: Device node with the connection @@ -1238,3 +1299,38 @@ void *fwnode_connection_find_match(struct fwnode_handle *fwnode, return fwnode_devcon_match(fwnode, con_id, data, match); } EXPORT_SYMBOL_GPL(fwnode_connection_find_match); + +/** + * fwnode_connection_find_matches - Find connections from a device node + * @fwnode: Device node with the connection + * @con_id: Identifier for the connection + * @data: Data for the match function + * @match: Function to check and convert the connection description + * @matches: Array of pointers to fill with matches + * @matches_len: Length of @matches + * + * Find up to @matches_len connections with unique identifier @con_id between + * @fwnode and other device nodes. @match will be used to convert the + * connection description to data the caller is expecting to be returned + * through the @matches array. + * + * Return: Number of matches resolved, of negative errno. + */ +int fwnode_connection_find_matches(struct fwnode_handle *fwnode, + const char *con_id, void *data, + devcon_match_fn_t match, + void **matches, unsigned int matches_len) +{ + unsigned int count; + + if (!fwnode || !match || !matches) + return -EINVAL; + + count = fwnode_graph_devcon_matches(fwnode, con_id, data, match, + matches, matches_len); + + return count + fwnode_devcon_matches(fwnode, con_id, data, match, + matches + count, + matches_len - count); +} +EXPORT_SYMBOL_GPL(fwnode_connection_find_matches); diff --git a/include/linux/property.h b/include/linux/property.h index 7399a0b45f98..aa7ef9d06ebe 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -446,6 +446,11 @@ static inline void *device_connection_find_match(struct device *dev, return fwnode_connection_find_match(dev_fwnode(dev), con_id, data, match); } +int fwnode_connection_find_matches(struct fwnode_handle *fwnode, + const char *con_id, void *data, + devcon_match_fn_t match, + void **matches, unsigned int matches_len); + /* -------------------------------------------------------------------------- */ /* Software fwnode support - when HW description is incomplete or missing */
In some cases multiple connections with the same connection id needs to be resolved from a fwnode graph. One such example is when separate hardware is used for performing muxing and/or orientation switching of the SuperSpeed and SBU lines in a USB-C connector. In this case the connector needs to belong to a graph with multiple matching remote endpoints, and the TypeC controller needs to be able to resolve them both. Add a new API that allows this kind of lookup. Given that the match() callback returns an opaque reference to something provided by the client it's not possible for the implementation to release the returned object and as such it's not possible to handle errors, which in turn means that it's not possible to query the number of elements or dynamically grow the results array. It's however expected that the number of matches will be reasonably low and that the worst case is known by the caller before hand. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- Changes since v1: - Iterator in fwnode_devcon_matches() is now unsigned. - fwnode_handle_put() node for unavailable nodes. - Extended commit message on the subject of supporting dynamically sized "matches" array. drivers/base/property.c | 96 ++++++++++++++++++++++++++++++++++++++++ include/linux/property.h | 5 +++ 2 files changed, 101 insertions(+)