Re: [PATCH v2] software_node: Add support for fwnode_graph*() family of functions

[Sakari Ailus]

On Fri, Sep 18, 2020 at 11:57:09AM +0300, Heikki Krogerus wrote:
> On Fri, Sep 18, 2020 at 10:57:41AM +0300, Sakari Ailus wrote:
> > On Fri, Sep 18, 2020 at 08:46:52AM +0100, Dan Scally wrote:
> > > On 18/09/2020 08:34, Sakari Ailus wrote:
> > > > On Fri, Sep 18, 2020 at 07:49:31AM +0100, Dan Scally wrote:
> > > >> Good morning
> > > >>
> > > >> On 18/09/2020 07:22, Sakari Ailus wrote:
> > > >>> Hi Dan,
> > > >>>
> > > >>> On Wed, Sep 16, 2020 at 02:22:10PM +0100, Dan Scally wrote:
> > > >>>> Hi Sakari - thanks for the comments
> > > >>>>
> > > >>>> On 16/09/2020 10:17, Sakari Ailus wrote:
> > > >>>>> Moi Daniel and Heikki,
> > > >>>>>
> > > >>>>> On Wed, Sep 16, 2020 at 12:28:27AM +0100, Daniel Scally wrote:
> > > >>>>>> From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > > >>>>>>
> > > >>>>>> This implements the remaining .graph_* callbacks in the
> > > >>>>>> fwnode operations vector for the software nodes. That makes
> > > >>>>>> the fwnode_graph*() functions available in the drivers also
> > > >>>>>> when software nodes are used.
> > > >>>>>>
> > > >>>>>> The implementation tries to mimic the "OF graph" as much as
> > > >>>>>> possible, but there is no support for the "reg" device
> > > >>>>>> property. The ports will need to have the index in their
> > > >>>>>> name which starts with "port" (for example "port0", "port1",
> > > >>>>>> ...) and endpoints will use the index of the software node
> > > >>>>>> that is given to them during creation. The port nodes can
> > > >>>>>> also be grouped under a specially named "ports" subnode,
> > > >>>>>> just like in DT, if necessary.
> > > >>>>>>
> > > >>>>>> The remote-endpoints are reference properties under the
> > > >>>>>> endpoint nodes that are named "remote-endpoint". 
> > > >>>>>>
> > > >>>>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > > >>>>>> Co-developed-by: Daniel Scally <djrscally@xxxxxxxxx>
> > > >>>>>> Signed-off-by: Daniel Scally <djrscally@xxxxxxxxx>
> > > >>>>>> ---
> > > >>>>>> changes in v2:
> > > >>>>>> 	- added software_node_device_is_available
> > > >>>>>> 	- altered software_node_get_next_child to get references
> > > >>>>>> 	- altered software_node_get_next_endpoint to release references
> > > >>>>>> 	to ports and avoid passing invalid combinations of swnodes to
> > > >>>>>> 	software_node_get_next_child
> > > >>>>>> 	- altered swnode_graph_find_next_port to release port rather than
> > > >>>>>> 	old
> > > >>>>>> 	
> > > >>>>>>  drivers/base/swnode.c | 129 +++++++++++++++++++++++++++++++++++++++++-
> > > >>>>>>  1 file changed, 127 insertions(+), 2 deletions(-)
> > > >>>>>>
> > > >>>>>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> > > >>>>>> index 010828fc785b..d69034b807e3 100644
> > > >>>>>> --- a/drivers/base/swnode.c
> > > >>>>>> +++ b/drivers/base/swnode.c
> > > >>>>>> @@ -363,6 +363,11 @@ static void software_node_put(struct fwnode_handle *fwnode)
> > > >>>>>>  	kobject_put(&swnode->kobj);
> > > >>>>>>  }
> > > >>>>>>  
> > > >>>>>> +static bool software_node_device_is_available(const struct fwnode_handle *fwnode)
> > > >>>>>> +{
> > > >>>>>> +	return is_software_node(fwnode);
> > > >>>>> This basically tells whether the device is there. Are there software node
> > > >>>>> based devices, i.e. do you need this?
> > > >>>>>
> > > >>>>> If you do really need this, then I guess this could just return true for
> > > >>>>> now as if you somehow get here, the node is a software node anyway.
> > > >>>> I do think its better to include it; I'm targeting using this with
> > > >>>> ipu3-cio2; the cio2_parse_firmware() call there doesn't pass
> > > >>>> FWNODE_GRAPH_DEVICE_DISABLED to fwnode_graph_get_endpoint_by_id() so
> > > >>> I wonder if this has something to do with replacing the device's fwnode
> > > >>> in the cio2-bridge patch.
> > > >>>
> > > >>> It's the device that needs to be enabled, and it's not a software node.
> > > >>>
> > > >> I think it is because of that yes, but I don't see a way around it at
> > > >> the moment - unless there's a way to attach the software_node port and
> > > >> endpoints that cio2-bridge creates to the device's existing firmware
> > > >> instead.
> > > > I thought this was how it was meant to be used?
> > > >
> > > > The secondary field is there for this purpose. But it may be not all fwnode
> > > > interface functions operate on fwnode->secondary?
> > > Let me test it; it might just require some changes to
> > > software_node_graph_get_port_parent() to check if the parent fwnode is a
> > > secondary, and if it is to return the primary instead.
> > 
> > Ah, indeed. I forgot this part. I wonder if it'd cause issues to return the
> > primary if you've got the secondary swnode.
> > 
> > Heikki, any idea?
> > 
> > Code elsewhere (e.g. V4L2 fwnode framework + drivers) assume a device is
> > identified by a single fwnode, not two --- currently the swnode graph
> > function returning port parent returns the secondary so there's no match
> > with the primary fwnode.
> 
> Sorry I don't think I understand the scenario here, but never return
> the primary node when the software node is the secondary from the
> software node API! The software node functions deal and return
> software nodes, and nothing else, just like ACPI deals with ACPI nodes
> only and DT deals with OF nodes only. We must never jump between the
> fwnode types at this level. That also means that if you want to
> describe the device graph with software nodes, then every node in the
> graph, starting from the port parents, must be a software node.
> Whether or not the node is secondary is irrelevant. But I guess this
> is not a problem here (or is it?).

The way software nodes work (as in this patch) is not consistent with DT or
ACPI. For instance, the parent of the port node, returned by
software_node_graph_get_port_parent() is fwnode->secondary of the device,
not device's fwnode.

This is not expected by the users of the fwnode property API.

Also, it leads to drivers only seeing the software nodes while DT and ACPI
nodes as well as properties would be hidden.

> 
> Considering the secondary node will unfortunately need to be done by
> the callers of fwnode API when the fwnode API can't take care of that.

What problems would there be in returning the primary fwnode?

-- 
Regards,

Sakari Ailus