[PATCH v2 RESEND 0/4] RZ/G2L CSI/CRU improvements

[PATCH v2 RESEND 0/4] RZ/G2L CSI/CRU improvements

[Biju Das]

This patch series aims to sync the CSI/CRU driver code with the latest
hardware manual (R01UH0914EJ0140 Rev.1.40).

v1->v2:
 * Updated commit description for patch#1 removing deprecated for
   SET_RUNTIME_PM_OPS() macro.
 * Aligned RUNTIME_PM_OPS() macro.
 * Added Rb tag from Laurent for patch#2 and #3.
 * Replaced usleep_range()->fsleep().
 * Added blank space after manual in commit description for patch#{2,3}.
 * Dropped clk-provider.h and __clk_is_enabled() as consumer clk should
   not use it. Plan to send RFC for clk_disable_unprepare_sync() in ccf.

Biju Das (4):
  media: platform: rzg2l-cru: rzg2l-csi2: Switch to RUNTIME_PM_OPS()
  media: platform: rzg2l-cru: rzg2l-ip: Add delay after D-PHY reset
  media: platform: rzg2l-cru: rzg2l-video: Fix image processing
    initialization
  media: platform: rzg2l-cru: rzg2l-video: Restructure clk handling

 .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  3 -
 .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   | 37 +++++----
 .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 18 ++--
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 83 ++++++++-----------
 4 files changed, 62 insertions(+), 79 deletions(-)

-- 
2.25.1

[PATCH v2 RESEND 1/4] media: platform: rzg2l-cru: rzg2l-csi2: Switch to RUNTIME_PM_OPS()

[Biju Das]

Replace the old SET_RUNTIME_PM_OPS() helpers with its modern alternative
RUNTIME_PM_OPS(). The usage of pm_ptr and RUNTIME_PM_OPS() allows the
compiler to see where it's used but still drop the dead code. After this
we can get rid of the unnecessary '__maybe_unused' annotations on PM
functions.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2:
 * Updated commit description.
 * Aligned RUNTIME_PM_OPS() macro.
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
index d20f4eff93a4..e00d9379dd2c 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
@@ -834,7 +834,7 @@ static void rzg2l_csi2_remove(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 }
 
-static int __maybe_unused rzg2l_csi2_pm_runtime_suspend(struct device *dev)
+static int rzg2l_csi2_pm_runtime_suspend(struct device *dev)
 {
 	struct rzg2l_csi2 *csi2 = dev_get_drvdata(dev);
 
@@ -843,7 +843,7 @@ static int __maybe_unused rzg2l_csi2_pm_runtime_suspend(struct device *dev)
 	return 0;
 }
 
