Re: [PATCH v1 1/1] media: imx: csi: Parse link configuration from fw_node

[Sakari Ailus]

Hi Mathis,

Thanks for the patch.

On Wed, Mar 05, 2025 at 12:38:02PM +0100, Mathis Foerst wrote:
> The imx-media-csi driver requires upstream camera drivers to implement
> the subdev-pad-op "get_mbus_config" [0]. Camera drivers that don't
> implement this function are not usable on the i.MX6.
> 
> The docs for get_mbus_config [1] say:
> @get_mbus_config: get the media bus configuration of a remote sub-device.
>             The media bus configuration is usually retrieved from the
>             firmware interface at sub-device probe time, immediately
>             applied to the hardware and eventually adjusted by the
>             driver.
> 
> Currently, the imx-media-csi driver is not incorporating the information
> from the firmware interface and therefore relies on the implementation of
> get_mbus_config by the camera driver.
> 
> To be compatible with camera drivers not implementing get_mbus_config
> (which is the usual case), use the bus information from the fw interface:
> 
> The camera does not necessarily has a direct media bus link to the CSI as
> the video-mux and/or the MIPI CSI-2 receiver of the i.MX6 might be in
> between them on the media pipeline.
> The CSI driver already implements the functionality to find the connected
> camera sub-device to call get_mbus_config on it.
> 
> At this point the driver is modified as follows:
> In the case that get_mbus_config is not implemented by the upstream
> camera, try to get its endpoint configuration from the firmware interface
> usign v4l2_fwnode_endpoint_parse.
> For the supported mbus_types (V4L2_MBUS_PARALLEL, V4L2_MBUS_BT656 and
> V4L2_MBUS_CSI2_DPHY), extract the mbus_config from the endpoint
> configuration.
> For all other mbus_types, return an error.
> 
> Note that parsing the mbus_config from the fw interface is not done during
> probing because the camera that's connected to the CSI can change based on
> the selected input of the video-mux at runtime.
> 
> [0] drivers/staging/media/imx/imx-media-csi.c - line 211..216
> [1] include/media/v4l2-subdev.h - line 814
> 
> Signed-off-by: Mathis Foerst <mathis.foerst@xxxxxx>
> ---
>  drivers/staging/media/imx/imx-media-csi.c | 36 ++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index 3edbc57be2ca..394a9321a10b 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -169,6 +169,8 @@ static int csi_get_upstream_mbus_config(struct csi_priv *priv,
>  {
>  	struct v4l2_subdev *sd, *remote_sd;
>  	struct media_pad *remote_pad;
> +	struct fwnode_handle *ep_node;
> +	struct v4l2_fwnode_endpoint ep = { .bus_type = 0 };

Are there any defaults in DT bindings (other than 0's)? Also initialising a
field to zero this way is redundant, just use {}.

>  	int ret;
>  
>  	if (!priv->src_sd)
> @@ -210,11 +212,37 @@ static int csi_get_upstream_mbus_config(struct csi_priv *priv,
>  
>  	ret = v4l2_subdev_call(remote_sd, pad, get_mbus_config,
>  			       remote_pad->index, mbus_cfg);
> -	if (ret == -ENOIOCTLCMD)
> -		v4l2_err(&priv->sd,
> -			 "entity %s does not implement get_mbus_config()\n",
> -			 remote_pad->entity->name);
> +	if (ret == -ENOIOCTLCMD) {

	if (!ret)
		return 0;

And you can unindent the rest.

> +		/*
> +		 * If the upstream sd does not implement get_mbus_config,
> +		 * try to parse the link configuration from its fw_node
> +		 */
> +		ep_node = fwnode_graph_get_endpoint_by_id(dev_fwnode(remote_sd->dev),

Always parse only local, not remote endpoints.

Also: instead of supporting get_mbus_config() in a driver, we would ideally
have a helper that optionally uses it and secondarily gets the endpoint
configuration from a local endpoint. It's better to do that once rather
than have every driver do this differently, including a different set of
bugs.

That being said, V4L2 fwnode endpoint parsing is a somewhat driver specific
task and that should remain with the driver. So I'd think the helper should
take a driver-parsed struct v4l2_fwnode_endpoint as an argument, for that
endpoint.

I'm not saying no to this patch though. But in the long run we'll be better
off with a helper in the framework.

> +							  0, 0,
> +							  FWNODE_GRAPH_ENDPOINT_NEXT);
> +		if (!ep_node)
> +			return -ENOTCONN;
> +
> +		ret = v4l2_fwnode_endpoint_parse(ep_node, &ep);
> +		fwnode_handle_put(ep_node);
> +		if (ret)
> +			return ret;
>  
> +		mbus_cfg->type = ep.bus_type;
> +		switch (ep.bus_type) {
> +		case V4L2_MBUS_PARALLEL:
> +		case V4L2_MBUS_BT656:
> +			mbus_cfg->bus.parallel = ep.bus.parallel;
> +			break;
> +		case V4L2_MBUS_CSI2_DPHY:
> +			mbus_cfg->bus.mipi_csi2 = ep.bus.mipi_csi2;
> +			break;
> +		default:
> +			v4l2_err(&priv->sd, "Unsupported mbus_type: %i\n",
> +				 ep.bus_type);
> +			return -EINVAL;
> +		}
> +	}
>  	return ret;
>  }
>  

-- 
Kind regards,

Sakari Ailus