Re: [PATCH v3 1/6] device property: Helper to match multiple connections

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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