Re: [PATCH v11 04/15] media: v4l2-fwnode: add initial connector parsing support

From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Marco Felsch <m.felsch@pengutronix.de>
Cc: mchehab@kernel.org, hans.verkuil@cisco.com,
	jacopo+renesas@jmondi.org, robh+dt@kernel.org,
	laurent.pinchart@ideasonboard.com, devicetree@vger.kernel.org,
	kernel@pengutronix.de, linux-media@vger.kernel.org
Subject: Re: [PATCH v11 04/15] media: v4l2-fwnode: add initial connector parsing support
Date: Tue, 18 Feb 2020 14:18:40 +0200	[thread overview]
Message-ID: <20200218121839.GA5379@paasikivi.fi.intel.com> (raw)
In-Reply-To: <20200115170621.jszy2p3e4w3b3hpn@pengutronix.de>

Hi Marco,

On Wed, Jan 15, 2020 at 06:06:21PM +0100, Marco Felsch wrote:
> Hi Sakari,
> 
> On 19-11-16 01:34, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Mon, Sep 30, 2019 at 11:38:49AM +0200, Marco Felsch wrote:
> 
> ...
> 
> Let me sum up our irc discussion about that kAPI.
> 
> Our starting point is a fwnode based subdev which has connectors in
> front of there pins. Connectors are used to limit the subdev to some
> device limits e.g. if the device support only PAL-Input streams and the
> subdev has an buggy autodetect mechanism. In that case the connector can
> be used by the subdev to set the possible TV-Norms to PAL. Currently the
> tvp5150 is the only fwnode based subdev implementing connectors.
> 
> Connectors have common and connector specific properties. All current
> provided connectors can be found here:
> Documentation/devicetree/bindings/display/connector/ .
> 
> Parsing the properties is common to all _upcoming_ fwnode based subdevs
> so this should be done within the core. So lets move on to the parsing
> helper.
> 
> > > +int v4l2_fwnode_connector_alloc_parse(struct fwnode_handle *fwnode,
> > > +				      struct v4l2_fwnode_connector *connector)
> > > +{
> 
> This kAPI seems to fit all current use cases. The API is not responsible
> to alloc the 'struct v4l2_fwnode_connector' instead it is only used to
> fill this struct. The given 'struct fwnode_handle' should be the subdev
> local ep-fwnode because the user already has a reference to this ep.
> 
> This helper has two use-cases:
>   1) Parsing the connector properties and add the initial (1st) link.
>   2) Add further n-links upon n-calls to an already parsed connector.
> 
> Going this way we need to ensure that the caller init the 'struct
> v4l2_fwnode_connector' to '0' before call this helper. This can be
> documented within the kAPI doc.

The problem with the above is that although the API does not prevent
sharing connectors as such, it does not support it either: parsing a
connector on another sub-device ends up looking like a new connector,
independently of whether one (or more) entities representing the same
connector already exist.

Either way, I discussed this with Hans, and we concluded it's fine for now.
We've dealt with similar changes before, so when someone needs sharing
connectors, he'll need to implement it, also changing the kAPI drivers use.

Could you address the other comments I've given on the set?

-- 
Kind regards,

Sakari Ailus