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

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

[Mathis Foerst]

Hi,

this patch fixes the imx-media-csi driver to work with camera drivers that
do not implement the subdev-pad-operation "get_mbus_format".

It's the follow up of this discussion:
https://lore.kernel.org/linux-media/Z8AoA6WjbXQufqR6@kekkonen.localdomain/

I tested the changes successfully on an i.MX6DL with an MT9M114 and an 
MT9V032 camera. They both use the parallel camera interface.
Sadly, I don't have the hardware to test with a MIPI CSI-2 camera.

Best regards,
Mathis Foerst


Mathis Foerst (1):
  media: imx: csi: Parse link configuration from fw_node

 drivers/staging/media/imx/imx-media-csi.c | 36 ++++++++++++++++++++---
 1 file changed, 32 insertions(+), 4 deletions(-)


base-commit: ac9c34d1e45a4c25174ced4fc0cfc33ff3ed08c7
-- 
2.34.1

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

[Mathis Foerst]

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@mt.com>
---
 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 };
 	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 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),
+							  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;
 }
 
-- 
2.34.1

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@mt.com>
> ---
>  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

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

[Dan Carpenter]

On Thu, Mar 06, 2025 at 04:33:18PM +0000, Sakari Ailus wrote:
> 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@mt.com>
> > ---
> >  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 {}.
> 

I was going to respond in much the same way.  This is equivalen to:

struct v4l2_fwnode_endpoint ep = { .bus_type = V4L2_MBUS_UNKNOWN };

> >  	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.

I was going to say this too but then I thought actually this needs to
be:

	if (ret != -ENOIOCTLCMD)
		return ret;

Which is weird.  Better to break all the new code into a separate
helper function.

	if (ret == -ENOIOCTLCMD)
		ret = parse_fw_link_config_stuff();

	return ret;

regards,
dan carpenter

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

[Sakari Ailus]

Hi Dan,

On Thu, Mar 06, 2025 at 10:07:20PM +0300, Dan Carpenter wrote:
> On Thu, Mar 06, 2025 at 04:33:18PM +0000, Sakari Ailus wrote:
> > 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@mt.com>
> > > ---
> > >  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 {}.
> > 
> 
> I was going to respond in much the same way.  This is equivalen to:
> 
> struct v4l2_fwnode_endpoint ep = { .bus_type = V4L2_MBUS_UNKNOWN };

Thinking about this in a context of parsing the endpoint, in fact the
bus_type should be specified. Presumably the hardware is D-PHY, so the
correct value would be V4L2_MBUS_CSI2_DPHY. This way
v4l2_fwnode_endpoint_parse() doesn't need to guess.

> 
> > >  	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.
> 
> I was going to say this too but then I thought actually this needs to
> be:
> 
> 	if (ret != -ENOIOCTLCMD)
> 		return ret;
> 
> Which is weird.  Better to break all the new code into a separate
> helper function.
> 
> 	if (ret == -ENOIOCTLCMD)
> 		ret = parse_fw_link_config_stuff();
> 
> 	return ret;

Indeed. get_mbus_config() presumably wouldn't return an error but
correctness is usually a good idea.

-- 
Regards,

Sakari Ailus

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

[Mathis Foerst]

Hi Sakari, Hi Dan,

thanks a lot for your feedback.

On Thu, Mar 06, 2025 at 09:13:46PM +0000, Sakari Ailus wrote:
> Hi Dan,
> 
> On Thu, Mar 06, 2025 at 10:07:20PM +0300, Dan Carpenter wrote:
> > On Thu, Mar 06, 2025 at 04:33:18PM +0000, Sakari Ailus wrote:
> > > 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@mt.com>
> > > > ---
> > > >  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 {}.
> > > 
> > 
> > I was going to respond in much the same way.  This is equivalen to:
> > 
> > struct v4l2_fwnode_endpoint ep = { .bus_type = V4L2_MBUS_UNKNOWN };
> 
> Thinking about this in a context of parsing the endpoint, in fact the
> bus_type should be specified. Presumably the hardware is D-PHY, so the
> correct value would be V4L2_MBUS_CSI2_DPHY. This way
> v4l2_fwnode_endpoint_parse() doesn't need to guess.

