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
