diff mbox series

[41/55] media: rkisp1: csi: Plumb the CSI RX subdev

Message ID 20220614191127.3420492-42-paul.elder@ideasonboard.com (mailing list archive)
State Superseded
Headers show
Series media: rkisp1: Cleanups and add support for i.MX8MP | expand

Commit Message

Paul Elder June 14, 2022, 7:11 p.m. UTC 
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Connect the CSI receiver subdevice into the rest of the driver. This
includes:
- calling the subdevice via the v4l2 subdev API
- moving the async notifier for the sensor from the ISP to the CSI
  receiver
- in the ISP, create a media link to the CSI receiver, and remove the
  media link creation to the sensor
- in the CSI receiver, create a media link to the sensor

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../platform/rockchip/rkisp1/rkisp1-csi.c     | 34 ++++++++++++++++--
 .../platform/rockchip/rkisp1/rkisp1-csi.h     |  6 ++--
 .../platform/rockchip/rkisp1/rkisp1-dev.c     | 36 +++++++++----------
 .../platform/rockchip/rkisp1/rkisp1-isp.c     | 21 ++---------
 4 files changed, 53 insertions(+), 44 deletions(-)

Comments

Laurent Pinchart June 15, 2022, 11:10 p.m. UTC | #1 
Hi Paul,

Thank you for the patch.