-static int __maybe_unused rzg2l_csi2_pm_runtime_resume(struct device *dev)
+static int rzg2l_csi2_pm_runtime_resume(struct device *dev)
 {
 	struct rzg2l_csi2 *csi2 = dev_get_drvdata(dev);
 
@@ -851,7 +851,8 @@ static int __maybe_unused rzg2l_csi2_pm_runtime_resume(struct device *dev)
 }
 
 static const struct dev_pm_ops rzg2l_csi2_pm_ops = {
-	SET_RUNTIME_PM_OPS(rzg2l_csi2_pm_runtime_suspend, rzg2l_csi2_pm_runtime_resume, NULL)
+	RUNTIME_PM_OPS(rzg2l_csi2_pm_runtime_suspend,
+		       rzg2l_csi2_pm_runtime_resume, NULL)
 };
 
 static const struct of_device_id rzg2l_csi2_of_table[] = {
@@ -865,7 +866,7 @@ static struct platform_driver rzg2l_csi2_pdrv = {
 	.driver	= {
 		.name = "rzg2l-csi2",
 		.of_match_table = rzg2l_csi2_of_table,
-		.pm = &rzg2l_csi2_pm_ops,
+		.pm = pm_ptr(&rzg2l_csi2_pm_ops),
 	},
 };
 
-- 
2.25.1

[PATCH v2 RESEND 2/4] media: platform: rzg2l-cru: rzg2l-ip: Add delay after D-PHY reset

[Biju Das]

As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on the
latest hardware manual (R01UH0914EJ0140 Rev.1.40) it is mentioned that
after DPHY reset, we need to wait for 1 msec or more before start
receiving data from the sensor. So add a delay after pre_streamon().

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
v1->v2:
 * Added Rb tag from Laurent.
 * Replaced usleep_range()->fsleep().
 * Added blank space after manual in commit description.
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
index 9f351a05893e..8466b4e55909 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2022 Renesas Electronics Corp.
  */
 
+#include <linux/delay.h>
 #include "rzg2l-cru.h"
 
 struct rzg2l_cru_ip_format {
@@ -71,6 +72,8 @@ static int rzg2l_cru_ip_s_stream(struct v4l2_subdev *sd, int enable)
 		if (ret)
 			return ret;
 
+		fsleep(1000);
+
 		ret = rzg2l_cru_start_image_processing(cru);
 		if (ret) {
 			v4l2_subdev_call(cru->ip.remote, video, post_streamoff);
-- 
2.25.1

[PATCH v2 RESEND 3/4] media: platform: rzg2l-cru: rzg2l-video: Fix image processing initialization

[Biju Das]

As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on the
latest hardware manual (R01UH0914EJ0140 Rev.1.40) it is mentioned that
initialize the AXI master first and then initialize the image processing.
Fix the start procedure as per the hardware manual.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
v1->v2:
 * Added Rb tag from Laurent.
 * Added a blank space after manual in commit description.
---
 .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index d0ffa90bc656..a7d6fe831d54 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -430,13 +430,6 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
 
 	spin_lock_irqsave(&cru->qlock, flags);
 
-	/* Initialize image convert */
-	ret = rzg2l_cru_initialize_image_conv(cru, fmt);
-	if (ret) {
-		spin_unlock_irqrestore(&cru->qlock, flags);
-		return ret;
-	}
-
 	/* Select a video input */
 	rzg2l_cru_write(cru, CRUnCTRL, CRUnCTRL_VINSEL(0));
 
@@ -450,6 +443,13 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
 	/* Initialize the AXI master */
 	rzg2l_cru_initialize_axi(cru);
 
+	/* Initialize image convert */
+	ret = rzg2l_cru_initialize_image_conv(cru, fmt);
+	if (ret) {
+		spin_unlock_irqrestore(&cru->qlock, flags);
+		return ret;
+	}
+
 	/* Enable interrupt */
 	rzg2l_cru_write(cru, CRUnIE, CRUnIE_EFE);
 
-- 
2.25.1

[PATCH v2 RESEND 4/4] media: platform: rzg2l-cru: rzg2l-video: Restructure clk handling

[Biju Das]

As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on the
latest hardware manual (R01UH0914EJ0140 Rev.1.40) it is mentioned that
we need to supply all CRU clks and we need to disable the vclk before
enabling the LINK reception and enable the vclk after enabling the link
Reception. So restructure clk handling as per the HW manual.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2:
 * Dropped clk-provider.h and __clk_is_enabled() as consumer clk should
   not use it. Plan to send RFC for clk_disable_unprepare_sync() in ccf.
---
 .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  3 -
 .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   | 28 +++++---
 .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 15 +---
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 69 ++++++++-----------
 4 files changed, 47 insertions(+), 68 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
index 811603f18af0..a5a99b004322 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
@@ -133,9 +133,6 @@ struct rzg2l_cru_dev {
 	struct v4l2_pix_format format;
 };
 
-void rzg2l_cru_vclk_unprepare(struct rzg2l_cru_dev *cru);
-int rzg2l_cru_vclk_prepare(struct rzg2l_cru_dev *cru);
-
 int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru);
 void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru);
 
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
index e00d9379dd2c..e68fcdaea207 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
@@ -108,6 +108,7 @@ struct rzg2l_csi2 {
 	struct reset_control *presetn;
 	struct reset_control *cmn_rstb;
 	struct clk *sysclk;
+	struct clk *vclk;
 	unsigned long vclk_rate;
 
 	struct v4l2_subdev subdev;
@@ -361,7 +362,7 @@ static int rzg2l_csi2_dphy_setting(struct v4l2_subdev *sd, bool on)
 	return rzg2l_csi2_dphy_disable(csi2);
 }
 
-static void rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
+static int rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
 {
 	unsigned long vclk_rate = csi2->vclk_rate / HZ_PER_MHZ;
 	u32 frrskw, frrclk, frrskw_coeff, frrclk_coeff;
@@ -386,11 +387,15 @@ static void rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
 	rzg2l_csi2_write(csi2, CSI2nDTEL, 0xf778ff0f);
 	rzg2l_csi2_write(csi2, CSI2nDTEH, 0x00ffff1f);
 
+	clk_disable_unprepare(csi2->vclk);
+
 	/* Enable LINK reception */
 	rzg2l_csi2_write(csi2, CSI2nMCT3, CSI2nMCT3_RXEN);
+
+	return clk_prepare_enable(csi2->vclk);
 }
 
-static void rzg2l_csi2_mipi_link_disable(struct rzg2l_csi2 *csi2)
+static int rzg2l_csi2_mipi_link_disable(struct rzg2l_csi2 *csi2)
 {
 	unsigned int timeout = VSRSTS_RETRIES;
 
@@ -409,18 +414,21 @@ static void rzg2l_csi2_mipi_link_disable(struct rzg2l_csi2 *csi2)
 
 	if (!timeout)
 		dev_err(csi2->dev, "Clearing CSI2nRTST.VSRSTS timed out\n");
+
+	return 0;
 }
 
 static int rzg2l_csi2_mipi_link_setting(struct v4l2_subdev *sd, bool on)
 {
 	struct rzg2l_csi2 *csi2 = sd_to_csi2(sd);
+	int ret;
 
 	if (on)
-		rzg2l_csi2_mipi_link_enable(csi2);
+		ret = rzg2l_csi2_mipi_link_enable(csi2);
 	else
-		rzg2l_csi2_mipi_link_disable(csi2);
+		ret = rzg2l_csi2_mipi_link_disable(csi2);
 
-	return 0;
+	return ret;
 }
 
 static int rzg2l_csi2_s_stream(struct v4l2_subdev *sd, int enable)
@@ -731,7 +739,6 @@ static const struct media_entity_operations rzg2l_csi2_entity_ops = {
 static int rzg2l_csi2_probe(struct platform_device *pdev)
 {
 	struct rzg2l_csi2 *csi2;
-	struct clk *vclk;
 	int ret;
 
 	csi2 = devm_kzalloc(&pdev->dev, sizeof(*csi2), GFP_KERNEL);
@@ -757,12 +764,11 @@ static int rzg2l_csi2_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, PTR_ERR(csi2->sysclk),
 				     "Failed to get system clk\n");
 
-	vclk = clk_get(&pdev->dev, "video");
-	if (IS_ERR(vclk))
-		return dev_err_probe(&pdev->dev, PTR_ERR(vclk),
+	csi2->vclk = devm_clk_get(&pdev->dev, "video");
+	if (IS_ERR(csi2->vclk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(csi2->vclk),
 				     "Failed to get video clock\n");
-	csi2->vclk_rate = clk_get_rate(vclk);
-	clk_put(vclk);
+	csi2->vclk_rate = clk_get_rate(csi2->vclk);
 
 	csi2->dev = &pdev->dev;
 
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
index 8466b4e55909..2d22c373588b 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
@@ -80,20 +80,9 @@ static int rzg2l_cru_ip_s_stream(struct v4l2_subdev *sd, int enable)
 			return ret;
 		}
 
-		rzg2l_cru_vclk_unprepare(cru);
-
 		ret = v4l2_subdev_call(cru->ip.remote, video, s_stream, enable);
-		if (ret == -ENOIOCTLCMD)
-			ret = 0;
-		if (!ret) {
-			ret = rzg2l_cru_vclk_prepare(cru);
-			if (!ret)
-				return 0;
-		} else {
-			/* enable back vclk so that s_stream in error path disables it */
-			if (rzg2l_cru_vclk_prepare(cru))
-				dev_err(cru->dev, "Failed to enable vclk\n");
-		}
+		if (!ret || (ret == -ENOIOCTLCMD))
+			return 0;
 
 		s_stream_ret = ret;
 
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index a7d6fe831d54..b16b8af6e8f8 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -461,16 +461,6 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
 	return 0;
 }
 
-void rzg2l_cru_vclk_unprepare(struct rzg2l_cru_dev *cru)
-{
-	clk_disable_unprepare(cru->vclk);
-}
-
-int rzg2l_cru_vclk_prepare(struct rzg2l_cru_dev *cru)
-{
-	return clk_prepare_enable(cru->vclk);
-}
-
 static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on)
 {
 	struct media_pipeline *pipe;
@@ -499,39 +489,24 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on)
 
 		video_device_pipeline_stop(&cru->vdev);
 
-		pm_runtime_put_sync(cru->dev);
-		clk_disable_unprepare(cru->vclk);
-
 		return stream_off_ret;
 	}
 
