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 > >