Re: [PATCH 03/18] media: v4l: async: Simplify async sub-device fwnode matching

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

Hi Sakari,

On Mon, Apr 24, 2023 at 10:33:06PM +0300, Sakari Ailus wrote:
> On Mon, Apr 24, 2023 at 09:20:22PM +0200, Niklas Söderlund wrote:
> > On 2023-04-14 14:07:40 +0300, Sakari Ailus wrote:
> > > On Thu, Apr 13, 2023 at 06:50:04PM +0200, Jacopo Mondi wrote:
> > > > On Thu, Mar 30, 2023 at 02:58:38PM +0300, Sakari Ailus wrote:
> > > > > V4L2 async sub-device matching originally used the device nodes only.
> > > > > Endpoint nodes were taken into use instead as using the device nodes was
> > > > > problematic for it was in some cases ambiguous which link might have been
> > > > > in question.
> > > > >
> > > > > There is however no need to use endpoint nodes on both sides, as the async
> > > > > sub-device's fwnode can always be trivially obtained using
> > > > > fwnode_graph_get_remote_endpoint() when needed while what counts is
> > > > > whether or not the link is between two device nodes, i.e. the device nodes
> > > > > match.
> > > > 
> > > > As you know I'm a bit debated.
> > > > 
> > > > Strict endpoint-matching requires one subdev to be registed per each
> > > > endpoint, and this is tedious for drivers that have to register a
> > > > subdev for each of its endpoints
> > > > 
> > > > Allowing a subdev to be matched multiple times on different endpoints
> > > > gives a way for lazy drivers to take a shortcut and simplify their
> > > > topologies to a single subdev, when they would actually need more.
> > > 
> > > I'd say this is really about interface design, not being "lazy". It depends
> > > on the sub-device. Ideally the framework should be also as easy for drivers
> > > drivers to use as possible.
> > > 
> > > What is not supported, though, is multiple sub-devices with a single device
> > > node. Do we need that? At least I don't think I came across a driver that
> > > would.
> > 
> > If I understand you correctly about multiple sub-device from a single 
> > device node, this exists today. The ADV748x driver have a single device 
> > node in DT and register multiple sub-devices, one for each source 
> > endpoint.
> > 
> > The ADV748x have two CSI-2 transmitters, one 4-lane and one 1-lane as 
> > well as two different video capture "ports" one HDMI and one CVBS. Both 
> > capture ports can be active at the same time and routed internally 
> > inside the ADV748x to the two different CSI-2 transmitters.
> > 
> > In order todo this the ADV748x register multiple subdevices and modifies 
> > the fwnode to be the endpoint instead of the device node. So the change 
> > in this patch for ADV748x driver would break that driver.
> 
> Ah, indeed. I guess I'll need to support that case as well then. It doesn't
> seem to be troublesome to implement, but I'm tempted making it a special
> case: every other driver would apparently be fine matching with device
> fwnode whereas doing endpoint-to-endpoint matching adds complexity to the
> drivers. This patch removes about 100 lines of rather ugly code largely
> from v4l2-async.

It's only 50 lines from v4l2-async, and I don't think the code is uglier
than the rest of the file :-) In general, I prefer implementing tricky
code in the framework and simplifying drivers. I think our goals align
there, the framework should do the right thing by default for the
majority of cases. However, as Niklas pointed out, endpoint matching is
needed for drivers that register multiple subdevs with external
connections (such as the adv742x), and that's exactly why endpoint
matching was added in the first place. I think this needs to be kept.

> There are other issues in the set with connection-subdev relations, I'll
> post v2 to address those as well.

-- 
Regards,

Laurent Pinchart