-	ret = pm_runtime_resume_and_get(cru->dev);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare_enable(cru->vclk);
-	if (ret)
-		goto err_pm_put;
-
 	ret = rzg2l_cru_mc_validate_format(cru, sd, pad);
 	if (ret)
-		goto err_vclk_disable;
+		return ret;
 
 	pipe = media_entity_pipeline(&sd->entity) ? : &cru->vdev.pipe;
 	ret = video_device_pipeline_start(&cru->vdev, pipe);
 	if (ret)
-		goto err_vclk_disable;
+		return ret;
 
 	ret = v4l2_subdev_call(sd, video, pre_streamon, 0);
-	if (ret == -ENOIOCTLCMD)
-		ret = 0;
-	if (ret)
+	if (ret && ret != -ENOIOCTLCMD)
 		goto pipe_line_stop;
 
 	ret = v4l2_subdev_call(sd, video, s_stream, 1);
-	if (ret == -ENOIOCTLCMD)
-		ret = 0;
-	if (ret)
+	if (ret && ret != -ENOIOCTLCMD)
 		goto err_s_stream;
 
 	return 0;
@@ -542,12 +517,6 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on)
 pipe_line_stop:
 	video_device_pipeline_stop(&cru->vdev);
 
-err_vclk_disable:
-	clk_disable_unprepare(cru->vclk);
-
-err_pm_put:
-	pm_runtime_put_sync(cru->dev);
-
 	return ret;
 }
 
@@ -646,25 +615,33 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count
 	struct rzg2l_cru_dev *cru = vb2_get_drv_priv(vq);
 	int ret;
 
+	ret = pm_runtime_resume_and_get(cru->dev);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(cru->vclk);
+	if (ret)
+		goto err_pm_put;
+
 	/* Release reset state */
 	ret = reset_control_deassert(cru->aresetn);
 	if (ret) {
 		dev_err(cru->dev, "failed to deassert aresetn\n");
-		return ret;
+		goto err_vclk_disable;
 	}
 
 	ret = reset_control_deassert(cru->presetn);
 	if (ret) {
 		reset_control_assert(cru->aresetn);
 		dev_err(cru->dev, "failed to deassert presetn\n");
-		return ret;
+		goto assert_aresetn;
 	}
 
 	ret = request_irq(cru->image_conv_irq, rzg2l_cru_irq,
 			  IRQF_SHARED, KBUILD_MODNAME, cru);
 	if (ret) {
 		dev_err(cru->dev, "failed to request irq\n");
-		goto assert_resets;
+		goto assert_presetn;
 	}
 
 	/* Allocate scratch buffer. */
@@ -696,10 +673,18 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count
 free_image_conv_irq:
 	free_irq(cru->image_conv_irq, cru);
 
-assert_resets:
+assert_presetn:
 	reset_control_assert(cru->presetn);
+
+assert_aresetn:
 	reset_control_assert(cru->aresetn);
 
+err_vclk_disable:
+	clk_disable_unprepare(cru->vclk);
+
+err_pm_put:
+	pm_runtime_put_sync(cru->dev);
+
 	return ret;
 }
 
@@ -714,9 +699,11 @@ static void rzg2l_cru_stop_streaming_vq(struct vb2_queue *vq)
 			  cru->scratch, cru->scratch_phys);
 
 	free_irq(cru->image_conv_irq, cru);