I think we must use "bus_type = V4L2_MBUS_UNKNOWN" here:

The i.MX6 has two types of camera interfaces: Parallel and MIPI CSI-2.
They are connected either directly or via a video-mux to the CSIs
(See IMX6DQRM.pdf - Figure 9-3 for the connection diagram)

Pre-defining V4L2_MBUS_CSI2_DPHY here would let
v4l2_fwnode_endpoint_parse() fail if the camera uses the parallel bus.

We could distinguish between MIPI CSI-2 and Parallel input by checking
the grp_id of the upstream device like it's already done in
csi_get_upstream_mbus_config().
But for the Parallel case we still can't know if we should set bus_type
to V4L2_MBUS_PARALLEL or to V4L2_MBUS_BT656 - the i.MX6 supports both
formats on the parallel interface.

That's why I would argue that v4l2_fwnode_endpoint_parse() must figure
out the bus_type from the fw node.

> 
> > 
> > > >  	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.
> > 
> > I was going to say this too but then I thought actually this needs to
> > be:
> > 
> > 	if (ret != -ENOIOCTLCMD)
> > 		return ret;
> > 
> > Which is weird.  Better to break all the new code into a separate
> > helper function.
> > 
> > 	if (ret == -ENOIOCTLCMD)
> > 		ret = parse_fw_link_config_stuff();
> > 
> > 	return ret;

Good point. I factored out a helper function as suggested.

> 
> Indeed. get_mbus_config() presumably wouldn't return an error but
> correctness is usually a good idea.
> 
> -- 
> Regards,
> 
> Sakari Ailus


Best regards,
Mathis Foerst

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

[Sakari Ailus]

Hi Mathis,

On Fri, Mar 07, 2025 at 02:38:29PM +0100, Mathis Foerst wrote:
> Hi Sakari, Hi Dan,
> 
> thanks a lot for your feedback.
> 
> On Thu, Mar 06, 2025 at 09:13:46PM +0000, Sakari Ailus wrote:
> > Hi Dan,
> > 
> > On Thu, Mar 06, 2025 at 10:07:20PM +0300, Dan Carpenter wrote:
> > > On Thu, Mar 06, 2025 at 04:33:18PM +0000, Sakari Ailus wrote:
> > > > 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@mt.com>
> > > > > ---
> > > > >  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 {}.
> > > > 
> > > 
> > > I was going to respond in much the same way.  This is equivalen to:
> > > 
> > > struct v4l2_fwnode_endpoint ep = { .bus_type = V4L2_MBUS_UNKNOWN };
> > 
> > Thinking about this in a context of parsing the endpoint, in fact the
> > bus_type should be specified. Presumably the hardware is D-PHY, so the
> > correct value would be V4L2_MBUS_CSI2_DPHY. This way
> > v4l2_fwnode_endpoint_parse() doesn't need to guess.
> 
> I think we must use "bus_type = V4L2_MBUS_UNKNOWN" here:
> 
> The i.MX6 has two types of camera interfaces: Parallel and MIPI CSI-2.
> They are connected either directly or via a video-mux to the CSIs
> (See IMX6DQRM.pdf - Figure 9-3 for the connection diagram)
> 
> Pre-defining V4L2_MBUS_CSI2_DPHY here would let
> v4l2_fwnode_endpoint_parse() fail if the camera uses the parallel bus.
> 
> We could distinguish between MIPI CSI-2 and Parallel input by checking
> the grp_id of the upstream device like it's already done in
> csi_get_upstream_mbus_config().
> But for the Parallel case we still can't know if we should set bus_type
> to V4L2_MBUS_PARALLEL or to V4L2_MBUS_BT656 - the i.MX6 supports both
> formats on the parallel interface.
> 
> That's why I would argue that v4l2_fwnode_endpoint_parse() must figure
> out the bus_type from the fw node.

