Although 4 lanes may be physically available, we may not be using all of
them. Get the number of configured lanes in the case a driver has
implemented the get_mbus_config op.

Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>

---

Currently, the imx-mipi-csis driver parses the device tree to determine
the number of configured lanes for the CSI receiver. This may be
incorrect in the case that the connected device only uses a subset of
lanes, for example. Allow the drivers for these cameras to create a
mbus_config to configure the number of lanes that are actually being
used.

If the driver does not support the get_mbus_config op, this patch will
have no functional change.

Compile tested against media-master (v6.17-rc1)
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 2beb5f43c2c0..efe4e2ad0382 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -939,6 +939,43 @@ static struct mipi_csis_device *sd_to_mipi_csis_device(struct v4l2_subdev *sdev)
 	return container_of(sdev, struct mipi_csis_device, sd);
 }
 
+static int mipi_csis_get_active_lanes(struct v4l2_subdev *sd)
+{
+	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
+	struct v4l2_mbus_config mbus_config = { 0 };
+	int ret;
+
+	ret = v4l2_subdev_call(csis->source.sd, pad, get_mbus_config,
+			       0, &mbus_config);
+	if (ret == -ENOIOCTLCMD) {
+		dev_dbg(csis->dev, "No remote mbus configuration available\n");
+		return 0;
+	}
+
+	if (ret) {
+		dev_err(csis->dev, "Failed to get remote mbus configuration\n");
+		return ret;
+	}
+
+	if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
+		dev_err(csis->dev, "Unsupported media bus type %u\n",
+			mbus_config.type);
+		return -EINVAL;
+	}
+
+	if (mbus_config.bus.mipi_csi2.num_data_lanes > csis->bus.num_data_lanes) {
+		dev_err(csis->dev,
+			"Unsupported mbus config: too many data lanes %u\n",
+			mbus_config.bus.mipi_csi2.num_data_lanes);
+		return -EINVAL;
+	}
+
+	csis->bus.num_data_lanes = mbus_config.bus.mipi_csi2.num_data_lanes;
+	dev_dbg(csis->dev, "Number of lanes: %d\n", csis->bus.num_data_lanes);
+
+	return 0;
+}
+
 static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
@@ -965,6 +1002,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
 	format = v4l2_subdev_state_get_format(state, CSIS_PAD_SINK);
 	csis_fmt = find_csis_format(format->code);
 
+	ret = mipi_csis_get_active_lanes(sd);
+	if (ret < 0)
+		dev_dbg(csis->dev, "Failed to get active lanes: %d", ret);
+
 	ret = mipi_csis_calculate_params(csis, csis_fmt);
 	if (ret < 0)
 		goto err_unlock;
-- 
2.43.0

Hi Isaac,

Thanks for the patch.

On Thu, Aug 14, 2025 at 12:37:01PM +0100, Isaac Scott wrote:
> Although 4 lanes may be physically available, we may not be using all of
> them. Get the number of configured lanes in the case a driver has
> implemented the get_mbus_config op.
> 
> Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
> 
> ---
> 
> Currently, the imx-mipi-csis driver parses the device tree to determine
> the number of configured lanes for the CSI receiver. This may be
> incorrect in the case that the connected device only uses a subset of
> lanes, for example. Allow the drivers for these cameras to create a
> mbus_config to configure the number of lanes that are actually being
> used.
> 
> If the driver does not support the get_mbus_config op, this patch will
> have no functional change.
> 
> Compile tested against media-master (v6.17-rc1)
> ---
>  drivers/media/platform/nxp/imx-mipi-csis.c | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index 2beb5f43c2c0..efe4e2ad0382 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -939,6 +939,43 @@ static struct mipi_csis_device *sd_to_mipi_csis_device(struct v4l2_subdev *sdev)
>  	return container_of(sdev, struct mipi_csis_device, sd);
>  }
>  
> +static int mipi_csis_get_active_lanes(struct v4l2_subdev *sd)
> +{
> +	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> +	struct v4l2_mbus_config mbus_config = { 0 };
> +	int ret;
> +
> +	ret = v4l2_subdev_call(csis->source.sd, pad, get_mbus_config,
> +			       0, &mbus_config);
> +	if (ret == -ENOIOCTLCMD) {
> +		dev_dbg(csis->dev, "No remote mbus configuration available\n");
> +		return 0;
> +	}
> +
> +	if (ret) {
> +		dev_err(csis->dev, "Failed to get remote mbus configuration\n");
> +		return ret;
> +	}
> +
> +	if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
> +		dev_err(csis->dev, "Unsupported media bus type %u\n",
> +			mbus_config.type);
> +		return -EINVAL;
> +	}
> +
> +	if (mbus_config.bus.mipi_csi2.num_data_lanes > csis->bus.num_data_lanes) {
> +		dev_err(csis->dev,
> +			"Unsupported mbus config: too many data lanes %u\n",
> +			mbus_config.bus.mipi_csi2.num_data_lanes);
> +		return -EINVAL;
> +	}
> +
> +	csis->bus.num_data_lanes = mbus_config.bus.mipi_csi2.num_data_lanes;
> +	dev_dbg(csis->dev, "Number of lanes: %d\n", csis->bus.num_data_lanes);