-	reset_control_assert(cru->presetn);
-
 	return_unused_buffers(cru, VB2_BUF_STATE_ERROR);
+
+	reset_control_assert(cru->presetn);
+	clk_disable_unprepare(cru->vclk);
+	pm_runtime_put_sync(cru->dev);
 }
 
 static const struct vb2_ops rzg2l_cru_qops = {
-- 
2.25.1

Re: [PATCH v2 RESEND 1/4] media: platform: rzg2l-cru: rzg2l-csi2: Switch to RUNTIME_PM_OPS()

[Laurent Pinchart]

Hi Biju,

Thank you for the patch.

On Tue, Jan 30, 2024 at 04:41:12PM +0000, Biju Das wrote:
> Replace the old SET_RUNTIME_PM_OPS() helpers with its modern alternative
> RUNTIME_PM_OPS(). The usage of pm_ptr and RUNTIME_PM_OPS() allows the
> compiler to see where it's used but still drop the dead code. After this
> we can get rid of the unnecessary '__maybe_unused' annotations on PM
> functions.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
> v1->v2:
>  * Updated commit description.
>  * Aligned RUNTIME_PM_OPS() macro.
> ---
>  drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> index d20f4eff93a4..e00d9379dd2c 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> @@ -834,7 +834,7 @@ static void rzg2l_csi2_remove(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  }
>  
> -static int __maybe_unused rzg2l_csi2_pm_runtime_suspend(struct device *dev)
> +static int rzg2l_csi2_pm_runtime_suspend(struct device *dev)
>  {
>  	struct rzg2l_csi2 *csi2 = dev_get_drvdata(dev);
>  
> @@ -843,7 +843,7 @@ static int __maybe_unused rzg2l_csi2_pm_runtime_suspend(struct device *dev)
>  	return 0;
>  }
>  
> -static int __maybe_unused rzg2l_csi2_pm_runtime_resume(struct device *dev)
> +static int rzg2l_csi2_pm_runtime_resume(struct device *dev)
>  {
>  	struct rzg2l_csi2 *csi2 = dev_get_drvdata(dev);
>  
> @@ -851,7 +851,8 @@ static int __maybe_unused rzg2l_csi2_pm_runtime_resume(struct device *dev)
>  }
>  
>  static const struct dev_pm_ops rzg2l_csi2_pm_ops = {
> -	SET_RUNTIME_PM_OPS(rzg2l_csi2_pm_runtime_suspend, rzg2l_csi2_pm_runtime_resume, NULL)
> +	RUNTIME_PM_OPS(rzg2l_csi2_pm_runtime_suspend,
> +		       rzg2l_csi2_pm_runtime_resume, NULL)
>  };
>  
>  static const struct of_device_id rzg2l_csi2_of_table[] = {
> @@ -865,7 +866,7 @@ static struct platform_driver rzg2l_csi2_pdrv = {
>  	.driver	= {
>  		.name = "rzg2l-csi2",
>  		.of_match_table = rzg2l_csi2_of_table,
> -		.pm = &rzg2l_csi2_pm_ops,
> +		.pm = pm_ptr(&rzg2l_csi2_pm_ops),
>  	},
>  };
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 RESEND 4/4] media: platform: rzg2l-cru: rzg2l-video: Restructure clk handling

[Laurent Pinchart]

Hi Biju,

Thank you for the patch.

On Tue, Jan 30, 2024 at 04:41:15PM +0000, Biju Das wrote:
> As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on the
> latest hardware manual (R01UH0914EJ0140 Rev.1.40) it is mentioned that
> we need to supply all CRU clks and we need to disable the vclk before
> enabling the LINK reception and enable the vclk after enabling the link
> Reception. So restructure clk handling as per the HW manual.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v1->v2:
>  * Dropped clk-provider.h and __clk_is_enabled() as consumer clk should
>    not use it. Plan to send RFC for clk_disable_unprepare_sync() in ccf.
> ---
>  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  3 -
>  .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   | 28 +++++---
>  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 15 +---
>  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 69 ++++++++-----------
>  4 files changed, 47 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> index 811603f18af0..a5a99b004322 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> @@ -133,9 +133,6 @@ struct rzg2l_cru_dev {
>  	struct v4l2_pix_format format;
>  };
>  
> -void rzg2l_cru_vclk_unprepare(struct rzg2l_cru_dev *cru);
> -int rzg2l_cru_vclk_prepare(struct rzg2l_cru_dev *cru);
> -
>  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru);
>  void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru);
>  
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> index e00d9379dd2c..e68fcdaea207 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> @@ -108,6 +108,7 @@ struct rzg2l_csi2 {
>  	struct reset_control *presetn;
>  	struct reset_control *cmn_rstb;
>  	struct clk *sysclk;
> +	struct clk *vclk;
>  	unsigned long vclk_rate;
>  
>  	struct v4l2_subdev subdev;
> @@ -361,7 +362,7 @@ static int rzg2l_csi2_dphy_setting(struct v4l2_subdev *sd, bool on)
>  	return rzg2l_csi2_dphy_disable(csi2);
>  }
>  
> -static void rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
> +static int rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
>  {
>  	unsigned long vclk_rate = csi2->vclk_rate / HZ_PER_MHZ;
>  	u32 frrskw, frrclk, frrskw_coeff, frrclk_coeff;
> @@ -386,11 +387,15 @@ static void rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
>  	rzg2l_csi2_write(csi2, CSI2nDTEL, 0xf778ff0f);
>  	rzg2l_csi2_write(csi2, CSI2nDTEH, 0x00ffff1f);
>  
> +	clk_disable_unprepare(csi2->vclk);
> +
>  	/* Enable LINK reception */
>  	rzg2l_csi2_write(csi2, CSI2nMCT3, CSI2nMCT3_RXEN);
> +
> +	return clk_prepare_enable(csi2->vclk);
>  }
>  
> -static void rzg2l_csi2_mipi_link_disable(struct rzg2l_csi2 *csi2)
> +static int rzg2l_csi2_mipi_link_disable(struct rzg2l_csi2 *csi2)
>  {
>  	unsigned int timeout = VSRSTS_RETRIES;
>  
> @@ -409,18 +414,21 @@ static void rzg2l_csi2_mipi_link_disable(struct rzg2l_csi2 *csi2)
>  
>  	if (!timeout)
>  		dev_err(csi2->dev, "Clearing CSI2nRTST.VSRSTS timed out\n");
> +
> +	return 0;
>  }
>  
>  static int rzg2l_csi2_mipi_link_setting(struct v4l2_subdev *sd, bool on)
>  {
>  	struct rzg2l_csi2 *csi2 = sd_to_csi2(sd);
> +	int ret;
>  
>  	if (on)
> -		rzg2l_csi2_mipi_link_enable(csi2);
> +		ret = rzg2l_csi2_mipi_link_enable(csi2);
>  	else
> -		rzg2l_csi2_mipi_link_disable(csi2);
> +		ret = rzg2l_csi2_mipi_link_disable(csi2);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int rzg2l_csi2_s_stream(struct v4l2_subdev *sd, int enable)
> @@ -731,7 +739,6 @@ static const struct media_entity_operations rzg2l_csi2_entity_ops = {
>  static int rzg2l_csi2_probe(struct platform_device *pdev)
>  {
>  	struct rzg2l_csi2 *csi2;
> -	struct clk *vclk;
>  	int ret;
>  
>  	csi2 = devm_kzalloc(&pdev->dev, sizeof(*csi2), GFP_KERNEL);
> @@ -757,12 +764,11 @@ static int rzg2l_csi2_probe(struct platform_device *pdev)
>  		return dev_err_probe(&pdev->dev, PTR_ERR(csi2->sysclk),
>  				     "Failed to get system clk\n");
>  
> -	vclk = clk_get(&pdev->dev, "video");
> -	if (IS_ERR(vclk))
> -		return dev_err_probe(&pdev->dev, PTR_ERR(vclk),
> +	csi2->vclk = devm_clk_get(&pdev->dev, "video");
> +	if (IS_ERR(csi2->vclk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(csi2->vclk),
>  				     "Failed to get video clock\n");
> -	csi2->vclk_rate = clk_get_rate(vclk);
> -	clk_put(vclk);
> +	csi2->vclk_rate = clk_get_rate(csi2->vclk);
>  
>  	csi2->dev = &pdev->dev;
>  
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> index 8466b4e55909..2d22c373588b 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> @@ -80,20 +80,9 @@ static int rzg2l_cru_ip_s_stream(struct v4l2_subdev *sd, int enable)
>  			return ret;
>  		}
>  
> -		rzg2l_cru_vclk_unprepare(cru);
> -
>  		ret = v4l2_subdev_call(cru->ip.remote, video, s_stream, enable);
> -		if (ret == -ENOIOCTLCMD)
> -			ret = 0;
> -		if (!ret) {
> -			ret = rzg2l_cru_vclk_prepare(cru);
> -			if (!ret)
> -				return 0;
> -		} else {
> -			/* enable back vclk so that s_stream in error path disables it */
> -			if (rzg2l_cru_vclk_prepare(cru))
> -				dev_err(cru->dev, "Failed to enable vclk\n");
> -		}
> +		if (!ret || (ret == -ENOIOCTLCMD))
> +			return 0;
>  
>  		s_stream_ret = ret;
>  
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index a7d6fe831d54..b16b8af6e8f8 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -461,16 +461,6 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
>  	return 0;
>  }
>  
> -void rzg2l_cru_vclk_unprepare(struct rzg2l_cru_dev *cru)
> -{
> -	clk_disable_unprepare(cru->vclk);
> -}
> -
> -int rzg2l_cru_vclk_prepare(struct rzg2l_cru_dev *cru)
> -{
> -	return clk_prepare_enable(cru->vclk);
> -}
> -
>  static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on)
>  {
>  	struct media_pipeline *pipe;
> @@ -499,39 +489,24 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on)
>  
>  		video_device_pipeline_stop(&cru->vdev);
>  
> -		pm_runtime_put_sync(cru->dev);
> -		clk_disable_unprepare(cru->vclk);
> -
>  		return stream_off_ret;
>  	}
>  
> -	ret = pm_runtime_resume_and_get(cru->dev);
> -	if (ret)
> -		return ret;
> -
> -	ret = clk_prepare_enable(cru->vclk);
> -	if (ret)
> -		goto err_pm_put;
> -

Why is this moved to rzg2l_cru_start_streaming_vq() ? At the very least
the commit message should explain it, but it may be best to split it to
a separate patch.

>  	ret = rzg2l_cru_mc_validate_format(cru, sd, pad);
>  	if (ret)
> -		goto err_vclk_disable;
> +		return ret;
>  
>  	pipe = media_entity_pipeline(&sd->entity) ? : &cru->vdev.pipe;
>  	ret = video_device_pipeline_start(&cru->vdev, pipe);
>  	if (ret)
> -		goto err_vclk_disable;
> +		return ret;
>  
>  	ret = v4l2_subdev_call(sd, video, pre_streamon, 0);
> -	if (ret == -ENOIOCTLCMD)
> -		ret = 0;
> -	if (ret)
> +	if (ret && ret != -ENOIOCTLCMD)
>  		goto pipe_line_stop;
>  
>  	ret = v4l2_subdev_call(sd, video, s_stream, 1);
> -	if (ret == -ENOIOCTLCMD)
> -		ret = 0;
> -	if (ret)
> +	if (ret && ret != -ENOIOCTLCMD)
>  		goto err_s_stream;
>  
>  	return 0;
> @@ -542,12 +517,6 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on)
>  pipe_line_stop:
>  	video_device_pipeline_stop(&cru->vdev);
>  
> -err_vclk_disable:
> -	clk_disable_unprepare(cru->vclk);
> -
> -err_pm_put:
> -	pm_runtime_put_sync(cru->dev);
> -
>  	return ret;
>  }
>  
> @@ -646,25 +615,33 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count
>  	struct rzg2l_cru_dev *cru = vb2_get_drv_priv(vq);
>  	int ret;
>  
> +	ret = pm_runtime_resume_and_get(cru->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(cru->vclk);
> +	if (ret)
> +		goto err_pm_put;
> +
>  	/* Release reset state */
>  	ret = reset_control_deassert(cru->aresetn);
>  	if (ret) {
>  		dev_err(cru->dev, "failed to deassert aresetn\n");
> -		return ret;
> +		goto err_vclk_disable;
>  	}
>  
>  	ret = reset_control_deassert(cru->presetn);
>  	if (ret) {
>  		reset_control_assert(cru->aresetn);
>  		dev_err(cru->dev, "failed to deassert presetn\n");
> -		return ret;
> +		goto assert_aresetn;
>  	}
>  
>  	ret = request_irq(cru->image_conv_irq, rzg2l_cru_irq,
>  			  IRQF_SHARED, KBUILD_MODNAME, cru);
>  	if (ret) {
>  		dev_err(cru->dev, "failed to request irq\n");
> -		goto assert_resets;
> +		goto assert_presetn;
>  	}
>  
>  	/* Allocate scratch buffer. */
> @@ -696,10 +673,18 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count
>  free_image_conv_irq:
>  	free_irq(cru->image_conv_irq, cru);
>  
> -assert_resets:
> +assert_presetn:
>  	reset_control_assert(cru->presetn);
> +
> +assert_aresetn:
>  	reset_control_assert(cru->aresetn);
>  
> +err_vclk_disable:
> +	clk_disable_unprepare(cru->vclk);
> +
> +err_pm_put:
> +	pm_runtime_put_sync(cru->dev);
> +
>  	return ret;
>  }
>  
> @@ -714,9 +699,11 @@ static void rzg2l_cru_stop_streaming_vq(struct vb2_queue *vq)
>  			  cru->scratch, cru->scratch_phys);
>  
>  	free_irq(cru->image_conv_irq, cru);
> -	reset_control_assert(cru->presetn);
> -
>  	return_unused_buffers(cru, VB2_BUF_STATE_ERROR);
> +
> +	reset_control_assert(cru->presetn);
> +	clk_disable_unprepare(cru->vclk);
> +	pm_runtime_put_sync(cru->dev);
>  }
>  
>  static const struct vb2_ops rzg2l_cru_qops = {

-- 
Regards,

Laurent Pinchart

RE: [PATCH v2 RESEND 4/4] media: platform: rzg2l-cru: rzg2l-video: Restructure clk handling

[Biju Das]

Hi Laurent Pinchart,

Thanks for the feedback.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Wednesday, February 7, 2024 10:00 PM
> Subject: Re: [PATCH v2 RESEND 4/4] media: platform: rzg2l-cru: rzg2l-
> video: Restructure clk handling
> 
> Hi Biju,
> 
> Thank you for the patch.
> 
> On Tue, Jan 30, 2024 at 04:41:15PM +0000, Biju Das wrote:
> > As per section 35.3.1 Starting Reception for the MIPI CSI-2 Input on
> > the latest hardware manual (R01UH0914EJ0140 Rev.1.40) it is mentioned
> > that we need to supply all CRU clks and we need to disable the vclk
> > before enabling the LINK reception and enable the vclk after enabling
> > the link Reception. So restructure clk handling as per the HW manual.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v1->v2:
> >  * Dropped clk-provider.h and __clk_is_enabled() as consumer clk should
> >    not use it. Plan to send RFC for clk_disable_unprepare_sync() in ccf.
> > ---
> >  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  3 -
> >  .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   | 28 +++++---
> >  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 15 +---
> >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 69
> > ++++++++-----------
> >  4 files changed, 47 insertions(+), 68 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > index 811603f18af0..a5a99b004322 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > @@ -133,9 +133,6 @@ struct rzg2l_cru_dev {
> >  	struct v4l2_pix_format format;
> >  };
> >
> > -void rzg2l_cru_vclk_unprepare(struct rzg2l_cru_dev *cru); -int
> > rzg2l_cru_vclk_prepare(struct rzg2l_cru_dev *cru);
> > -
> >  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru);
> > void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru);
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > index e00d9379dd2c..e68fcdaea207 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > @@ -108,6 +108,7 @@ struct rzg2l_csi2 {
> >  	struct reset_control *presetn;
> >  	struct reset_control *cmn_rstb;
> >  	struct clk *sysclk;
> > +	struct clk *vclk;
> >  	unsigned long vclk_rate;
> >
> >  	struct v4l2_subdev subdev;
> > @@ -361,7 +362,7 @@ static int rzg2l_csi2_dphy_setting(struct
> v4l2_subdev *sd, bool on)
> >  	return rzg2l_csi2_dphy_disable(csi2);  }
> >
> > -static void rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
> > +static int rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
> >  {
> >  	unsigned long vclk_rate = csi2->vclk_rate / HZ_PER_MHZ;
> >  	u32 frrskw, frrclk, frrskw_coeff, frrclk_coeff; @@ -386,11 +387,15
> > @@ static void rzg2l_csi2_mipi_link_enable(struct rzg2l_csi2 *csi2)
> >  	rzg2l_csi2_write(csi2, CSI2nDTEL, 0xf778ff0f);
> >  	rzg2l_csi2_write(csi2, CSI2nDTEH, 0x00ffff1f);
> >
> > +	clk_disable_unprepare(csi2->vclk);
> > +
> >  	/* Enable LINK reception */
> >  	rzg2l_csi2_write(csi2, CSI2nMCT3, CSI2nMCT3_RXEN);
> > +
> > +	return clk_prepare_enable(csi2->vclk);
> >  }
> >
> > -static void rzg2l_csi2_mipi_link_disable(struct rzg2l_csi2 *csi2)
> > +static int rzg2l_csi2_mipi_link_disable(struct rzg2l_csi2 *csi2)
> >  {
> >  	unsigned int timeout = VSRSTS_RETRIES;
> >
> > @@ -409,18 +414,21 @@ static void rzg2l_csi2_mipi_link_disable(struct
> > rzg2l_csi2 *csi2)
> >
> >  	if (!timeout)
> >  		dev_err(csi2->dev, "Clearing CSI2nRTST.VSRSTS timed out\n");
> > +
> > +	return 0;
> >  }
> >
> >  static int rzg2l_csi2_mipi_link_setting(struct v4l2_subdev *sd, bool
> > on)  {
> >  	struct rzg2l_csi2 *csi2 = sd_to_csi2(sd);
> > +	int ret;
> >
> >  	if (on)
> > -		rzg2l_csi2_mipi_link_enable(csi2);
> > +		ret = rzg2l_csi2_mipi_link_enable(csi2);
> >  	else
> > -		rzg2l_csi2_mipi_link_disable(csi2);
> > +		ret = rzg2l_csi2_mipi_link_disable(csi2);
> >
> > -	return 0;
> > +	return ret;
> >  }
> >
> >  static int rzg2l_csi2_s_stream(struct v4l2_subdev *sd, int enable) @@
> > -731,7 +739,6 @@ static const struct media_entity_operations
> > rzg2l_csi2_entity_ops = {  static int rzg2l_csi2_probe(struct
> > platform_device *pdev)  {
> >  	struct rzg2l_csi2 *csi2;
> > -	struct clk *vclk;
> >  	int ret;
> >
> >  	csi2 = devm_kzalloc(&pdev->dev, sizeof(*csi2), GFP_KERNEL); @@
> > -757,12 +764,11 @@ static int rzg2l_csi2_probe(struct platform_device
> *pdev)
> >  		return dev_err_probe(&pdev->dev, PTR_ERR(csi2->sysclk),
> >  				     "Failed to get system clk\n");
> >
> > -	vclk = clk_get(&pdev->dev, "video");
> > -	if (IS_ERR(vclk))
> > -		return dev_err_probe(&pdev->dev, PTR_ERR(vclk),
> > +	csi2->vclk = devm_clk_get(&pdev->dev, "video");
> > +	if (IS_ERR(csi2->vclk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(csi2->vclk),
> >  				     "Failed to get video clock\n");
> > -	csi2->vclk_rate = clk_get_rate(vclk);
> > -	clk_put(vclk);
> > +	csi2->vclk_rate = clk_get_rate(csi2->vclk);
> >
> >  	csi2->dev = &pdev->dev;
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > index 8466b4e55909..2d22c373588b 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > @@ -80,20 +80,9 @@ static int rzg2l_cru_ip_s_stream(struct v4l2_subdev
> *sd, int enable)
> >  			return ret;
> >  		}
> >
> > -		rzg2l_cru_vclk_unprepare(cru);
> > -
> >  		ret = v4l2_subdev_call(cru->ip.remote, video, s_stream,
> enable);
> > -		if (ret == -ENOIOCTLCMD)
> > -			ret = 0;
> > -		if (!ret) {
> > -			ret = rzg2l_cru_vclk_prepare(cru);
> > -			if (!ret)
> > -				return 0;
> > -		} else {
> > -			/* enable back vclk so that s_stream in error path
> disables it */
> > -			if (rzg2l_cru_vclk_prepare(cru))
> > -				dev_err(cru->dev, "Failed to enable vclk\n");
> > -		}
> > +		if (!ret || (ret == -ENOIOCTLCMD))
> > +			return 0;
> >
> >  		s_stream_ret = ret;
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > index a7d6fe831d54..b16b8af6e8f8 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > @@ -461,16 +461,6 @@ int rzg2l_cru_start_image_processing(struct
> rzg2l_cru_dev *cru)
> >  	return 0;
> >  }
> >
> > -void rzg2l_cru_vclk_unprepare(struct rzg2l_cru_dev *cru) -{
> > -	clk_disable_unprepare(cru->vclk);
> > -}
> > -
> > -int rzg2l_cru_vclk_prepare(struct rzg2l_cru_dev *cru) -{
> > -	return clk_prepare_enable(cru->vclk);
> > -}
> > -
> >  static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on)  {
> >  	struct media_pipeline *pipe;
> > @@ -499,39 +489,24 @@ static int rzg2l_cru_set_stream(struct
> > rzg2l_cru_dev *cru, int on)
> >
> >  		video_device_pipeline_stop(&cru->vdev);
> >
> > -		pm_runtime_put_sync(cru->dev);
> > -		clk_disable_unprepare(cru->vclk);
> > -
> >  		return stream_off_ret;
> >  	}
> >
> > -	ret = pm_runtime_resume_and_get(cru->dev);
> > -	if (ret)
> > -		return ret;
> > -
> > -	ret = clk_prepare_enable(cru->vclk);
> > -	if (ret)
> > -		goto err_pm_put;
> > -
> 
> Why is this moved to rzg2l_cru_start_streaming_vq() ? At the very least
> the commit message should explain it, but it may be best to split it to a
> separate patch.

As per section "35.3.1 Starting Reception for the MIPI CSI-2 Input", we need
to supply all the clocks and then release the CRU from reset state. Currently
it is doing other way around.

OK will split this and send as separate patch.

Cheers,
Biju

> 
> >  	ret = rzg2l_cru_mc_validate_format(cru, sd, pad);
> >  	if (ret)
> > -		goto err_vclk_disable;
> > +		return ret;
> >
> >  	pipe = media_entity_pipeline(&sd->entity) ? : &cru->vdev.pipe;
> >  	ret = video_device_pipeline_start(&cru->vdev, pipe);
> >  	if (ret)
> > -		goto err_vclk_disable;
> > +		return ret;
> >
> >  	ret = v4l2_subdev_call(sd, video, pre_streamon, 0);
> > -	if (ret == -ENOIOCTLCMD)
> > -		ret = 0;
> > -	if (ret)
> > +	if (ret && ret != -ENOIOCTLCMD)
> >  		goto pipe_line_stop;
> >
> >  	ret = v4l2_subdev_call(sd, video, s_stream, 1);
> > -	if (ret == -ENOIOCTLCMD)
> > -		ret = 0;
> > -	if (ret)
> > +	if (ret && ret != -ENOIOCTLCMD)
> >  		goto err_s_stream;
> >
> >  	return 0;
> > @@ -542,12 +517,6 @@ static int rzg2l_cru_set_stream(struct
> > rzg2l_cru_dev *cru, int on)
> >  pipe_line_stop:
> >  	video_device_pipeline_stop(&cru->vdev);
> >
> > -err_vclk_disable:
> > -	clk_disable_unprepare(cru->vclk);
> > -
> > -err_pm_put:
> > -	pm_runtime_put_sync(cru->dev);
> > -
> >  	return ret;
> >  }
> >
> > @@ -646,25 +615,33 @@ static int rzg2l_cru_start_streaming_vq(struct
> vb2_queue *vq, unsigned int count
> >  	struct rzg2l_cru_dev *cru = vb2_get_drv_priv(vq);
> >  	int ret;
> >
> > +	ret = pm_runtime_resume_and_get(cru->dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_prepare_enable(cru->vclk);
> > +	if (ret)
> > +		goto err_pm_put;
> > +
> >  	/* Release reset state */
> >  	ret = reset_control_deassert(cru->aresetn);
> >  	if (ret) {
> >  		dev_err(cru->dev, "failed to deassert aresetn\n");
> > -		return ret;
> > +		goto err_vclk_disable;
> >  	}
> >
> >  	ret = reset_control_deassert(cru->presetn);
> >  	if (ret) {
> >  		reset_control_assert(cru->aresetn);
> >  		dev_err(cru->dev, "failed to deassert presetn\n");
> > -		return ret;
> > +		goto assert_aresetn;
> >  	}
> >
> >  	ret = request_irq(cru->image_conv_irq, rzg2l_cru_irq,
> >  			  IRQF_SHARED, KBUILD_MODNAME, cru);
> >  	if (ret) {
> >  		dev_err(cru->dev, "failed to request irq\n");
> > -		goto assert_resets;
> > +		goto assert_presetn;
> >  	}
> >
> >  	/* Allocate scratch buffer. */
> > @@ -696,10 +673,18 @@ static int rzg2l_cru_start_streaming_vq(struct
> > vb2_queue *vq, unsigned int count
> >  free_image_conv_irq:
> >  	free_irq(cru->image_conv_irq, cru);
> >
> > -assert_resets:
> > +assert_presetn:
> >  	reset_control_assert(cru->presetn);
> > +
> > +assert_aresetn:
> >  	reset_control_assert(cru->aresetn);
> >
> > +err_vclk_disable:
> > +	clk_disable_unprepare(cru->vclk);
> > +
> > +err_pm_put:
> > +	pm_runtime_put_sync(cru->dev);
> > +
> >  	return ret;
> >  }
> >
> > @@ -714,9 +699,11 @@ static void rzg2l_cru_stop_streaming_vq(struct
> vb2_queue *vq)
> >  			  cru->scratch, cru->scratch_phys);
> >
> >  	free_irq(cru->image_conv_irq, cru);
> > -	reset_control_assert(cru->presetn);
> > -
> >  	return_unused_buffers(cru, VB2_BUF_STATE_ERROR);
> > +
> > +	reset_control_assert(cru->presetn);
> > +	clk_disable_unprepare(cru->vclk);
> > +	pm_runtime_put_sync(cru->dev);
> >  }
> >
> >  static const struct vb2_ops rzg2l_cru_qops = {
> 
> --
> Regards,
> 
> Laurent Pinchart