On Wed, Jun 15, 2022 at 04:11:13AM +0900, Paul Elder wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Connect the CSI receiver subdevice into the rest of the driver. This
> includes:
> - calling the subdevice via the v4l2 subdev API
> - moving the async notifier for the sensor from the ISP to the CSI
>   receiver
> - in the ISP, create a media link to the CSI receiver, and remove the
>   media link creation to the sensor
> - in the CSI receiver, create a media link to the sensor
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../platform/rockchip/rkisp1/rkisp1-csi.c     | 34 ++++++++++++++++--
>  .../platform/rockchip/rkisp1/rkisp1-csi.h     |  6 ++--
>  .../platform/rockchip/rkisp1/rkisp1-dev.c     | 36 +++++++++----------
>  .../platform/rockchip/rkisp1/rkisp1-isp.c     | 21 ++---------
>  4 files changed, 53 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> index 8182694a6fe0..96712b467dde 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> @@ -43,6 +43,34 @@ rkisp1_csi_get_pad_fmt(struct rkisp1_csi *csi,
>  		return v4l2_subdev_get_try_format(&csi->sd, &state, pad);
>  }
>  
> +int rkisp1_csi_link_sensor(struct rkisp1_device *rkisp1, struct v4l2_subdev *sd,
> +			   struct rkisp1_sensor_async *s_asd,
> +			   unsigned int source_pad)
> +{
> +	struct rkisp1_csi *csi = &rkisp1->csi;
> +	int ret;		

There's trailing whitespace here.

> +
> +	s_asd->pixel_rate_ctrl = v4l2_ctrl_find(sd->ctrl_handler,
> +						V4L2_CID_PIXEL_RATE);
> +	if (!s_asd->pixel_rate_ctrl) {
> +		dev_err(rkisp1->dev, "No pixel rate control in subdev %s\n",
> +			sd->name);
> +		return -EINVAL;
> +	}
> +
> +	/* Create the link from the sensor to the CSI receiver. */
> +	ret = media_create_pad_link(&sd->entity, source_pad,
> +				    &csi->sd.entity, RKISP1_CSI_PAD_SINK,
> +				    !s_asd->index ? MEDIA_LNK_FL_ENABLED : 0);
> +	if (ret) {
> +		dev_err(csi->rkisp1->dev, "failed to link src pad of %s\n",
> +			sd->name);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int rkisp1_csi_config(struct rkisp1_csi *csi,
>  			     const struct rkisp1_sensor_async *sensor)
>  {
> @@ -118,8 +146,8 @@ static void rkisp1_csi_disable(struct rkisp1_csi *csi)
>  		     val & (~RKISP1_CIF_MIPI_CTRL_OUTPUT_ENA));
>  }
>  
> -int rkisp1_csi_start(struct rkisp1_csi *csi,
> -		     const struct rkisp1_sensor_async *sensor)
> +static int rkisp1_csi_start(struct rkisp1_csi *csi,
> +			    const struct rkisp1_sensor_async *sensor)
>  {
>  	struct rkisp1_device *rkisp1 = csi->rkisp1;
>  	union phy_configure_opts opts;
> @@ -155,7 +183,7 @@ int rkisp1_csi_start(struct rkisp1_csi *csi,
>  	return 0;
>  }
>  
> -void rkisp1_csi_stop(struct rkisp1_csi *csi)
> +static void rkisp1_csi_stop(struct rkisp1_csi *csi)
>  {
>  	rkisp1_csi_disable(csi);
>  
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h
> index ddf8e5e08f55..eadcd24f65fb 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h
> @@ -21,8 +21,8 @@ void rkisp1_csi_cleanup(struct rkisp1_device *rkisp1);
>  int rkisp1_csi_register(struct rkisp1_device *rkisp1);
>  void rkisp1_csi_unregister(struct rkisp1_device *rkisp1);
>  
> -int rkisp1_csi_start(struct rkisp1_csi *csi,
> -		     const struct rkisp1_sensor_async *sensor);
> -void rkisp1_csi_stop(struct rkisp1_csi *csi);
> +int rkisp1_csi_link_sensor(struct rkisp1_device *rkisp1, struct v4l2_subdev *sd,
> +			   struct rkisp1_sensor_async *s_asd,
> +			   unsigned int source_pad);
>  
>  #endif /* _RKISP1_CSI_H */
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> index faf2cd4c8149..a3e182c86bdd 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -17,6 +17,7 @@
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/pm_runtime.h>
>  #include <media/v4l2-fwnode.h>
> +#include <media/v4l2-mc.h>
>  
>  #include "rkisp1-common.h"
>  #include "rkisp1-csi.h"
> @@ -119,17 +120,8 @@ static int rkisp1_subdev_notifier_bound(struct v4l2_async_notifier *notifier,
>  		container_of(asd, struct rkisp1_sensor_async, asd);
>  	int source_pad;
>  
> -	s_asd->pixel_rate_ctrl = v4l2_ctrl_find(sd->ctrl_handler,
> -						V4L2_CID_PIXEL_RATE);
> -	if (!s_asd->pixel_rate_ctrl) {
> -		dev_err(rkisp1->dev, "No pixel rate control in subdev %s\n",
> -			sd->name);
> -		return -EINVAL;
> -	}
> -
>  	s_asd->sd = sd;
>  
> -	/* Create the link to the sensor. */
>  	source_pad = media_entity_get_fwnode_pad(&sd->entity, s_asd->source_ep,
>  						 MEDIA_PAD_FL_SOURCE);
>  	if (source_pad < 0) {
> @@ -138,10 +130,7 @@ static int rkisp1_subdev_notifier_bound(struct v4l2_async_notifier *notifier,
>  		return source_pad;
>  	}
>  
> -	return media_create_pad_link(&sd->entity, source_pad,
> -				     &rkisp1->isp.sd.entity,
> -				     RKISP1_ISP_PAD_SINK_VIDEO,
> -				     !s_asd->index ? MEDIA_LNK_FL_ENABLED : 0);
> +	return rkisp1_csi_link_sensor(rkisp1, sd, s_asd, source_pad);
>  }
>  
>  static int rkisp1_subdev_notifier_complete(struct v4l2_async_notifier *notifier)
> @@ -283,6 +272,14 @@ static int rkisp1_create_links(struct rkisp1_device *rkisp1)
>  	unsigned int i;
>  	int ret;
>  
> +	/* Link the CSI receiver to the ISP. */
> +	ret = media_create_pad_link(&rkisp1->csi.sd.entity, RKISP1_CSI_PAD_SRC,
> +				    &rkisp1->isp.sd.entity,
> +				    RKISP1_ISP_PAD_SINK_VIDEO,
> +				    MEDIA_LNK_FL_ENABLED);
> +	if (ret)
> +		return ret;
> +
>  	/* create ISP->RSZ->CAP links */
>  	for (i = 0; i < 2; i++) {
>  		struct media_entity *resizer =
> @@ -364,13 +361,6 @@ static int rkisp1_entities_register(struct rkisp1_device *rkisp1)
>  	if (ret)
>  		goto error;
>  
> -	ret = rkisp1_subdev_notifier_register(rkisp1);
> -	if (ret) {
> -		dev_err(rkisp1->dev,
> -			"Failed to register subdev notifier(%d)\n", ret);
> -		goto error;
> -	}
> -
>  	return 0;
>  
>  error:
> @@ -534,10 +524,16 @@ static int rkisp1_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_cleanup_csi;
>  
> +	ret = rkisp1_subdev_notifier_register(rkisp1);
> +	if (ret)
> +		goto err_unreg_entities;
> +
>  	rkisp1_debug_init(rkisp1);
>  
>  	return 0;
>  
> +err_unreg_entities:
> +	rkisp1_entities_unregister(rkisp1);
>  err_cleanup_csi:
>  	rkisp1_csi_cleanup(rkisp1);
>  err_unreg_media_dev:
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> index 5afb8be311c7..260c9ce0dca4 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> @@ -16,7 +16,6 @@
>  #include <media/v4l2-event.h>
>  
>  #include "rkisp1-common.h"
> -#include "rkisp1-csi.h"
>  
>  #define RKISP1_DEF_SINK_PAD_FMT MEDIA_BUS_FMT_SRGGB10_1X10
>  #define RKISP1_DEF_SRC_PAD_FMT MEDIA_BUS_FMT_YUYV8_2X8
> @@ -728,16 +727,12 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct rkisp1_isp *isp = to_rkisp1_isp(sd);
>  	struct rkisp1_device *rkisp1 = isp->rkisp1;
> -	const struct rkisp1_sensor_async *asd;
>  	struct media_pad *source_pad;
>  	int ret;
>  
>  	if (!enable) {
>  		v4l2_subdev_call(rkisp1->source, video, s_stream, false);
> -
> -		rkisp1_csi_stop(&rkisp1->csi);
>  		rkisp1_isp_stop(isp);
> -
>  		return 0;
>  	}
>  
> @@ -754,30 +749,20 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
>  		return -EPIPE;
>  	}
>  
> -	asd = container_of(rkisp1->source->asd, struct rkisp1_sensor_async,
> -			   asd);
> -
> -	if (asd->mbus_type != V4L2_MBUS_CSI2_DPHY)
> -		return -EINVAL;
> +	if (rkisp1->source != &rkisp1->csi.sd)
> +		return -EPIPE;
>  
>  	isp->frame_sequence = -1;
>  	mutex_lock(&isp->ops_lock);
> -	ret = rkisp1_config_cif(isp, asd->mbus_type, asd->mbus_flags);
> +	ret = rkisp1_config_cif(isp, V4L2_MBUS_CSI2_DPHY, 0);
>  	if (ret)
>  		goto mutex_unlock;
>  
>  	rkisp1_isp_start(isp);
>  
> -	ret = rkisp1_csi_start(&rkisp1->csi, asd);
> -	if (ret) {
> -		rkisp1_isp_stop(isp);
> -		goto mutex_unlock;
> -	}
> -
>  	ret = v4l2_subdev_call(rkisp1->source, video, s_stream, true);
>  	if (ret) {
>  		rkisp1_isp_stop(isp);
> -		rkisp1_csi_stop(&rkisp1->csi);
>  		goto mutex_unlock;
>  	}
>  
> -- 
> 2.30.2
>
Dafna Hirschfeld June 25, 2022, 7:45 a.m. UTC | #2 
On 15.06.2022 04:11, Paul Elder wrote:
>From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
>Connect the CSI receiver subdevice into the rest of the driver. This
>includes:

To make it clear where you put the csi, i'll add here
'Connect the CSI subdevice between the source
and the isp' or something like that.

Also the sketch 'Media Topology' in rkisp1-dev.c should be updated.
I'd also wonder if we should ignore src configuration of the isp entity
since it must be identical to the sink of the csi.

>- calling the subdevice via the v4l2 subdev API
>- moving the async notifier for the sensor from the ISP to the CSI
>  receiver
>- in the ISP, create a media link to the CSI receiver, and remove the
>  media link creation to the sensor
>- in the CSI receiver, create a media link to the sensor
>
>Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>---
> .../platform/rockchip/rkisp1/rkisp1-csi.c     | 34 ++++++++++++++++--
> .../platform/rockchip/rkisp1/rkisp1-csi.h     |  6 ++--
> .../platform/rockchip/rkisp1/rkisp1-dev.c     | 36 +++++++++----------
> .../platform/rockchip/rkisp1/rkisp1-isp.c     | 21 ++---------
> 4 files changed, 53 insertions(+), 44 deletions(-)
>
>diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
>index 8182694a6fe0..96712b467dde 100644
>--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
>+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
>@@ -43,6 +43,34 @@ rkisp1_csi_get_pad_fmt(struct rkisp1_csi *csi,
> 		return v4l2_subdev_get_try_format(&csi->sd, &state, pad);
> }
>
>+int rkisp1_csi_link_sensor(struct rkisp1_device *rkisp1, struct v4l2_subdev *sd,
>+			   struct rkisp1_sensor_async *s_asd,
>+			   unsigned int source_pad)
>+{
>+	struct rkisp1_csi *csi = &rkisp1->csi;
>+	int ret;		

extra space after 'ret;' ?

>+
>+	s_asd->pixel_rate_ctrl = v4l2_ctrl_find(sd->ctrl_handler,
>+						V4L2_CID_PIXEL_RATE);
>+	if (!s_asd->pixel_rate_ctrl) {
>+		dev_err(rkisp1->dev, "No pixel rate control in subdev %s\n",
>+			sd->name);
>+		return -EINVAL;
>+	}
>+
>+	/* Create the link from the sensor to the CSI receiver. */
>+	ret = media_create_pad_link(&sd->entity, source_pad,
>+				    &csi->sd.entity, RKISP1_CSI_PAD_SINK,
>+				    !s_asd->index ? MEDIA_LNK_FL_ENABLED : 0);
>+	if (ret) {
>+		dev_err(csi->rkisp1->dev, "failed to link src pad of %s\n",
>+			sd->name);
>+		return ret;
>+	}
>+
>+	return 0;
>+}
>+
> static int rkisp1_csi_config(struct rkisp1_csi *csi,
> 			     const struct rkisp1_sensor_async *sensor)
> {
>@@ -118,8 +146,8 @@ static void rkisp1_csi_disable(struct rkisp1_csi *csi)
> 		     val & (~RKISP1_CIF_MIPI_CTRL_OUTPUT_ENA));
> }
>
>-int rkisp1_csi_start(struct rkisp1_csi *csi,
>-		     const struct rkisp1_sensor_async *sensor)
>+static int rkisp1_csi_start(struct rkisp1_csi *csi,
>+			    const struct rkisp1_sensor_async *sensor)
> {
> 	struct rkisp1_device *rkisp1 = csi->rkisp1;
> 	union phy_configure_opts opts;
>@@ -155,7 +183,7 @@ int rkisp1_csi_start(struct rkisp1_csi *csi,
> 	return 0;
> }
>
>-void rkisp1_csi_stop(struct rkisp1_csi *csi)
>+static void rkisp1_csi_stop(struct rkisp1_csi *csi)
> {
> 	rkisp1_csi_disable(csi);
>
>diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h
>index ddf8e5e08f55..eadcd24f65fb 100644
>--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h
>+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h
>@@ -21,8 +21,8 @@ void rkisp1_csi_cleanup(struct rkisp1_device *rkisp1);
> int rkisp1_csi_register(struct rkisp1_device *rkisp1);
> void rkisp1_csi_unregister(struct rkisp1_device *rkisp1);
>
>-int rkisp1_csi_start(struct rkisp1_csi *csi,
>-		     const struct rkisp1_sensor_async *sensor);
>-void rkisp1_csi_stop(struct rkisp1_csi *csi);
>+int rkisp1_csi_link_sensor(struct rkisp1_device *rkisp1, struct v4l2_subdev *sd,
>+			   struct rkisp1_sensor_async *s_asd,
>+			   unsigned int source_pad);
>
> #endif /* _RKISP1_CSI_H */
>diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
>index faf2cd4c8149..a3e182c86bdd 100644
>--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
>+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
>@@ -17,6 +17,7 @@
> #include <linux/pinctrl/consumer.h>
> #include <linux/pm_runtime.h>
> #include <media/v4l2-fwnode.h>
>+#include <media/v4l2-mc.h>
>
> #include "rkisp1-common.h"
> #include "rkisp1-csi.h"
>@@ -119,17 +120,8 @@ static int rkisp1_subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> 		container_of(asd, struct rkisp1_sensor_async, asd);
> 	int source_pad;
>
>-	s_asd->pixel_rate_ctrl = v4l2_ctrl_find(sd->ctrl_handler,
>-						V4L2_CID_PIXEL_RATE);
>-	if (!s_asd->pixel_rate_ctrl) {
>-		dev_err(rkisp1->dev, "No pixel rate control in subdev %s\n",
>-			sd->name);
>-		return -EINVAL;
>-	}
>-
> 	s_asd->sd = sd;
>
>-	/* Create the link to the sensor. */
> 	source_pad = media_entity_get_fwnode_pad(&sd->entity, s_asd->source_ep,
> 						 MEDIA_PAD_FL_SOURCE);
> 	if (source_pad < 0) {
>@@ -138,10 +130,7 @@ static int rkisp1_subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> 		return source_pad;
> 	}
>
>-	return media_create_pad_link(&sd->entity, source_pad,
>-				     &rkisp1->isp.sd.entity,
>-				     RKISP1_ISP_PAD_SINK_VIDEO,
>-				     !s_asd->index ? MEDIA_LNK_FL_ENABLED : 0);
>+	return rkisp1_csi_link_sensor(rkisp1, sd, s_asd, source_pad);
> }
>
> static int rkisp1_subdev_notifier_complete(struct v4l2_async_notifier *notifier)
>@@ -283,6 +272,14 @@ static int rkisp1_create_links(struct rkisp1_device *rkisp1)
> 	unsigned int i;
> 	int ret;
>
>+	/* Link the CSI receiver to the ISP. */
>+	ret = media_create_pad_link(&rkisp1->csi.sd.entity, RKISP1_CSI_PAD_SRC,
>+				    &rkisp1->isp.sd.entity,
>+				    RKISP1_ISP_PAD_SINK_VIDEO,
>+				    MEDIA_LNK_FL_ENABLED);

should this also be immutable?

>+	if (ret)
>+		return ret;
>+
> 	/* create ISP->RSZ->CAP links */
> 	for (i = 0; i < 2; i++) {
> 		struct media_entity *resizer =
>@@ -364,13 +361,6 @@ static int rkisp1_entities_register(struct rkisp1_device *rkisp1)
> 	if (ret)
> 		goto error;
>
>-	ret = rkisp1_subdev_notifier_register(rkisp1);
>-	if (ret) {
>-		dev_err(rkisp1->dev,
>-			"Failed to register subdev notifier(%d)\n", ret);
>-		goto error;
>-	}
>-
> 	return 0;
>
> error:
>@@ -534,10 +524,16 @@ static int rkisp1_probe(struct platform_device *pdev)
> 	if (ret)
> 		goto err_cleanup_csi;
>
>+	ret = rkisp1_subdev_notifier_register(rkisp1);
>+	if (ret)
>+		goto err_unreg_entities;
>+
> 	rkisp1_debug_init(rkisp1);
>
> 	return 0;
>
>+err_unreg_entities:
>+	rkisp1_entities_unregister(rkisp1);
> err_cleanup_csi:
> 	rkisp1_csi_cleanup(rkisp1);
> err_unreg_media_dev:
>diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>index 5afb8be311c7..260c9ce0dca4 100644
>--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>@@ -16,7 +16,6 @@
> #include <media/v4l2-event.h>
>
> #include "rkisp1-common.h"
>-#include "rkisp1-csi.h"
>
> #define RKISP1_DEF_SINK_PAD_FMT MEDIA_BUS_FMT_SRGGB10_1X10
> #define RKISP1_DEF_SRC_PAD_FMT MEDIA_BUS_FMT_YUYV8_2X8
>@@ -728,16 +727,12 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
> {
> 	struct rkisp1_isp *isp = to_rkisp1_isp(sd);
> 	struct rkisp1_device *rkisp1 = isp->rkisp1;
>-	const struct rkisp1_sensor_async *asd;
> 	struct media_pad *source_pad;
> 	int ret;
>
> 	if (!enable) {
> 		v4l2_subdev_call(rkisp1->source, video, s_stream, false);
>-
>-		rkisp1_csi_stop(&rkisp1->csi);
> 		rkisp1_isp_stop(isp);
>-
> 		return 0;
> 	}
>
>@@ -754,30 +749,20 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
> 		return -EPIPE;
> 	}
>
>-	asd = container_of(rkisp1->source->asd, struct rkisp1_sensor_async,
>-			   asd);
>-
>-	if (asd->mbus_type != V4L2_MBUS_CSI2_DPHY)
>-		return -EINVAL;
>+	if (rkisp1->source != &rkisp1->csi.sd)
>+		return -EPIPE;

This is not very clear, 'source' should point to the remote source (the one outside this driver)
so why should it be 'csi.sd' ?
Or is it that now 'rkips1->source' might be either the remote source or csi? if so I think it is a bit confusing.

>
> 	isp->frame_sequence = -1;
> 	mutex_lock(&isp->ops_lock);
>-	ret = rkisp1_config_cif(isp, asd->mbus_type, asd->mbus_flags);
>+	ret = rkisp1_config_cif(isp, V4L2_MBUS_CSI2_DPHY, 0);
> 	if (ret)
> 		goto mutex_unlock;
>
> 	rkisp1_isp_start(isp);
>
>-	ret = rkisp1_csi_start(&rkisp1->csi, asd);
>-	if (ret) {
>-		rkisp1_isp_stop(isp);
>-		goto mutex_unlock;
>-	}
>-
> 	ret = v4l2_subdev_call(rkisp1->source, video, s_stream, true);

Now that isp should call 's_stream' for 'rkisp1->csi->sd' and not 'rkisp1->source'.

thanks,
Dafna

> 	if (ret) {
> 		rkisp1_isp_stop(isp);
>-		rkisp1_csi_stop(&rkisp1->csi);
> 		goto mutex_unlock;
> 	}
>
>-- 
>2.30.2
>
Laurent Pinchart June 25, 2022, 4:07 p.m. UTC | #3 
Hi Dafna,

On Sat, Jun 25, 2022 at 10:45:59AM +0300, Dafna Hirschfeld wrote:
> On 15.06.2022 04:11, Paul Elder wrote:
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Connect the CSI receiver subdevice into the rest of the driver. This
> > includes:
> 
> To make it clear where you put the csi, i'll add here
> 'Connect the CSI subdevice between the source
> and the isp' or something like that.
> 
> Also the sketch 'Media Topology' in rkisp1-dev.c should be updated.

Good point, I'll do that.

> I'd also wonder if we should ignore src configuration of the isp entity
> since it must be identical to the sink of the csi.

What do you mean exactly by ignoring it ?

> > - calling the subdevice via the v4l2 subdev API
> > - moving the async notifier for the sensor from the ISP to the CSI
> >   receiver
> > - in the ISP, create a media link to the CSI receiver, and remove the
> >   media link creation to the sensor
> > - in the CSI receiver, create a media link to the sensor
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  .../platform/rockchip/rkisp1/rkisp1-csi.c     | 34 ++++++++++++++++--
> >  .../platform/rockchip/rkisp1/rkisp1-csi.h     |  6 ++--
> >  .../platform/rockchip/rkisp1/rkisp1-dev.c     | 36 +++++++++----------
> >  .../platform/rockchip/rkisp1/rkisp1-isp.c     | 21 ++---------
> >  4 files changed, 53 insertions(+), 44 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> > index 8182694a6fe0..96712b467dde 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> > @@ -43,6 +43,34 @@ rkisp1_csi_get_pad_fmt(struct rkisp1_csi *csi,
> >  		return v4l2_subdev_get_try_format(&csi->sd, &state, pad);
> >  }
> > 
> > +int rkisp1_csi_link_sensor(struct rkisp1_device *rkisp1, struct v4l2_subdev *sd,
> > +			   struct rkisp1_sensor_async *s_asd,
> > +			   unsigned int source_pad)
> > +{
> > +	struct rkisp1_csi *csi = &rkisp1->csi;
> > +	int ret;		
> 
> extra space after 'ret;' ?

Oh.

Paul, did you get so used to the fact that checkstyle.py is integrated
in git hooks in libcamera that you forgot that checkpatch.pl has to be
run manually ? :-)

> > +
> > +	s_asd->pixel_rate_ctrl = v4l2_ctrl_find(sd->ctrl_handler,
> > +						V4L2_CID_PIXEL_RATE);
> > +	if (!s_asd->pixel_rate_ctrl) {
> > +		dev_err(rkisp1->dev, "No pixel rate control in subdev %s\n",
> > +			sd->name);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Create the link from the sensor to the CSI receiver. */
> > +	ret = media_create_pad_link(&sd->entity, source_pad,
> > +				    &csi->sd.entity, RKISP1_CSI_PAD_SINK,
> > +				    !s_asd->index ? MEDIA_LNK_FL_ENABLED : 0);
> > +	if (ret) {
> > +		dev_err(csi->rkisp1->dev, "failed to link src pad of %s\n",
> > +			sd->name);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int rkisp1_csi_config(struct rkisp1_csi *csi,
> >  			     const struct rkisp1_sensor_async *sensor)
> >  {
> > @@ -118,8 +146,8 @@ static void rkisp1_csi_disable(struct rkisp1_csi *csi)
> >  		     val & (~RKISP1_CIF_MIPI_CTRL_OUTPUT_ENA));
> >  }
> > 
> > -int rkisp1_csi_start(struct rkisp1_csi *csi,
> > -		     const struct rkisp1_sensor_async *sensor)
> > +static int rkisp1_csi_start(struct rkisp1_csi *csi,
> > +			    const struct rkisp1_sensor_async *sensor)
> >  {
> >  	struct rkisp1_device *rkisp1 = csi->rkisp1;
> >  	union phy_configure_opts opts;
> > @@ -155,7 +183,7 @@ int rkisp1_csi_start(struct rkisp1_csi *csi,
> >  	return 0;
> >  }
> > 
> > -void rkisp1_csi_stop(struct rkisp1_csi *csi)
> > +static void rkisp1_csi_stop(struct rkisp1_csi *csi)
> >  {
> >  	rkisp1_csi_disable(csi);
> > 
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h
> > index ddf8e5e08f55..eadcd24f65fb 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h
> > @@ -21,8 +21,8 @@ void rkisp1_csi_cleanup(struct rkisp1_device *rkisp1);
> >  int rkisp1_csi_register(struct rkisp1_device *rkisp1);
> >  void rkisp1_csi_unregister(struct rkisp1_device *rkisp1);
> > 
> > -int rkisp1_csi_start(struct rkisp1_csi *csi,
> > -		     const struct rkisp1_sensor_async *sensor);
> > -void rkisp1_csi_stop(struct rkisp1_csi *csi);
> > +int rkisp1_csi_link_sensor(struct rkisp1_device *rkisp1, struct v4l2_subdev *sd,
> > +			   struct rkisp1_sensor_async *s_asd,
> > +			   unsigned int source_pad);
> > 
> >  #endif /* _RKISP1_CSI_H */
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > index faf2cd4c8149..a3e182c86bdd 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/pinctrl/consumer.h>
> >  #include <linux/pm_runtime.h>
> >  #include <media/v4l2-fwnode.h>
> > +#include <media/v4l2-mc.h>
> > 
> >  #include "rkisp1-common.h"
> >  #include "rkisp1-csi.h"
> > @@ -119,17 +120,8 @@ static int rkisp1_subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> >  		container_of(asd, struct rkisp1_sensor_async, asd);
> >  	int source_pad;
> > 
> > -	s_asd->pixel_rate_ctrl = v4l2_ctrl_find(sd->ctrl_handler,
> > -						V4L2_CID_PIXEL_RATE);
> > -	if (!s_asd->pixel_rate_ctrl) {
> > -		dev_err(rkisp1->dev, "No pixel rate control in subdev %s\n",
> > -			sd->name);
> > -		return -EINVAL;
> > -	}
> > -
> >  	s_asd->sd = sd;
> > 
> > -	/* Create the link to the sensor. */
> >  	source_pad = media_entity_get_fwnode_pad(&sd->entity, s_asd->source_ep,
> >  						 MEDIA_PAD_FL_SOURCE);
> >  	if (source_pad < 0) {
> > @@ -138,10 +130,7 @@ static int rkisp1_subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> >  		return source_pad;
> >  	}
> > 
> > -	return media_create_pad_link(&sd->entity, source_pad,
> > -				     &rkisp1->isp.sd.entity,
> > -				     RKISP1_ISP_PAD_SINK_VIDEO,
> > -				     !s_asd->index ? MEDIA_LNK_FL_ENABLED : 0);
> > +	return rkisp1_csi_link_sensor(rkisp1, sd, s_asd, source_pad);
> >  }
> > 
> >  static int rkisp1_subdev_notifier_complete(struct v4l2_async_notifier *notifier)
> > @@ -283,6 +272,14 @@ static int rkisp1_create_links(struct rkisp1_device *rkisp1)
> >  	unsigned int i;
> >  	int ret;
> > 
> > +	/* Link the CSI receiver to the ISP. */
> > +	ret = media_create_pad_link(&rkisp1->csi.sd.entity, RKISP1_CSI_PAD_SRC,
> > +				    &rkisp1->isp.sd.entity,
> > +				    RKISP1_ISP_PAD_SINK_VIDEO,
> > +				    MEDIA_LNK_FL_ENABLED);
> 
> should this also be immutable?

I don't think so, because one could connect a parallel sensor directly
to the ISP parallel input. This isn't well supported in the driver today
though, probably because nobody has cared much about upstreaming support
for such a setup, but I think it's a valid hardware option.

> > +	if (ret)
> > +		return ret;
> > +
> >  	/* create ISP->RSZ->CAP links */
> >  	for (i = 0; i < 2; i++) {
> >  		struct media_entity *resizer =
> > @@ -364,13 +361,6 @@ static int rkisp1_entities_register(struct rkisp1_device *rkisp1)
> >  	if (ret)
> >  		goto error;
> > 
> > -	ret = rkisp1_subdev_notifier_register(rkisp1);
> > -	if (ret) {
> > -		dev_err(rkisp1->dev,
> > -			"Failed to register subdev notifier(%d)\n", ret);
> > -		goto error;
> > -	}
> > -
> >  	return 0;
> > 
> >  error:
> > @@ -534,10 +524,16 @@ static int rkisp1_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		goto err_cleanup_csi;
> > 
> > +	ret = rkisp1_subdev_notifier_register(rkisp1);
> > +	if (ret)
> > +		goto err_unreg_entities;
> > +
> >  	rkisp1_debug_init(rkisp1);
> > 
> >  	return 0;
> > 
> > +err_unreg_entities:
> > +	rkisp1_entities_unregister(rkisp1);
> >  err_cleanup_csi:
> >  	rkisp1_csi_cleanup(rkisp1);
> >  err_unreg_media_dev:
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > index 5afb8be311c7..260c9ce0dca4 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > @@ -16,7 +16,6 @@
> >  #include <media/v4l2-event.h>
> > 
> >  #include "rkisp1-common.h"
> > -#include "rkisp1-csi.h"
> > 
> >  #define RKISP1_DEF_SINK_PAD_FMT MEDIA_BUS_FMT_SRGGB10_1X10
> >  #define RKISP1_DEF_SRC_PAD_FMT MEDIA_BUS_FMT_YUYV8_2X8
> > @@ -728,16 +727,12 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
> >  {
> >  	struct rkisp1_isp *isp = to_rkisp1_isp(sd);
> >  	struct rkisp1_device *rkisp1 = isp->rkisp1;
> > -	const struct rkisp1_sensor_async *asd;
> >  	struct media_pad *source_pad;
> >  	int ret;
> > 
> >  	if (!enable) {
> >  		v4l2_subdev_call(rkisp1->source, video, s_stream, false);
> > -
> > -		rkisp1_csi_stop(&rkisp1->csi);
> >  		rkisp1_isp_stop(isp);
> > -
> >  		return 0;
> >  	}
> > 
> > @@ -754,30 +749,20 @@ static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
> >  		return -EPIPE;
> >  	}
> > 
> > -	asd = container_of(rkisp1->source->asd, struct rkisp1_sensor_async,
> > -			   asd);
> > -
> > -	if (asd->mbus_type != V4L2_MBUS_CSI2_DPHY)
> > -		return -EINVAL;
> > +	if (rkisp1->source != &rkisp1->csi.sd)
> > +		return -EPIPE;
> 
> This is not very clear, 'source' should point to the remote source (the one outside this driver)
> so why should it be 'csi.sd' ?
> Or is it that now 'rkips1->source' might be either the remote source or csi? if so I think it is a bit confusing.

rkisp1->source is the source of the ISP. it can be the CSI-2 receiver,
or if a parallel sensor is connected directly to the ISP, the sensor
itself. The check here is meant to catch bugs in the implementation
while transitioning, as in this patch external CSI-2 receivers or
parallel sensors are not supported yet. Patch "media: rkisp1: Support
the ISP parallel input" then changes this code.

> > 
> >  	isp->frame_sequence = -1;
> >  	mutex_lock(&isp->ops_lock);
> > -	ret = rkisp1_config_cif(isp, asd->mbus_type, asd->mbus_flags);
> > +	ret = rkisp1_config_cif(isp, V4L2_MBUS_CSI2_DPHY, 0);
> >  	if (ret)
> >  		goto mutex_unlock;
> > 
> >  	rkisp1_isp_start(isp);
> > 
> > -	ret = rkisp1_csi_start(&rkisp1->csi, asd);
> > -	if (ret) {
> > -		rkisp1_isp_stop(isp);
> > -		goto mutex_unlock;
> > -	}
> > -
> >  	ret = v4l2_subdev_call(rkisp1->source, video, s_stream, true);
> 
> Now that isp should call 's_stream' for 'rkisp1->csi->sd' and not 'rkisp1->source'.

As explained above, I think source is correct here, as rkisp1->csi is
the internal CSI-2 receiver, which may or may not be the source of the
ISP, depending on link configuration.

> >  	if (ret) {
> >  		rkisp1_isp_stop(isp);
> > -		rkisp1_csi_stop(&rkisp1->csi);
> >  		goto mutex_unlock;
> >  	}
> >
diff mbox series

Patch

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
index 8182694a6fe0..96712b467dde 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
@@ -43,6 +43,34 @@  rkisp1_csi_get_pad_fmt(struct rkisp1_csi *csi,
 		return v4l2_subdev_get_try_format(&csi->sd, &state, pad);
 }
 
+int rkisp1_csi_link_sensor(struct rkisp1_device *rkisp1, struct v4l2_subdev *sd,
+			   struct rkisp1_sensor_async *s_asd,
+			   unsigned int source_pad)
+{
+	struct rkisp1_csi *csi = &rkisp1->csi;
+	int ret;		
+
+	s_asd->pixel_rate_ctrl = v4l2_ctrl_find(sd->ctrl_handler,
+						V4L2_CID_PIXEL_RATE);
+	if (!s_asd->pixel_rate_ctrl) {
+		dev_err(rkisp1->dev, "No pixel rate control in subdev %s\n",
+			sd->name);
+		return -EINVAL;
+	}
+
+	/* Create the link from the sensor to the CSI receiver. */
+	ret = media_create_pad_link(&sd->entity, source_pad,
+				    &csi->sd.entity, RKISP1_CSI_PAD_SINK,
+				    !s_asd->index ? MEDIA_LNK_FL_ENABLED : 0);
+	if (ret) {
+		dev_err(csi->rkisp1->dev, "failed to link src pad of %s\n",
+			sd->name);
+		return ret;
+	}
+
+	return 0;
+}
+
 static int rkisp1_csi_config(struct rkisp1_csi *csi,
 			     const struct rkisp1_sensor_async *sensor)
 {
@@ -118,8 +146,8 @@  static void rkisp1_csi_disable(struct rkisp1_csi *csi)
 		     val & (~RKISP1_CIF_MIPI_CTRL_OUTPUT_ENA));
 }
 
-int rkisp1_csi_start(struct rkisp1_csi *csi,
-		     const struct rkisp1_sensor_async *sensor)
+static int rkisp1_csi_start(struct rkisp1_csi *csi,
+			    const struct rkisp1_sensor_async *sensor)
 {
 	struct rkisp1_device *rkisp1 = csi->rkisp1;
 	union phy_configure_opts opts;
@@ -155,7 +183,7 @@  int rkisp1_csi_start(struct rkisp1_csi *csi,
 	return 0;
 }
 
-void rkisp1_csi_stop(struct rkisp1_csi *csi)
+static void rkisp1_csi_stop(struct rkisp1_csi *csi)
 {
 	rkisp1_csi_disable(csi);
 
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h
index ddf8e5e08f55..eadcd24f65fb 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.h
@@ -21,8 +21,8 @@  void rkisp1_csi_cleanup(struct rkisp1_device *rkisp1);
 int rkisp1_csi_register(struct rkisp1_device *rkisp1);
 void rkisp1_csi_unregister(struct rkisp1_device *rkisp1);
 
-int rkisp1_csi_start(struct rkisp1_csi *csi,
-		     const struct rkisp1_sensor_async *sensor);
-void rkisp1_csi_stop(struct rkisp1_csi *csi);
+int rkisp1_csi_link_sensor(struct rkisp1_device *rkisp1, struct v4l2_subdev *sd,
+			   struct rkisp1_sensor_async *s_asd,
+			   unsigned int source_pad);
 
 #endif /* _RKISP1_CSI_H */
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
index faf2cd4c8149..a3e182c86bdd 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
@@ -17,6 +17,7 @@ 
 #include <linux/pinctrl/consumer.h>
 #include <linux/pm_runtime.h>
 #include <media/v4l2-fwnode.h>
+#include <media/v4l2-mc.h>
 
 #include "rkisp1-common.h"
 #include "rkisp1-csi.h"
@@ -119,17 +120,8 @@  static int rkisp1_subdev_notifier_bound(struct v4l2_async_notifier *notifier,
 		container_of(asd, struct rkisp1_sensor_async, asd);
 	int source_pad;
 
-	s_asd->pixel_rate_ctrl = v4l2_ctrl_find(sd->ctrl_handler,
-						V4L2_CID_PIXEL_RATE);
-	if (!s_asd->pixel_rate_ctrl) {
-		dev_err(rkisp1->dev, "No pixel rate control in subdev %s\n",
-			sd->name);
-		return -EINVAL;
-	}
-
 	s_asd->sd = sd;
 
-	/* Create the link to the sensor. */
 	source_pad = media_entity_get_fwnode_pad(&sd->entity, s_asd->source_ep,
 						 MEDIA_PAD_FL_SOURCE);
 	if (source_pad < 0) {
@@ -138,10 +130,7 @@  static int rkisp1_subdev_notifier_bound(struct v4l2_async_notifier *notifier,
 		return source_pad;
 	}
 
-	return media_create_pad_link(&sd->entity, source_pad,
-				     &rkisp1->isp.sd.entity,
-				     RKISP1_ISP_PAD_SINK_VIDEO,
-				     !s_asd->index ? MEDIA_LNK_FL_ENABLED : 0);
+	return rkisp1_csi_link_sensor(rkisp1, sd, s_asd, source_pad);
 }
 
 static int rkisp1_subdev_notifier_complete(struct v4l2_async_notifier *notifier)
@@ -283,6 +272,14 @@  static int rkisp1_create_links(struct rkisp1_device *rkisp1)
 	unsigned int i;
 	int ret;
 
+	/* Link the CSI receiver to the ISP. */
+	ret = media_create_pad_link(&rkisp1->csi.sd.entity, RKISP1_CSI_PAD_SRC,
+				    &rkisp1->isp.sd.entity,
+				    RKISP1_ISP_PAD_SINK_VIDEO,
+				    MEDIA_LNK_FL_ENABLED);
+	if (ret)
+		return ret;
+
 	/* create ISP->RSZ->CAP links */
 	for (i = 0; i < 2; i++) {
 		struct media_entity *resizer =
@@ -364,13 +361,6 @@  static int rkisp1_entities_register(struct rkisp1_device *rkisp1)
 	if (ret)
 		goto error;
 
-	ret = rkisp1_subdev_notifier_register(rkisp1);
-	if (ret) {
-		dev_err(rkisp1->dev,
-			"Failed to register subdev notifier(%d)\n", ret);
-		goto error;
-	}
-
 	return 0;
 
 error:
@@ -534,10 +524,16 @@  static int rkisp1_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_cleanup_csi;
 
+	ret = rkisp1_subdev_notifier_register(rkisp1);
+	if (ret)
+		goto err_unreg_entities;
+
 	rkisp1_debug_init(rkisp1);
 
 	return 0;
 
+err_unreg_entities:
+	rkisp1_entities_unregister(rkisp1);
 err_cleanup_csi:
 	rkisp1_csi_cleanup(rkisp1);
 err_unreg_media_dev:
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
index 5afb8be311c7..260c9ce0dca4 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
@@ -16,7 +16,6 @@ 
 #include <media/v4l2-event.h>
 
 #include "rkisp1-common.h"
-#include "rkisp1-csi.h"
 
 #define RKISP1_DEF_SINK_PAD_FMT MEDIA_BUS_FMT_SRGGB10_1X10
 #define RKISP1_DEF_SRC_PAD_FMT MEDIA_BUS_FMT_YUYV8_2X8
@@ -728,16 +727,12 @@  static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct rkisp1_isp *isp = to_rkisp1_isp(sd);
 	struct rkisp1_device *rkisp1 = isp->rkisp1;
-	const struct rkisp1_sensor_async *asd;
 	struct media_pad *source_pad;
 	int ret;
 
 	if (!enable) {
 		v4l2_subdev_call(rkisp1->source, video, s_stream, false);
-
-		rkisp1_csi_stop(&rkisp1->csi);
 		rkisp1_isp_stop(isp);
-
 		return 0;
 	}
 
@@ -754,30 +749,20 @@  static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
 		return -EPIPE;
 	}
 
-	asd = container_of(rkisp1->source->asd, struct rkisp1_sensor_async,
-			   asd);
-
-	if (asd->mbus_type != V4L2_MBUS_CSI2_DPHY)
-		return -EINVAL;
+	if (rkisp1->source != &rkisp1->csi.sd)
+		return -EPIPE;
 
 	isp->frame_sequence = -1;
 	mutex_lock(&isp->ops_lock);
-	ret = rkisp1_config_cif(isp, asd->mbus_type, asd->mbus_flags);
+	ret = rkisp1_config_cif(isp, V4L2_MBUS_CSI2_DPHY, 0);
 	if (ret)
 		goto mutex_unlock;
 
 	rkisp1_isp_start(isp);
 
-	ret = rkisp1_csi_start(&rkisp1->csi, asd);
-	if (ret) {
-		rkisp1_isp_stop(isp);
-		goto mutex_unlock;
-	}
-
 	ret = v4l2_subdev_call(rkisp1->source, video, s_stream, true);
 	if (ret) {
 		rkisp1_isp_stop(isp);
-		rkisp1_csi_stop(&rkisp1->csi);
 		goto mutex_unlock;
 	}