From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: "Laurent Pinchart" <laurent.pinchart+renesas@ideasonboard.com>,
linux-media@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] media: v4l2-async: Accept endpoints and devices for fwnode matching
Date: Wed, 18 Mar 2020 15:35:36 +0200 [thread overview]
Message-ID: <20200318133536.GD15448@kekkonen.localdomain> (raw)
In-Reply-To: <20200318112216.GC4733@pendragon.ideasonboard.com>
Hi Laurent,
On Wed, Mar 18, 2020 at 01:22:16PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Wed, Mar 18, 2020 at 09:52:25AM +0200, Sakari Ailus wrote:
> > On Wed, Mar 18, 2020 at 02:17:26AM +0200, Laurent Pinchart wrote:
> > > On Wed, Mar 18, 2020 at 01:04:32AM +0200, Laurent Pinchart wrote:
> > >> On Tue, Mar 17, 2020 at 02:44:56PM +0200, Sakari Ailus wrote:
> > >>> On Sun, Mar 15, 2020 at 02:55:11PM +0200, 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 node name) 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>
> > >>>> ---
> > >>>> This has been compile-tested only. Prabhakar, could you check if it
> > >>>> fixes your issue ?
> > >>>>
> > >>>> drivers/media/v4l2-core/v4l2-async.c | 42 +++++++++++++++++++++++++++-
> > >>>> 1 file changed, 41 insertions(+), 1 deletion(-)
> > >>>>
> > >>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > >>>> index 8bde33c21ce4..995e5464cba7 100644
> > >>>> --- a/drivers/media/v4l2-core/v4l2-async.c
> > >>>> +++ b/drivers/media/v4l2-core/v4l2-async.c
> > >>>> @@ -71,7 +71,47 @@ 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;
> > >>>> + const char *name;
> > >>>> +
> > >>>> + /*
> > >>>> + * 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.
> > >>>> + */
> > >>>> + name = fwnode_get_name(sd->fwnode);
> > >>>> + sd_fwnode_is_ep = name && strstarts(name, "endpoint");
> > >>>> + name = fwnode_get_name(asd->match.fwnode);
> > >>>> + asd_fwnode_is_ep = name && strstarts(name, "endpoint");
> > >>>
> > >>> Apart from the fact that you're parsing graph node names here, this looks
> > >>> good.
> > >
> > > And why is that an issue btw ? the ACPI fwnode ops seem to provide a
> > > .get_name() operation, would it return the ACPI bus ID here ?
> >
> > I'd really prefer not to do graph parsing outside the main parser(s), OF,
> > ACPI and property frameworks.
> >
> > Just for an example, the v4l2_fwnode_link_parse() was broken for ACPI for a
> > long time just because it did not use the graph parsing functions, but
> > implemented graph parsing on its own.
> >
> > >>> How about checking instead that calling
> > >>> fwnode_graph_get_remote_endpoint(fwnode_graph_get_remote_endpoint()) yields
> > >>> the same node? That would ensure you're dealing with endpoint nodes without
> > >>> explicitly parsing the graph in any way.
> > >>
> > >> Would it be simpler to check for the presence of an endpoint property ?
> >
> > There's no endpoint property, apart from an old ACPI definition.
>
> There isn't ? How does it work on ACPI then ?
> acpi_graph_get_remote_endpoint() starts with
>
> ret = acpi_node_get_property_reference(__fwnode, "remote-endpoint", 0,
> &args);
The remote-endpoint property is used in ACPI, too, yes. But the question
was about a property named "endpoint".
>
> > There are differences in the implementations and this is not the best place
> > to try to take them all into account.
>
> OK, but in that case I think we need an fwnode_graph_is_endpoint().
Thinking about this --- could you check fwnode_graph_get_remote_endpoint()
returns non-NULL, and then put the remote? That would be more simple as you
are only interested in knowing you're dealing with an endpoint. I don't
object having a little helper for this though.
--
Regards,
Sakari Ailus
prev parent reply other threads:[~2020-03-18 13:35 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-15 10:27 [PATCH 0/2] rcar-csi2: make use V4L2_ASYNC_MATCH_CUSTOM to do fwnode matching Lad Prabhakar
2020-03-15 10:27 ` [PATCH 1/2] media: v4l2-async: Pass pointer to struct v4l2_subdev in match_custom callback Lad Prabhakar
2020-03-15 10:56 ` Laurent Pinchart
2020-03-15 10:27 ` [PATCH 2/2] media: rcar-csi2: Let the driver handle fwnode matching using " Lad Prabhakar
2020-03-15 10:29 ` Laurent Pinchart
2020-03-15 12:10 ` Lad, Prabhakar
2020-03-15 12:57 ` Laurent Pinchart
2020-03-15 12:55 ` [PATCH] media: v4l2-async: Accept endpoints and devices for fwnode matching Laurent Pinchart
2020-03-15 13:32 ` Lad, Prabhakar
2020-03-15 21:47 ` Jacopo Mondi
2020-03-16 6:34 ` Laurent Pinchart
2020-03-16 8:59 ` Jacopo Mondi
2020-03-16 9:56 ` Laurent Pinchart
2020-03-16 21:40 ` Niklas Söderlund
2020-03-16 21:47 ` Laurent Pinchart
2020-03-17 12:33 ` Kieran Bingham
2020-03-17 23:08 ` Laurent Pinchart
2020-03-17 12:44 ` Sakari Ailus
2020-03-17 23:04 ` Laurent Pinchart
2020-03-18 0:17 ` Laurent Pinchart
2020-03-18 7:52 ` Sakari Ailus
2020-03-18 11:22 ` Laurent Pinchart
2020-03-18 13:35 ` Sakari Ailus [this message]
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=20200318133536.GD15448@kekkonen.localdomain \
--to=sakari.ailus@linux.intel.com \
--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=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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).