Right, nowadays you can indeed do this -- it wasn't a long ago when you
couldn't. I presume the bindings do specify the bus-type property is
mandatory? Where are the bindings btw.?

> 
> > 
> > > 
> > > > >  	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.
> > > 
> > > I was going to say this too but then I thought actually this needs to
> > > be:
> > > 
> > > 	if (ret != -ENOIOCTLCMD)
> > > 		return ret;
> > > 
> > > Which is weird.  Better to break all the new code into a separate
> > > helper function.
> > > 
> > > 	if (ret == -ENOIOCTLCMD)
> > > 		ret = parse_fw_link_config_stuff();
> > > 
> > > 	return ret;
> 
> Good point. I factored out a helper function as suggested.
> 
> > 
> > Indeed. get_mbus_config() presumably wouldn't return an error but
> > correctness is usually a good idea.
> > 

-- 
Regards,

Sakari Ailus

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

[Mathis Foerst]

On Mon, Mar 10, 2025 at 10:25:04AM +0000, Sakari Ailus wrote:
Hi Sakari,

> Hi Mathis,
> 
> On Fri, Mar 07, 2025 at 02:38:29PM +0100, Mathis Foerst wrote:
> > Hi Sakari, Hi Dan,
> > 
> > thanks a lot for your feedback.
> > 
> > On Thu, Mar 06, 2025 at 09:13:46PM +0000, Sakari Ailus wrote:
> > > Hi Dan,
> > > 
> > > On Thu, Mar 06, 2025 at 10:07:20PM +0300, Dan Carpenter wrote:
> > > > On Thu, Mar 06, 2025 at 04:33:18PM +0000, Sakari Ailus wrote:
> > > > > 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@mt.com>
> > > > > > ---
> > > > > >  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 {}.
> > > > > 
> > > > 
> > > > I was going to respond in much the same way.  This is equivalen to:
> > > > 
> > > > struct v4l2_fwnode_endpoint ep = { .bus_type = V4L2_MBUS_UNKNOWN };
> > > 
> > > Thinking about this in a context of parsing the endpoint, in fact the
> > > bus_type should be specified. Presumably the hardware is D-PHY, so the
> > > correct value would be V4L2_MBUS_CSI2_DPHY. This way
> > > v4l2_fwnode_endpoint_parse() doesn't need to guess.
> > 
> > I think we must use "bus_type = V4L2_MBUS_UNKNOWN" here:
> > 
> > The i.MX6 has two types of camera interfaces: Parallel and MIPI CSI-2.
> > They are connected either directly or via a video-mux to the CSIs
> > (See IMX6DQRM.pdf - Figure 9-3 for the connection diagram)
> > 
> > Pre-defining V4L2_MBUS_CSI2_DPHY here would let
> > v4l2_fwnode_endpoint_parse() fail if the camera uses the parallel bus.
> > 
> > We could distinguish between MIPI CSI-2 and Parallel input by checking
> > the grp_id of the upstream device like it's already done in
> > csi_get_upstream_mbus_config().
> > But for the Parallel case we still can't know if we should set bus_type
> > to V4L2_MBUS_PARALLEL or to V4L2_MBUS_BT656 - the i.MX6 supports both
> > formats on the parallel interface.
> > 
> > That's why I would argue that v4l2_fwnode_endpoint_parse() must figure
> > out the bus_type from the fw node.
> 
> Right, nowadays you can indeed do this -- it wasn't a long ago when you
> couldn't. I presume the bindings do specify the bus-type property is
> mandatory? Where are the bindings btw.?
> 

