Hi Andy, Björn,
On Fri, Mar 04, 2022 at 02:54:21PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 03, 2022 at 02:33:46PM -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
> > Type-C connector. In this case the connector needs to belong to a graph
> > with multiple matching remote endpoints, and the Type-C controller needs
> > to be able to resolve them both.
> >
> > Add a new API that allows this kind of lookup.
>
> ...
>
> > +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 && matches) {
> > + fwnode_handle_put(ep);
> > + return count;
> > + }
>
> Wouldn't be the same as
>
> if (count >= matches_len) {
> fwnode_handle_put(ep);
> break;
> }
Don't you need to check for non-NULL matches here?
I find return above easier to read.
>
> ?
>
> > + 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) {
> > + if (matches)
> > + matches[count] = ret;
> > + count++;
> > + }
> > + }
> > + return count;
> > +}
>
> ...
>
> > +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 && matches)
> > + return count;
>
> Ditto.
>
> > + node = fwnode_find_reference(fwnode, con_id, i);
> > + if (IS_ERR(node))
> > + break;
> > +
> > + ret = match(node, NULL, data);
> > + fwnode_handle_put(node);
> > + if (ret) {
> > + if (matches)
> > + matches[count] = ret;
> > + count++;
> > + }
> > + }
> > +
> > + return count;
> > +}
>
> ...
>
> > +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_graph;
> > + unsigned int count_ref;
> > +
> > + if (!fwnode || !match)
> > + return -EINVAL;
>
> > + count_graph = fwnode_graph_devcon_matches(fwnode, con_id, data, match,
> > + matches, matches_len);
>
> > + matches += count_graph;
> > + matches_len -= count_graph;
>
> No, won't work when matches == NULL.
>
> Also, matches_len is expected to be 0 in that case (or at least being ignored,
> check with vsnprintf() behaviour in similar case).
>
> So, something like this, perhaps
>
> if (matches && matches_len) {
> matches += count_graph;
> matches_len -= count_graph;
> }
Good find!
>
> > + count_ref = fwnode_devcon_matches(fwnode, con_id, data, match,
> > + matches, matches_len);
> > +
> > + return count_graph + count_ref;
> > +}
>
>
--
Regards,
Sakari Ailus
