Unnamed repository; edit this file 'description' to name the repository.
 help / color / mirror / Atom feed
From: Sakari Ailus <sakari.ailus@iki.fi>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: "Laurent Pinchart" <laurent.pinchart+renesas@ideasonboard.com>,
	linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	"Jacopo Mondi" <jacopo+renesas@jmondi.org>,
	"Niklas Söderlund" <niklas.soderlund@ragnatech.se>,
	"Kieran Bingham" <kieran.bingham+renesas@ideasonboard.com>,
	"Lad Prabhakar" <prabhakar.csengg@gmail.com>
Subject: Re: [PATCH v3 1/3] media: v4l2-async: Accept endpoints and devices for fwnode matching
Date: Wed, 24 Jun 2020 13:04:59 +0300
Message-ID: <20200624100459.GH870@valkosipuli.retiisi.org.uk> (raw)
In-Reply-To: <20200624001253.GS5870@pendragon.ideasonboard.com>

Hi Laurent,

On Wed, Jun 24, 2020 at 03:12:53AM +0300, Laurent Pinchart wrote:
> 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 have been a different versions of the ACPI graph definitions, and
firmware using both exists. See e.g. is_acpi_graph_node() and functions
below that in drivers/acpi/property.c .

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

I think that'd be fine. If there's a need to change the implementation in
the future, it'll be easy.

-- 
Sakari Ailus

  reply	other threads:[~2020-06-24 10:05 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-21  0:00 [PATCH v3 0/3] " Laurent Pinchart
2020-06-21  0:00 ` [PATCH v3 1/3] " Laurent Pinchart
2020-06-23 21:13   ` Sakari Ailus
2020-06-23 21:22     ` Laurent Pinchart
2020-06-23 23:12       ` Sakari Ailus
2020-06-24  0:12         ` Laurent Pinchart
2020-06-24 10:04           ` Sakari Ailus [this message]
2020-06-21  0:00 ` [PATCH v3 2/3] media: v4l2-async: Pass notifier pointer to match functions Laurent Pinchart
2020-06-21  0:00 ` [PATCH v3 3/3] media: v4l2-async: Log message in case of heterogeneous fwnode match Laurent Pinchart
2020-06-30 21:48 ` [PATCH v3 0/3] media: v4l2-async: Accept endpoints and devices for fwnode matching Niklas Söderlund

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200624100459.GH870@valkosipuli.retiisi.org.uk \
    --to=sakari.ailus@iki.fi \
    --cc=jacopo+renesas@jmondi.org \
    --cc=kieran.bingham+renesas@ideasonboard.com \
    --cc=laurent.pinchart+renesas@ideasonboard.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=niklas.soderlund@ragnatech.se \
    --cc=prabhakar.csengg@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Unnamed repository; edit this file 'description' to name the repository.

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://archive.lwn.net:8080/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ http://archive.lwn.net:8080/linux-media \
		linux-media@vger.kernel.org lwn-linux-media@archive.lwn.net
	public-inbox-index linux-media

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://archive.lwn.net/lwn.kernel.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git