None of the above is really specific to this driver. Could you instead
implement a function that parses the information from the fwnode endpoint
and uses mbus configuration on top?

The function could take struct media_pad pointer as an argument, or struct
v4l2_subdev pointer and the pad number.

I wonder if any other parameters could change dynamically but I can't think
of that now, so perhaps just the number of lanes is what the function
should indeed return.

> +
> +	return 0;
> +}
> +
>  static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> @@ -965,6 +1002,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
>  	format = v4l2_subdev_state_get_format(state, CSIS_PAD_SINK);
>  	csis_fmt = find_csis_format(format->code);
>  
> +	ret = mipi_csis_get_active_lanes(sd);
> +	if (ret < 0)
> +		dev_dbg(csis->dev, "Failed to get active lanes: %d", ret);
> +
>  	ret = mipi_csis_calculate_params(csis, csis_fmt);
>  	if (ret < 0)
>  		goto err_unlock;

-- 
Kind regards,

Sakari Ailus

On Fri, Aug 15, 2025 at 09:18:13AM +0000, Sakari Ailus wrote:
> On Thu, Aug 14, 2025 at 12:37:01PM +0100, Isaac Scott wrote:
> > Although 4 lanes may be physically available, we may not be using all of
> > them. Get the number of configured lanes in the case a driver has
> > implemented the get_mbus_config op.
> > 
> > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
> > 
> > ---
> > 
> > Currently, the imx-mipi-csis driver parses the device tree to determine
> > the number of configured lanes for the CSI receiver. This may be
> > incorrect in the case that the connected device only uses a subset of
> > lanes, for example. Allow the drivers for these cameras to create a
> > mbus_config to configure the number of lanes that are actually being
> > used.
> > 
> > If the driver does not support the get_mbus_config op, this patch will
> > have no functional change.
> > 
> > Compile tested against media-master (v6.17-rc1)
> > ---
> >  drivers/media/platform/nxp/imx-mipi-csis.c | 41 ++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> > index 2beb5f43c2c0..efe4e2ad0382 100644
> > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > @@ -939,6 +939,43 @@ static struct mipi_csis_device *sd_to_mipi_csis_device(struct v4l2_subdev *sdev)
> >  	return container_of(sdev, struct mipi_csis_device, sd);
> >  }
> >  
> > +static int mipi_csis_get_active_lanes(struct v4l2_subdev *sd)
> > +{
> > +	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > +	struct v4l2_mbus_config mbus_config = { 0 };
> > +	int ret;
> > +
> > +	ret = v4l2_subdev_call(csis->source.sd, pad, get_mbus_config,
> > +			       0, &mbus_config);
> > +	if (ret == -ENOIOCTLCMD) {
> > +		dev_dbg(csis->dev, "No remote mbus configuration available\n");
> > +		return 0;
> > +	}
> > +
> > +	if (ret) {
> > +		dev_err(csis->dev, "Failed to get remote mbus configuration\n");
> > +		return ret;
> > +	}
> > +
> > +	if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
> > +		dev_err(csis->dev, "Unsupported media bus type %u\n",
> > +			mbus_config.type);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (mbus_config.bus.mipi_csi2.num_data_lanes > csis->bus.num_data_lanes) {
> > +		dev_err(csis->dev,
> > +			"Unsupported mbus config: too many data lanes %u\n",
> > +			mbus_config.bus.mipi_csi2.num_data_lanes);
> > +		return -EINVAL;
> > +	}
> > +
> > +	csis->bus.num_data_lanes = mbus_config.bus.mipi_csi2.num_data_lanes;

There's a bug here, you override the number of lanes retrieved from DT,
which is the number of connected lanes, with the number of lanes used by
the source for its particular configuration. You will never be able to
then use more lanes in a different source configuration.

> > +	dev_dbg(csis->dev, "Number of lanes: %d\n", csis->bus.num_data_lanes);
> 
> None of the above is really specific to this driver. Could you instead
> implement a function that parses the information from the fwnode endpoint
> and uses mbus configuration on top?

That would need to parse the endpoint every time we start streaming, it
doesn't sound ideal.

> The function could take struct media_pad pointer as an argument, or struct
> v4l2_subdev pointer and the pad number.
> 
> I wonder if any other parameters could change dynamically but I can't think
> of that now, so perhaps just the number of lanes is what the function
> should indeed return.
> 
> > +
> > +	return 0;
> > +}
> > +
> >  static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> >  {
> >  	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > @@ -965,6 +1002,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> >  	format = v4l2_subdev_state_get_format(state, CSIS_PAD_SINK);
> >  	csis_fmt = find_csis_format(format->code);
> >  
> > +	ret = mipi_csis_get_active_lanes(sd);
> > +	if (ret < 0)
> > +		dev_dbg(csis->dev, "Failed to get active lanes: %d", ret);
> > +
> >  	ret = mipi_csis_calculate_params(csis, csis_fmt);
> >  	if (ret < 0)
> >  		goto err_unlock;

-- 
Regards,

Laurent Pinchart

Hi Laurent,

On Fri, Aug 15, 2025 at 01:32:05PM +0300, Laurent Pinchart wrote:
> On Fri, Aug 15, 2025 at 09:18:13AM +0000, Sakari Ailus wrote:
> > On Thu, Aug 14, 2025 at 12:37:01PM +0100, Isaac Scott wrote:
> > > Although 4 lanes may be physically available, we may not be using all of
> > > them. Get the number of configured lanes in the case a driver has
> > > implemented the get_mbus_config op.
> > > 
> > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
> > > 
> > > ---
> > > 
> > > Currently, the imx-mipi-csis driver parses the device tree to determine
> > > the number of configured lanes for the CSI receiver. This may be
> > > incorrect in the case that the connected device only uses a subset of
> > > lanes, for example. Allow the drivers for these cameras to create a
> > > mbus_config to configure the number of lanes that are actually being
> > > used.
> > > 
> > > If the driver does not support the get_mbus_config op, this patch will
> > > have no functional change.
> > > 
> > > Compile tested against media-master (v6.17-rc1)
> > > ---
> > >  drivers/media/platform/nxp/imx-mipi-csis.c | 41 ++++++++++++++++++++++
> > >  1 file changed, 41 insertions(+)
> > > 
> > > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> > > index 2beb5f43c2c0..efe4e2ad0382 100644
> > > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > > @@ -939,6 +939,43 @@ static struct mipi_csis_device *sd_to_mipi_csis_device(struct v4l2_subdev *sdev)
> > >  	return container_of(sdev, struct mipi_csis_device, sd);
> > >  }
> > >  
> > > +static int mipi_csis_get_active_lanes(struct v4l2_subdev *sd)
> > > +{
> > > +	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > > +	struct v4l2_mbus_config mbus_config = { 0 };
> > > +	int ret;
> > > +
> > > +	ret = v4l2_subdev_call(csis->source.sd, pad, get_mbus_config,
> > > +			       0, &mbus_config);
> > > +	if (ret == -ENOIOCTLCMD) {
> > > +		dev_dbg(csis->dev, "No remote mbus configuration available\n");
> > > +		return 0;
> > > +	}
> > > +
> > > +	if (ret) {
> > > +		dev_err(csis->dev, "Failed to get remote mbus configuration\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
> > > +		dev_err(csis->dev, "Unsupported media bus type %u\n",
> > > +			mbus_config.type);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (mbus_config.bus.mipi_csi2.num_data_lanes > csis->bus.num_data_lanes) {
> > > +		dev_err(csis->dev,
> > > +			"Unsupported mbus config: too many data lanes %u\n",
> > > +			mbus_config.bus.mipi_csi2.num_data_lanes);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	csis->bus.num_data_lanes = mbus_config.bus.mipi_csi2.num_data_lanes;
> 
> There's a bug here, you override the number of lanes retrieved from DT,
> which is the number of connected lanes, with the number of lanes used by
> the source for its particular configuration. You will never be able to
> then use more lanes in a different source configuration.
> 
> > > +	dev_dbg(csis->dev, "Number of lanes: %d\n", csis->bus.num_data_lanes);
> > 
> > None of the above is really specific to this driver. Could you instead
> > implement a function that parses the information from the fwnode endpoint
> > and uses mbus configuration on top?
> 
> That would need to parse the endpoint every time we start streaming, it
> doesn't sound ideal.

Perhaps not, but does that matter in practice? Parsing the endpoint is,
after all, fairly trivial. The advantage would be simplifying drivers.

Alternatively we could think of caching this information somewhere but I
don't think it's worth it.

> 
> > The function could take struct media_pad pointer as an argument, or struct
> > v4l2_subdev pointer and the pad number.
> > 
> > I wonder if any other parameters could change dynamically but I can't think
> > of that now, so perhaps just the number of lanes is what the function
> > should indeed return.
> > 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > >  {
> > >  	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > > @@ -965,6 +1002,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > >  	format = v4l2_subdev_state_get_format(state, CSIS_PAD_SINK);
> > >  	csis_fmt = find_csis_format(format->code);
> > >  
> > > +	ret = mipi_csis_get_active_lanes(sd);
> > > +	if (ret < 0)
> > > +		dev_dbg(csis->dev, "Failed to get active lanes: %d", ret);
> > > +
> > >  	ret = mipi_csis_calculate_params(csis, csis_fmt);
> > >  	if (ret < 0)
> > >  		goto err_unlock;
> 

-- 
Regards,

Sakari Ailus

On Fri, Aug 15, 2025 at 11:25:24AM +0000, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Fri, Aug 15, 2025 at 01:32:05PM +0300, Laurent Pinchart wrote:
> > On Fri, Aug 15, 2025 at 09:18:13AM +0000, Sakari Ailus wrote:
> > > On Thu, Aug 14, 2025 at 12:37:01PM +0100, Isaac Scott wrote:
> > > > Although 4 lanes may be physically available, we may not be using all of
> > > > them. Get the number of configured lanes in the case a driver has
> > > > implemented the get_mbus_config op.
> > > > 
> > > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
> > > > 
> > > > ---
> > > > 
> > > > Currently, the imx-mipi-csis driver parses the device tree to determine
> > > > the number of configured lanes for the CSI receiver. This may be
> > > > incorrect in the case that the connected device only uses a subset of
> > > > lanes, for example. Allow the drivers for these cameras to create a
> > > > mbus_config to configure the number of lanes that are actually being
> > > > used.
> > > > 
> > > > If the driver does not support the get_mbus_config op, this patch will
> > > > have no functional change.
> > > > 
> > > > Compile tested against media-master (v6.17-rc1)
> > > > ---
> > > >  drivers/media/platform/nxp/imx-mipi-csis.c | 41 ++++++++++++++++++++++
> > > >  1 file changed, 41 insertions(+)
> > > > 
> > > > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> > > > index 2beb5f43c2c0..efe4e2ad0382 100644
> > > > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > > > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > > > @@ -939,6 +939,43 @@ static struct mipi_csis_device *sd_to_mipi_csis_device(struct v4l2_subdev *sdev)
> > > >  	return container_of(sdev, struct mipi_csis_device, sd);
> > > >  }
> > > >  
> > > > +static int mipi_csis_get_active_lanes(struct v4l2_subdev *sd)
> > > > +{
> > > > +	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > > > +	struct v4l2_mbus_config mbus_config = { 0 };
> > > > +	int ret;
> > > > +
> > > > +	ret = v4l2_subdev_call(csis->source.sd, pad, get_mbus_config,
> > > > +			       0, &mbus_config);
> > > > +	if (ret == -ENOIOCTLCMD) {
> > > > +		dev_dbg(csis->dev, "No remote mbus configuration available\n");
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > +	if (ret) {
> > > > +		dev_err(csis->dev, "Failed to get remote mbus configuration\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
> > > > +		dev_err(csis->dev, "Unsupported media bus type %u\n",
> > > > +			mbus_config.type);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	if (mbus_config.bus.mipi_csi2.num_data_lanes > csis->bus.num_data_lanes) {
> > > > +		dev_err(csis->dev,
> > > > +			"Unsupported mbus config: too many data lanes %u\n",
> > > > +			mbus_config.bus.mipi_csi2.num_data_lanes);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	csis->bus.num_data_lanes = mbus_config.bus.mipi_csi2.num_data_lanes;
> > 
> > There's a bug here, you override the number of lanes retrieved from DT,
> > which is the number of connected lanes, with the number of lanes used by
> > the source for its particular configuration. You will never be able to
> > then use more lanes in a different source configuration.
> > 
> > > > +	dev_dbg(csis->dev, "Number of lanes: %d\n", csis->bus.num_data_lanes);
> > > 
> > > None of the above is really specific to this driver. Could you instead
> > > implement a function that parses the information from the fwnode endpoint
> > > and uses mbus configuration on top?
> > 
> > That would need to parse the endpoint every time we start streaming, it
> > doesn't sound ideal.
> 
> Perhaps not, but does that matter in practice? Parsing the endpoint is,
> after all, fairly trivial. The advantage would be simplifying drivers.

It's trivial from a code point of view, but it's not a cheap operation.
I'd like to avoid making starting streaming more expensive.

> Alternatively we could think of caching this information somewhere but I
> don't think it's worth it.

Drivers likely need to parse endpoints for other reasons. I'd cache the
value in drivers, like done today, and pass it to a get_active_lanes
helper.

> > > The function could take struct media_pad pointer as an argument, or struct
> > > v4l2_subdev pointer and the pad number.
> > > 
> > > I wonder if any other parameters could change dynamically but I can't think
> > > of that now, so perhaps just the number of lanes is what the function
> > > should indeed return.
> > > 
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > > >  {
> > > >  	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > > > @@ -965,6 +1002,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > > >  	format = v4l2_subdev_state_get_format(state, CSIS_PAD_SINK);
> > > >  	csis_fmt = find_csis_format(format->code);
> > > >  
> > > > +	ret = mipi_csis_get_active_lanes(sd);
> > > > +	if (ret < 0)
> > > > +		dev_dbg(csis->dev, "Failed to get active lanes: %d", ret);
> > > > +
> > > >  	ret = mipi_csis_calculate_params(csis, csis_fmt);
> > > >  	if (ret < 0)
> > > >  		goto err_unlock;

-- 
Regards,

Laurent Pinchart

Hi Laurent,

On Fri, Aug 15, 2025 at 02:36:33PM +0300, Laurent Pinchart wrote:
> On Fri, Aug 15, 2025 at 11:25:24AM +0000, Sakari Ailus wrote:
> > Hi Laurent,
> > 
> > On Fri, Aug 15, 2025 at 01:32:05PM +0300, Laurent Pinchart wrote:
> > > On Fri, Aug 15, 2025 at 09:18:13AM +0000, Sakari Ailus wrote:
> > > > On Thu, Aug 14, 2025 at 12:37:01PM +0100, Isaac Scott wrote:
> > > > > Although 4 lanes may be physically available, we may not be using all of
> > > > > them. Get the number of configured lanes in the case a driver has
> > > > > implemented the get_mbus_config op.
> > > > > 
> > > > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
> > > > > 
> > > > > ---
> > > > > 
> > > > > Currently, the imx-mipi-csis driver parses the device tree to determine
> > > > > the number of configured lanes for the CSI receiver. This may be
> > > > > incorrect in the case that the connected device only uses a subset of
> > > > > lanes, for example. Allow the drivers for these cameras to create a
> > > > > mbus_config to configure the number of lanes that are actually being
> > > > > used.
> > > > > 
> > > > > If the driver does not support the get_mbus_config op, this patch will
> > > > > have no functional change.
> > > > > 
> > > > > Compile tested against media-master (v6.17-rc1)
> > > > > ---
> > > > >  drivers/media/platform/nxp/imx-mipi-csis.c | 41 ++++++++++++++++++++++
> > > > >  1 file changed, 41 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> > > > > index 2beb5f43c2c0..efe4e2ad0382 100644
> > > > > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > > > > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > > > > @@ -939,6 +939,43 @@ static struct mipi_csis_device *sd_to_mipi_csis_device(struct v4l2_subdev *sdev)
> > > > >  	return container_of(sdev, struct mipi_csis_device, sd);
> > > > >  }
> > > > >  
> > > > > +static int mipi_csis_get_active_lanes(struct v4l2_subdev *sd)
> > > > > +{
> > > > > +	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > > > > +	struct v4l2_mbus_config mbus_config = { 0 };
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = v4l2_subdev_call(csis->source.sd, pad, get_mbus_config,
> > > > > +			       0, &mbus_config);
> > > > > +	if (ret == -ENOIOCTLCMD) {
> > > > > +		dev_dbg(csis->dev, "No remote mbus configuration available\n");
> > > > > +		return 0;
> > > > > +	}
> > > > > +
> > > > > +	if (ret) {
> > > > > +		dev_err(csis->dev, "Failed to get remote mbus configuration\n");
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
> > > > > +		dev_err(csis->dev, "Unsupported media bus type %u\n",
> > > > > +			mbus_config.type);
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	if (mbus_config.bus.mipi_csi2.num_data_lanes > csis->bus.num_data_lanes) {
> > > > > +		dev_err(csis->dev,
> > > > > +			"Unsupported mbus config: too many data lanes %u\n",
> > > > > +			mbus_config.bus.mipi_csi2.num_data_lanes);
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	csis->bus.num_data_lanes = mbus_config.bus.mipi_csi2.num_data_lanes;
> > > 
> > > There's a bug here, you override the number of lanes retrieved from DT,
> > > which is the number of connected lanes, with the number of lanes used by
> > > the source for its particular configuration. You will never be able to
> > > then use more lanes in a different source configuration.
> > > 
> > > > > +	dev_dbg(csis->dev, "Number of lanes: %d\n", csis->bus.num_data_lanes);
> > > > 
> > > > None of the above is really specific to this driver. Could you instead
> > > > implement a function that parses the information from the fwnode endpoint
> > > > and uses mbus configuration on top?
> > > 
> > > That would need to parse the endpoint every time we start streaming, it
> > > doesn't sound ideal.
> > 
> > Perhaps not, but does that matter in practice? Parsing the endpoint is,
> > after all, fairly trivial. The advantage would be simplifying drivers.
> 
> It's trivial from a code point of view, but it's not a cheap operation.
> I'd like to avoid making starting streaming more expensive.

How cheap is "not cheap"? I'd be surprised if parsing an endpoint took more
time than e.g. an I²C register write. Of course it depends on the CPU...

> 
> > Alternatively we could think of caching this information somewhere but I
> > don't think it's worth it.
> 
> Drivers likely need to parse endpoints for other reasons. I'd cache the
> value in drivers, like done today, and pass it to a get_active_lanes
> helper.

Then drivers presumably would also validate this against the endpoint
configuration, wouldn't they? That's extra code in every CSI-2 receiver
driver.

> 
> > > > The function could take struct media_pad pointer as an argument, or struct
> > > > v4l2_subdev pointer and the pad number.
> > > > 
> > > > I wonder if any other parameters could change dynamically but I can't think
> > > > of that now, so perhaps just the number of lanes is what the function
> > > > should indeed return.
> > > > 
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > > > >  {
> > > > >  	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > > > > @@ -965,6 +1002,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > > > >  	format = v4l2_subdev_state_get_format(state, CSIS_PAD_SINK);
> > > > >  	csis_fmt = find_csis_format(format->code);
> > > > >  
> > > > > +	ret = mipi_csis_get_active_lanes(sd);
> > > > > +	if (ret < 0)
> > > > > +		dev_dbg(csis->dev, "Failed to get active lanes: %d", ret);
> > > > > +
> > > > >  	ret = mipi_csis_calculate_params(csis, csis_fmt);
> > > > >  	if (ret < 0)
> > > > >  		goto err_unlock;
> 

-- 
Regards,

Sakari Ailus

On Fri, Aug 15, 2025 at 12:33:46PM +0000, Sakari Ailus wrote:
> On Fri, Aug 15, 2025 at 02:36:33PM +0300, Laurent Pinchart wrote:
> > On Fri, Aug 15, 2025 at 11:25:24AM +0000, Sakari Ailus wrote:
> > > On Fri, Aug 15, 2025 at 01:32:05PM +0300, Laurent Pinchart wrote:
> > > > On Fri, Aug 15, 2025 at 09:18:13AM +0000, Sakari Ailus wrote:
> > > > > On Thu, Aug 14, 2025 at 12:37:01PM +0100, Isaac Scott wrote:
> > > > > > Although 4 lanes may be physically available, we may not be using all of
> > > > > > them. Get the number of configured lanes in the case a driver has
> > > > > > implemented the get_mbus_config op.
> > > > > > 
> > > > > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
> > > > > > 
> > > > > > ---
> > > > > > 
> > > > > > Currently, the imx-mipi-csis driver parses the device tree to determine
> > > > > > the number of configured lanes for the CSI receiver. This may be
> > > > > > incorrect in the case that the connected device only uses a subset of
> > > > > > lanes, for example. Allow the drivers for these cameras to create a
> > > > > > mbus_config to configure the number of lanes that are actually being
> > > > > > used.
> > > > > > 
> > > > > > If the driver does not support the get_mbus_config op, this patch will
> > > > > > have no functional change.
> > > > > > 
> > > > > > Compile tested against media-master (v6.17-rc1)
> > > > > > ---
> > > > > >  drivers/media/platform/nxp/imx-mipi-csis.c | 41 ++++++++++++++++++++++
> > > > > >  1 file changed, 41 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> > > > > > index 2beb5f43c2c0..efe4e2ad0382 100644
> > > > > > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > > > > > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > > > > > @@ -939,6 +939,43 @@ static struct mipi_csis_device *sd_to_mipi_csis_device(struct v4l2_subdev *sdev)
> > > > > >  	return container_of(sdev, struct mipi_csis_device, sd);
> > > > > >  }
> > > > > >  
> > > > > > +static int mipi_csis_get_active_lanes(struct v4l2_subdev *sd)
> > > > > > +{
> > > > > > +	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > > > > > +	struct v4l2_mbus_config mbus_config = { 0 };
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	ret = v4l2_subdev_call(csis->source.sd, pad, get_mbus_config,
> > > > > > +			       0, &mbus_config);
> > > > > > +	if (ret == -ENOIOCTLCMD) {
> > > > > > +		dev_dbg(csis->dev, "No remote mbus configuration available\n");
> > > > > > +		return 0;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (ret) {
> > > > > > +		dev_err(csis->dev, "Failed to get remote mbus configuration\n");
> > > > > > +		return ret;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
> > > > > > +		dev_err(csis->dev, "Unsupported media bus type %u\n",
> > > > > > +			mbus_config.type);
> > > > > > +		return -EINVAL;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (mbus_config.bus.mipi_csi2.num_data_lanes > csis->bus.num_data_lanes) {
> > > > > > +		dev_err(csis->dev,
> > > > > > +			"Unsupported mbus config: too many data lanes %u\n",
> > > > > > +			mbus_config.bus.mipi_csi2.num_data_lanes);
> > > > > > +		return -EINVAL;
> > > > > > +	}
> > > > > > +
> > > > > > +	csis->bus.num_data_lanes = mbus_config.bus.mipi_csi2.num_data_lanes;
> > > > 
> > > > There's a bug here, you override the number of lanes retrieved from DT,
> > > > which is the number of connected lanes, with the number of lanes used by
> > > > the source for its particular configuration. You will never be able to
> > > > then use more lanes in a different source configuration.
> > > > 
> > > > > > +	dev_dbg(csis->dev, "Number of lanes: %d\n", csis->bus.num_data_lanes);
> > > > > 
> > > > > None of the above is really specific to this driver. Could you instead
> > > > > implement a function that parses the information from the fwnode endpoint
> > > > > and uses mbus configuration on top?
> > > > 
> > > > That would need to parse the endpoint every time we start streaming, it
> > > > doesn't sound ideal.
> > > 
> > > Perhaps not, but does that matter in practice? Parsing the endpoint is,
> > > after all, fairly trivial. The advantage would be simplifying drivers.
> > 
> > It's trivial from a code point of view, but it's not a cheap operation.
> > I'd like to avoid making starting streaming more expensive.
> 
> How cheap is "not cheap"? I'd be surprised if parsing an endpoint took more
> time than e.g. an I²C register write. Of course it depends on the CPU...

Still, it's not cheap, and I think it can easily be avoided.

> > > Alternatively we could think of caching this information somewhere but I
> > > don't think it's worth it.
> > 
> > Drivers likely need to parse endpoints for other reasons. I'd cache the
> > value in drivers, like done today, and pass it to a get_active_lanes
> > helper.
> 
> Then drivers presumably would also validate this against the endpoint
> configuration, wouldn't they? That's extra code in every CSI-2 receiver
> driver.

Why so ? The number of connected lanes can be passed to the helper
function, which can use it to validate the number of lanes reported by
the source subdev.

> > > > > The function could take struct media_pad pointer as an argument, or struct
> > > > > v4l2_subdev pointer and the pad number.
> > > > > 
> > > > > I wonder if any other parameters could change dynamically but I can't think
> > > > > of that now, so perhaps just the number of lanes is what the function
> > > > > should indeed return.
> > > > > 
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > >  static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > > > > >  {
> > > > > >  	struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > > > > > @@ -965,6 +1002,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > > > > >  	format = v4l2_subdev_state_get_format(state, CSIS_PAD_SINK);
> > > > > >  	csis_fmt = find_csis_format(format->code);
> > > > > >  
> > > > > > +	ret = mipi_csis_get_active_lanes(sd);
> > > > > > +	if (ret < 0)
> > > > > > +		dev_dbg(csis->dev, "Failed to get active lanes: %d", ret);
> > > > > > +
> > > > > >  	ret = mipi_csis_calculate_params(csis, csis_fmt);
> > > > > >  	if (ret < 0)
> > > > > >  		goto err_unlock;

-- 
Regards,

Laurent Pinchart

Hi All,

Quoting Laurent Pinchart (2025-08-19 03:44:13)
<snip>
> > > > > That would need to parse the endpoint every time we start streaming, it
> > > > > doesn't sound ideal.
> > > > 
> > > > Perhaps not, but does that matter in practice? Parsing the endpoint is,
> > > > after all, fairly trivial. The advantage would be simplifying drivers.
> > > 
> > > It's trivial from a code point of view, but it's not a cheap operation.
> > > I'd like to avoid making starting streaming more expensive.
> > 
> > How cheap is "not cheap"? I'd be surprised if parsing an endpoint took more
> > time than e.g. an I²C register write. Of course it depends on the CPU...
> 
> Still, it's not cheap, and I think it can easily be avoided.
> 
> > > > Alternatively we could think of caching this information somewhere but I
> > > > don't think it's worth it.
> > > 
> > > Drivers likely need to parse endpoints for other reasons. I'd cache the
> > > value in drivers, like done today, and pass it to a get_active_lanes
> > > helper.
> > 
> > Then drivers presumably would also validate this against the endpoint
> > configuration, wouldn't they? That's extra code in every CSI-2 receiver
> > driver.
> 
> Why so ? The number of connected lanes can be passed to the helper
> function, which can use it to validate the number of lanes reported by
> the source subdev.
> 

Apologies if I'm interpreting this wrong, but it seems that the main
thing I'm reading is that this is not the correct place to implement
this, and it should be implemented at a higher level (e.g. in v4l2) that
lets all MIPI CSI reciever drivers use it?

I have noticed that similar functionality has been implemented as part
of __v4l2_get_link_freq_pad. Are you suggesting that I take a similar
approach and resubmit as a new series?

> > > > > > The function could take struct media_pad pointer as an argument, or struct
> > > > > > v4l2_subdev pointer and the pad number.
> > > > > > 
> > > > > > I wonder if any other parameters could change dynamically but I can't think
> > > > > > of that now, so perhaps just the number of lanes is what the function
> > > > > > should indeed return.
> > > > > > 
> > > > > > > +
> > > > > > > +   return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > >  static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > > > > > >  {
> > > > > > >     struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > > > > > > @@ -965,6 +1002,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > > > > > >     format = v4l2_subdev_state_get_format(state, CSIS_PAD_SINK);
> > > > > > >     csis_fmt = find_csis_format(format->code);
> > > > > > >  
> > > > > > > +   ret = mipi_csis_get_active_lanes(sd);
> > > > > > > +   if (ret < 0)
> > > > > > > +           dev_dbg(csis->dev, "Failed to get active lanes: %d", ret);
> > > > > > > +
> > > > > > >     ret = mipi_csis_calculate_params(csis, csis_fmt);
> > > > > > >     if (ret < 0)
> > > > > > >             goto err_unlock;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Thank you very much for the help!

Best wishes,

Isaac

On Tue, Sep 02, 2025 at 01:28:37PM +0100, Isaac Scott wrote:
> Quoting Laurent Pinchart (2025-08-19 03:44:13)
> <snip>
> > > > > > That would need to parse the endpoint every time we start streaming, it
> > > > > > doesn't sound ideal.
> > > > > 
> > > > > Perhaps not, but does that matter in practice? Parsing the endpoint is,
> > > > > after all, fairly trivial. The advantage would be simplifying drivers.
> > > > 
> > > > It's trivial from a code point of view, but it's not a cheap operation.
> > > > I'd like to avoid making starting streaming more expensive.
> > > 
> > > How cheap is "not cheap"? I'd be surprised if parsing an endpoint took more
> > > time than e.g. an I²C register write. Of course it depends on the CPU...
> > 
> > Still, it's not cheap, and I think it can easily be avoided.
> > 
> > > > > Alternatively we could think of caching this information somewhere but I
> > > > > don't think it's worth it.
> > > > 
> > > > Drivers likely need to parse endpoints for other reasons. I'd cache the
> > > > value in drivers, like done today, and pass it to a get_active_lanes
> > > > helper.
> > > 
> > > Then drivers presumably would also validate this against the endpoint
> > > configuration, wouldn't they? That's extra code in every CSI-2 receiver
> > > driver.
> > 
> > Why so ? The number of connected lanes can be passed to the helper
> > function, which can use it to validate the number of lanes reported by
> > the source subdev.
> 
> Apologies if I'm interpreting this wrong, but it seems that the main
> thing I'm reading is that this is not the correct place to implement
> this, and it should be implemented at a higher level (e.g. in v4l2) that
> lets all MIPI CSI reciever drivers use it?
> 
> I have noticed that similar functionality has been implemented as part
> of __v4l2_get_link_freq_pad. Are you suggesting that I take a similar
> approach and resubmit as a new series?

As far as iI understand, Sakari would like a helper function that will
query the remote subdev for the number of data lanes it uses, and
validates that against the number of connected data lanes as described
by DT. I don't like the idea of parsing the endpoint properties every
time we do so, so I think the number of connected data lanes should be
passed by the driver to the helper instead. The helper would still query
the remote subdev, and validate the value.

> > > > > > > The function could take struct media_pad pointer as an argument, or struct
> > > > > > > v4l2_subdev pointer and the pad number.
> > > > > > > 
> > > > > > > I wonder if any other parameters could change dynamically but I can't think
> > > > > > > of that now, so perhaps just the number of lanes is what the function
> > > > > > > should indeed return.
> > > > > > > 
> > > > > > > > +
> > > > > > > > +   return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > > > > > > >  {
> > > > > > > >     struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > > > > > > > @@ -965,6 +1002,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > > > > > > >     format = v4l2_subdev_state_get_format(state, CSIS_PAD_SINK);
> > > > > > > >     csis_fmt = find_csis_format(format->code);
> > > > > > > >  
> > > > > > > > +   ret = mipi_csis_get_active_lanes(sd);
> > > > > > > > +   if (ret < 0)
> > > > > > > > +           dev_dbg(csis->dev, "Failed to get active lanes: %d", ret);
> > > > > > > > +
> > > > > > > >     ret = mipi_csis_calculate_params(csis, csis_fmt);
> > > > > > > >     if (ret < 0)
> > > > > > > >             goto err_unlock;

-- 
Regards,

Laurent Pinchart

Hi Laurent, Isaac,

On Tue, Sep 02, 2025 at 02:38:05PM +0200, Laurent Pinchart wrote:
> On Tue, Sep 02, 2025 at 01:28:37PM +0100, Isaac Scott wrote:
> > Quoting Laurent Pinchart (2025-08-19 03:44:13)
> > <snip>
> > > > > > > That would need to parse the endpoint every time we start streaming, it
> > > > > > > doesn't sound ideal.
> > > > > > 
> > > > > > Perhaps not, but does that matter in practice? Parsing the endpoint is,
> > > > > > after all, fairly trivial. The advantage would be simplifying drivers.
> > > > > 
> > > > > It's trivial from a code point of view, but it's not a cheap operation.
> > > > > I'd like to avoid making starting streaming more expensive.
> > > > 
> > > > How cheap is "not cheap"? I'd be surprised if parsing an endpoint took more
> > > > time than e.g. an I²C register write. Of course it depends on the CPU...
> > > 
> > > Still, it's not cheap, and I think it can easily be avoided.
> > > 
> > > > > > Alternatively we could think of caching this information somewhere but I
> > > > > > don't think it's worth it.
> > > > > 
> > > > > Drivers likely need to parse endpoints for other reasons. I'd cache the
> > > > > value in drivers, like done today, and pass it to a get_active_lanes
> > > > > helper.
> > > > 
> > > > Then drivers presumably would also validate this against the endpoint
> > > > configuration, wouldn't they? That's extra code in every CSI-2 receiver
> > > > driver.
> > > 
> > > Why so ? The number of connected lanes can be passed to the helper
> > > function, which can use it to validate the number of lanes reported by
> > > the source subdev.
> > 
> > Apologies if I'm interpreting this wrong, but it seems that the main
> > thing I'm reading is that this is not the correct place to implement
> > this, and it should be implemented at a higher level (e.g. in v4l2) that
> > lets all MIPI CSI reciever drivers use it?
> > 
> > I have noticed that similar functionality has been implemented as part
> > of __v4l2_get_link_freq_pad. Are you suggesting that I take a similar
> > approach and resubmit as a new series?
> 
> As far as iI understand, Sakari would like a helper function that will
> query the remote subdev for the number of data lanes it uses, and
> validates that against the number of connected data lanes as described
> by DT. I don't like the idea of parsing the endpoint properties every
> time we do so, so I think the number of connected data lanes should be
> passed by the driver to the helper instead. The helper would still query
> the remote subdev, and validate the value.

As long as the bulk of the work is done by the helper, I'm fine. This is a
fairly specific need but still in principle every CSI-2 receiver driver
needs it, so ease of use does count.

The helper could e.g. take the number of lanes in the endpoint as an
additional argument and just return the value if the sub-device doesn't
implement get_mbus_config() pad op. That'd be fairly trivial to use in a
driver.

> 
> > > > > > > > The function could take struct media_pad pointer as an argument, or struct
> > > > > > > > v4l2_subdev pointer and the pad number.
> > > > > > > > 
> > > > > > > > I wonder if any other parameters could change dynamically but I can't think
> > > > > > > > of that now, so perhaps just the number of lanes is what the function
> > > > > > > > should indeed return.
> > > > > > > > 
> > > > > > > > > +
> > > > > > > > > +   return 0;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > > > > > > > >  {
> > > > > > > > >     struct mipi_csis_device *csis = sd_to_mipi_csis_device(sd);
> > > > > > > > > @@ -965,6 +1002,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > > > > > > > >     format = v4l2_subdev_state_get_format(state, CSIS_PAD_SINK);
> > > > > > > > >     csis_fmt = find_csis_format(format->code);
> > > > > > > > >  
> > > > > > > > > +   ret = mipi_csis_get_active_lanes(sd);
> > > > > > > > > +   if (ret < 0)
> > > > > > > > > +           dev_dbg(csis->dev, "Failed to get active lanes: %d", ret);
> > > > > > > > > +
> > > > > > > > >     ret = mipi_csis_calculate_params(csis, csis_fmt);
> > > > > > > > >     if (ret < 0)
> > > > > > > > >             goto err_unlock;

-- 
Kind regards,

Sakari Ailus