From my understanding, it's not even required to set the bus-type as 
v4l2_fwnode_endpoint_parse() will try to parse the endpoint first as a
CSI-2 bus and in case of failure as a Parallel bus if the bus-type is
unknown (see drivers/media/v4l2-core/v4l2-fwnode.c#L493).

About the bindings:

There are bindings for the MIPI CSI-2 receiver:
Documentation/devicetree/bindings/media/imx.txt
I think here it's not necessary to make the bus-type property mandatory
as the imx6-mipi-csi2 driver enforces V4L2_MBUS_CSI2_DPHY anyhow
(see drivers/staging/media/imx/imx6-mipi-csi2.c#L677).

For the case of a camera with parallel bus, the camera endpoint is
connected to a video-mux and not directly to the CSI. Therefore, we cannot
make the bus-type property mandatory on this endpoint as it this wouldn't
apply to other use-cases of video-mux.

> > 
> > > 
> > > > 
> > > > > >  	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.
> > > > 
> > > > I was going to say this too but then I thought actually this needs to
> > > > be:
> > > > 
> > > > 	if (ret != -ENOIOCTLCMD)
> > > > 		return ret;
> > > > 
> > > > Which is weird.  Better to break all the new code into a separate
> > > > helper function.
> > > > 
> > > > 	if (ret == -ENOIOCTLCMD)
> > > > 		ret = parse_fw_link_config_stuff();
> > > > 
> > > > 	return ret;
> > 
> > Good point. I factored out a helper function as suggested.
> > 
> > > 
> > > Indeed. get_mbus_config() presumably wouldn't return an error but
> > > correctness is usually a good idea.
> > > 
> 
> -- 
> Regards,
> 
> Sakari Ailus

Best regards,
Mathis Foerst

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

[Sakari Ailus]

Hi Mathis,

On Fri, Mar 14, 2025 at 04:26:14PM +0100, Mathis Foerst wrote:
> On Mon, Mar 10, 2025 at 10:25:04AM +0000, Sakari Ailus wrote:
> Hi Sakari,
> 
> > Hi Mathis,
> > 
> > On Fri, Mar 07, 2025 at 02:38:29PM +0100, Mathis Foerst wrote:
> > > Hi Sakari, Hi Dan,
> > > 
> > > thanks a lot for your feedback.
> > > 
> > > On Thu, Mar 06, 2025 at 09:13:46PM +0000, Sakari Ailus wrote:
> > > > Hi Dan,
> > > > 
> > > > On Thu, Mar 06, 2025 at 10:07:20PM +0300, Dan Carpenter wrote:
> > > > > On Thu, Mar 06, 2025 at 04:33:18PM +0000, Sakari Ailus wrote:
> > > > > > 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@mt.com>
> > > > > > > ---
> > > > > > >  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 {}.
> > > > > > 
> > > > > 
> > > > > I was going to respond in much the same way.  This is equivalen to:
> > > > > 
> > > > > struct v4l2_fwnode_endpoint ep = { .bus_type = V4L2_MBUS_UNKNOWN };
> > > > 
> > > > Thinking about this in a context of parsing the endpoint, in fact the
> > > > bus_type should be specified. Presumably the hardware is D-PHY, so the
> > > > correct value would be V4L2_MBUS_CSI2_DPHY. This way
> > > > v4l2_fwnode_endpoint_parse() doesn't need to guess.
> > > 
> > > I think we must use "bus_type = V4L2_MBUS_UNKNOWN" here:
> > > 
> > > The i.MX6 has two types of camera interfaces: Parallel and MIPI CSI-2.
> > > They are connected either directly or via a video-mux to the CSIs
> > > (See IMX6DQRM.pdf - Figure 9-3 for the connection diagram)
> > > 
> > > Pre-defining V4L2_MBUS_CSI2_DPHY here would let
> > > v4l2_fwnode_endpoint_parse() fail if the camera uses the parallel bus.
> > > 
> > > We could distinguish between MIPI CSI-2 and Parallel input by checking
> > > the grp_id of the upstream device like it's already done in
> > > csi_get_upstream_mbus_config().
> > > But for the Parallel case we still can't know if we should set bus_type
> > > to V4L2_MBUS_PARALLEL or to V4L2_MBUS_BT656 - the i.MX6 supports both
> > > formats on the parallel interface.
> > > 
> > > That's why I would argue that v4l2_fwnode_endpoint_parse() must figure
> > > out the bus_type from the fw node.
> > 
> > Right, nowadays you can indeed do this -- it wasn't a long ago when you
> > couldn't. I presume the bindings do specify the bus-type property is
> > mandatory? Where are the bindings btw.?
> > 
> 
> From my understanding, it's not even required to set the bus-type as 
> v4l2_fwnode_endpoint_parse() will try to parse the endpoint first as a
> CSI-2 bus and in case of failure as a Parallel bus if the bus-type is
> unknown (see drivers/media/v4l2-core/v4l2-fwnode.c#L493).

It only exists for compatibility with old drivers. Do not add new users for
this code.

> 
> About the bindings:
> 
> There are bindings for the MIPI CSI-2 receiver:
> Documentation/devicetree/bindings/media/imx.txt
> I think here it's not necessary to make the bus-type property mandatory
> as the imx6-mipi-csi2 driver enforces V4L2_MBUS_CSI2_DPHY anyhow
> (see drivers/staging/media/imx/imx6-mipi-csi2.c#L677).

That seems to document a CSI-2 to parallel bridge, with support for four
virtual channels. I'd suppose you parse the ports knowing which interface
they use.

> 
> For the case of a camera with parallel bus, the camera endpoint is
> connected to a video-mux and not directly to the CSI. Therefore, we cannot
> make the bus-type property mandatory on this endpoint as it this wouldn't
> apply to other use-cases of video-mux.
> 
> > > 
> > > > 
> > > > > 
> > > > > > >  	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.
> > > > > 
> > > > > I was going to say this too but then I thought actually this needs to
> > > > > be:
> > > > > 
> > > > > 	if (ret != -ENOIOCTLCMD)
> > > > > 		return ret;
> > > > > 
> > > > > Which is weird.  Better to break all the new code into a separate
> > > > > helper function.
> > > > > 
> > > > > 	if (ret == -ENOIOCTLCMD)
> > > > > 		ret = parse_fw_link_config_stuff();
> > > > > 
> > > > > 	return ret;
> > > 
> > > Good point. I factored out a helper function as suggested.
> > > 
> > > > 
> > > > Indeed. get_mbus_config() presumably wouldn't return an error but
> > > > correctness is usually a good idea.
> > > > 
> > 

-- 
Regards,

Sakari Ailus

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

[Mathis Foerst]

Hi Sakari

On Fri, Mar 14, 2025 at 03:48:52PM +0000, Sakari Ailus wrote:
> Hi Mathis,
> 
> On Fri, Mar 14, 2025 at 04:26:14PM +0100, Mathis Foerst wrote:
> > On Mon, Mar 10, 2025 at 10:25:04AM +0000, Sakari Ailus wrote:
> > Hi Sakari,
> > 
> > > Hi Mathis,
> > > 
> > > On Fri, Mar 07, 2025 at 02:38:29PM +0100, Mathis Foerst wrote:
> > > > Hi Sakari, Hi Dan,
> > > > 
> > > > thanks a lot for your feedback.
> > > > 
> > > > On Thu, Mar 06, 2025 at 09:13:46PM +0000, Sakari Ailus wrote:
> > > > > Hi Dan,
> > > > > 
> > > > > On Thu, Mar 06, 2025 at 10:07:20PM +0300, Dan Carpenter wrote:
> > > > > > On Thu, Mar 06, 2025 at 04:33:18PM +0000, Sakari Ailus wrote:
> > > > > > > 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@mt.com>
> > > > > > > > ---
> > > > > > > >  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 {}.
> > > > > > > 
> > > > > > 
> > > > > > I was going to respond in much the same way.  This is equivalen to:
> > > > > > 
> > > > > > struct v4l2_fwnode_endpoint ep = { .bus_type = V4L2_MBUS_UNKNOWN };
> > > > > 
> > > > > Thinking about this in a context of parsing the endpoint, in fact the
> > > > > bus_type should be specified. Presumably the hardware is D-PHY, so the
> > > > > correct value would be V4L2_MBUS_CSI2_DPHY. This way
> > > > > v4l2_fwnode_endpoint_parse() doesn't need to guess.
> > > > 
> > > > I think we must use "bus_type = V4L2_MBUS_UNKNOWN" here:
> > > > 
> > > > The i.MX6 has two types of camera interfaces: Parallel and MIPI CSI-2.
> > > > They are connected either directly or via a video-mux to the CSIs
> > > > (See IMX6DQRM.pdf - Figure 9-3 for the connection diagram)
> > > > 
> > > > Pre-defining V4L2_MBUS_CSI2_DPHY here would let
> > > > v4l2_fwnode_endpoint_parse() fail if the camera uses the parallel bus.
> > > > 
> > > > We could distinguish between MIPI CSI-2 and Parallel input by checking
> > > > the grp_id of the upstream device like it's already done in
> > > > csi_get_upstream_mbus_config().
> > > > But for the Parallel case we still can't know if we should set bus_type
> > > > to V4L2_MBUS_PARALLEL or to V4L2_MBUS_BT656 - the i.MX6 supports both
> > > > formats on the parallel interface.
> > > > 
> > > > That's why I would argue that v4l2_fwnode_endpoint_parse() must figure
> > > > out the bus_type from the fw node.
> > > 
> > > Right, nowadays you can indeed do this -- it wasn't a long ago when you
> > > couldn't. I presume the bindings do specify the bus-type property is
> > > mandatory? Where are the bindings btw.?
> > > 
> > 
> > From my understanding, it's not even required to set the bus-type as 
> > v4l2_fwnode_endpoint_parse() will try to parse the endpoint first as a
> > CSI-2 bus and in case of failure as a Parallel bus if the bus-type is
> > unknown (see drivers/media/v4l2-core/v4l2-fwnode.c#L493).
> 
> It only exists for compatibility with old drivers. Do not add new users for
> this code.
> 
> > 
> > About the bindings:
> > 
> > There are bindings for the MIPI CSI-2 receiver:
> > Documentation/devicetree/bindings/media/imx.txt
> > I think here it's not necessary to make the bus-type property mandatory
> > as the imx6-mipi-csi2 driver enforces V4L2_MBUS_CSI2_DPHY anyhow
> > (see drivers/staging/media/imx/imx6-mipi-csi2.c#L677).
> 
> That seems to document a CSI-2 to parallel bridge, with support for four
> virtual channels. I'd suppose you parse the ports knowing which interface
> they use.

Yes, the video-mux has 5 input channels: 4 for the virtual MIPI channels
and one for the parallel port.
I now used this to determine the port type before parsing the endpoint.
If the parallel port is used, I must try to parse the endpoint as
V4L2_MBUS_PARALLEL and as V4L2_MBUS_BT656 because both formats are
possible on this port.

I submitted an updated version of the patch.

> 
> > 
> > For the case of a camera with parallel bus, the camera endpoint is
> > connected to a video-mux and not directly to the CSI. Therefore, we cannot
> > make the bus-type property mandatory on this endpoint as it this wouldn't
> > apply to other use-cases of video-mux.
> > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > > >  	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.
> > > > > > 
> > > > > > I was going to say this too but then I thought actually this needs to
> > > > > > be:
> > > > > > 
> > > > > > 	if (ret != -ENOIOCTLCMD)
> > > > > > 		return ret;
> > > > > > 
> > > > > > Which is weird.  Better to break all the new code into a separate
> > > > > > helper function.
> > > > > > 
> > > > > > 	if (ret == -ENOIOCTLCMD)
> > > > > > 		ret = parse_fw_link_config_stuff();
> > > > > > 
> > > > > > 	return ret;
> > > > 
> > > > Good point. I factored out a helper function as suggested.
> > > > 
> > > > > 
> > > > > Indeed. get_mbus_config() presumably wouldn't return an error but
> > > > > correctness is usually a good idea.
> > > > > 
> > > 
> 
> -- 
> Regards,
> 
> Sakari Ailus

Best regards,
Mathis Foerst