Re: [PATCH v3 1/3] media: v4l2-async: Accept endpoints and devices for fwnode matching

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sakari,

On Wed, Jun 24, 2020 at 02:12:05AM +0300, Sakari Ailus wrote:
> On Wed, Jun 24, 2020 at 12:22:41AM +0300, Laurent Pinchart wrote:
> > On Wed, Jun 24, 2020 at 12:13:06AM +0300, Sakari Ailus wrote:
> >> On Sun, Jun 21, 2020 at 03:00:26AM +0300, Laurent Pinchart wrote:
> >>> fwnode matching was designed to match on nodes corresponding to a
> >>> device. Some drivers, however, needed to match on endpoints, and have
> >>> passed endpoint fwnodes to v4l2-async. This works when both the subdev
> >>> and the notifier use the same fwnode types (endpoint or device), but
> >>> makes drivers that use different types incompatible.
> >>> 
> >>> Fix this by extending the fwnode match to handle fwnodes of different
> >>> types. When the types (deduced from the presence of remote endpoints)
> >>> are different, retrieve the device fwnode for the side that provides an
> >>> endpoint fwnode, and compare it with the device fwnode provided by the
> >>> other side. This allows interoperability between all drivers, regardless
> >>> of which type of fwnode they use for matching.
> >>> 
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>> Changes since v2:
> >>> 
> >>> - Add comment to explain that we're matching connecting endpoints
> >>> - Don't check fwnode name to detect endpoint
> >>> ---
> >>>  drivers/media/v4l2-core/v4l2-async.c | 45 +++++++++++++++++++++++++++-
> >>>  1 file changed, 44 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> >>> index 8bde33c21ce4..f82e0a32647d 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-async.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-async.c
> >>> @@ -71,7 +71,50 @@ static bool match_devname(struct v4l2_subdev *sd,
> >>>  
> >>>  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> >>>  {
> >>> -	return sd->fwnode == asd->match.fwnode;
> >>> +	struct fwnode_handle *other_fwnode;
> >>> +	struct fwnode_handle *dev_fwnode;
> >>> +	bool asd_fwnode_is_ep;
> >>> +	bool sd_fwnode_is_ep;
> >>> +
> >>> +	/*
> >>> +	 * Both the subdev and the async subdev can provide either an endpoint
> >>> +	 * fwnode or a device fwnode. Start with the simple case of direct
> >>> +	 * fwnode matching.
> >>> +	 */
> >>> +	if (sd->fwnode == asd->match.fwnode)
> >>> +		return true;
> >>> +
> >>> +	/*
> >>> +	 * Otherwise, check if the sd fwnode and the asd fwnode refer to an
> >>> +	 * endpoint or a device. If they're of the same type, there's no match.
> >>> +	 * Technically speaking this checks if the nodes refer to a connected
> >>> +	 * endpoint, which is the simplest check that works for both OF and
> >>> +	 * ACPI. This won't make a difference, as drivers should not try to
> >>> +	 * match unconnected endpoints.
> >>> +	 */
> >>> +	sd_fwnode_is_ep = fwnode_property_present(sd->fwnode,
> >>> +						  "remote-endpoint");
> >>> +	asd_fwnode_is_ep = fwnode_property_present(asd->match.fwnode,
> >>> +						   "remote-endpoint");
> >> 
> >> Please don't try parsing graph bindings outside the main parsers.
> > 
> > Why is that ? On the DT side, bindings are considered to be stable, so
> > isolating their parsing in helpers would not help with ABI compatibility
> > anyway. Maybe it would be useful if you could explain how graph parsing
> > works in ACPI ? The fact that there's a remote-endpoint property without
> > endpoints is a the minimum quite puzzling.
> 
> No other drivers (or frameworks to my knowledge) work with the graphs
> directly anymore. There was a staging driver (IMX) that did but that has
> been fixed now. It's easier to ensure the code is correct --- this is
> because the data structure is hard to parse, especially while taking
> firmware type differences into account but the functions that access it are
> relatively simple to use.
> 
> The fwnode property API has operations callbacks that are specific to the
> type of the node. Most access functions have a firmware specific backend.
> 
> With the presence of the "remote-endpoint" property there's no variation
> across the firmware types, at least not right now. But still putting it
> here right now looks like technical debt to me: much of the code parsing
> graph data structure outside the main parser has been buggy in the past.

For my information, could you still briefly explain how remote-endpoint
works on ACPI, without any fwnode named "endpoint" ?

> >> There's no API function to do just this, but you could go and check for the
> >> port parent right away. The code might be even more simple that way.
> > 
> > How will that help ? With OF at least, fwnode_graph_get_port_parent()
> > will return the grand-parent if the passed node isn't an endpoint, not
> > much can be deduced from that.
> 
> I meant to say fwnode_graph_get_remote_endpoint(). You'd need to release
> the fwnode reference, too.

That makes more sense :-)

> >> Alternatively, I guess we could add fwnode_graph_is_endpoint() or something
> >> but I wonder if it'd be worth it just for this.

Would

static inline bool fwnode_graph_is_endpoint(struct fwnode_handle *fwnode)
{
	return fwnode_property_present(fwnode, "remote-endpoint");
}

in include/linux/property.h be acceptable to you ?

> >>> +
> >>> +	if (sd_fwnode_is_ep == asd_fwnode_is_ep)
> >>> +		return false;
> >>> +
> >>> +	/*
> >>> +	 * The sd and asd fwnodes are of different types. Get the device fwnode
> >>> +	 * parent of the endpoint fwnode, and compare it with the other fwnode.
> >>> +	 */
> >>> +	if (sd_fwnode_is_ep) {
> >>> +		dev_fwnode = fwnode_graph_get_port_parent(sd->fwnode);
> >>> +		other_fwnode = asd->match.fwnode;
> >>> +	} else {
> >>> +		dev_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode);
> >>> +		other_fwnode = sd->fwnode;
> >>> +	}
> >>> +
> >>> +	fwnode_handle_put(dev_fwnode);
> >>> +
> >>> +	return dev_fwnode == other_fwnode;
> >>>  }
> >>>  
> >>>  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)

-- 
Regards,

Laurent Pinchart