Re: [PATCH 2/3] device property: Move fwnode graph ops to firmware specific locations

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Thu, Mar 30, 2017 at 06:10:19PM +0300, Sakari Ailus wrote:
> Hi Mika,
> 
> Thank you for the review.
> 
> On Mon, Mar 27, 2017 at 02:52:00PM +0300, Mika Westerberg wrote:
> > On Fri, Mar 24, 2017 at 01:03:51PM +0200, Sakari Ailus wrote:
> > >  const struct fwnode_operations acpi_fwnode_ops = {
> > >  	.property_present = acpi_fwnode_property_present,
> > >  	.property_read_int_array = acpi_fwnode_property_read_int_array,
> > > @@ -1193,4 +1248,9 @@ const struct fwnode_operations acpi_fwnode_ops = {
> > >  	.get_parent = acpi_fwnode_get_parent,
> > >  	.get_next_child_node = acpi_get_next_subnode,
> > >  	.get_named_child_node = acpi_fwnode_get_named_child_node,
> > > +	.graph_get_next_endpoint = acpi_fwnode_graph_get_next_endpoint,
> > > +	.graph_get_remote_endpoint = acpi_fwnode_graph_get_remote_endpoint,
> > > +	.graph_get_remote_port = acpi_fwnode_graph_get_remote_port,
> > > +	.graph_get_remote_port_parent = acpi_fwnode_graph_get_remote_port_parent,
> > > +	.graph_parse_endpoint = acpi_fwnode_graph_parse_endpoint,
> > >  };
> > 
> > Not sure if it is possible but it would be nice to have a single
> > primitive implementation specific graph callback and then build
> > everything else on top of that in generic code. Here you have 5
> > callbacks just for graph support.
> 
> Getting the parent of the port in OF graph is OF specific, the port parent
> is not necessarily a direct parent node of the port (in presence of the
> "ports" node that contains all port nodes).
> 
> I could potentially remove graph_get_remote_port() and use
> graph_get_remote_endpoint() and graph_get_parent() instead. I didn't
> originally do that as I thought it could be better ot leave it up to the
> implementation.
> 
> What do you think?

Well we are dealing with two kinds of objects here, ports and endpoins.
Maybe we can the low level functions parse and return these and then add
generic code on top of that only deals with the port and endpoint
objects. Something like get_graph() which parses the firmware structure
and returns Linux port and endpoint object tree.

Not sure if it is overkill in this case but adding 5 callbacks just for
graphs smells like the design could be improved ;-)