Re: [PATCH v12 15/17] media: cadence: csi2rx: Support runtime PM

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Rishikes,

On Fri, Mar 13, 2026 at 02:36:59PM +0530, Rishikesh Donadkar wrote:
> From: Changhuang Liang <changhuang.liang@starfivetech.com>
> 
> Use runtime power management hooks to save power when CSI-RX is not in
> use. Also, shift to goto based error handling in
> csi2rx_enable_streams() function
> 
> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> Tested-by: Rishikesh Donadkar <r-donadkar@ti.com>
> Reviewed-by: Rishikesh Donadkar <r-donadkar@ti.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
> ---
>  drivers/media/platform/cadence/Kconfig       |   1 +
>  drivers/media/platform/cadence/cdns-csi2rx.c | 136 ++++++++++++-------
>  2 files changed, 88 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/media/platform/cadence/Kconfig b/drivers/media/platform/cadence/Kconfig
> index 1aa608c00dbce..ea85ef82760e6 100644
> --- a/drivers/media/platform/cadence/Kconfig
> +++ b/drivers/media/platform/cadence/Kconfig
> @@ -5,6 +5,7 @@ comment "Cadence media platform drivers"
>  config VIDEO_CADENCE_CSI2RX
>  	tristate "Cadence MIPI-CSI2 RX Controller"
>  	depends on VIDEO_DEV
> +	depends on PM
>  	select MEDIA_CONTROLLER
>  	select VIDEO_V4L2_SUBDEV_API
>  	select V4L2_FWNODE
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> index f34df341a2cac..18737d00a7d7a 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -340,11 +340,6 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>  	u32 reg;
>  	int ret;
>  
> -	ret = clk_prepare_enable(csi2rx->p_clk);
> -	if (ret)
> -		return ret;
> -
> -	reset_control_deassert(csi2rx->p_rst);
>  	csi2rx_reset(csi2rx);
>  
>  	if (csi2rx->error_irq >= 0)
> @@ -385,7 +380,7 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>  		if (ret) {
>  			dev_err(csi2rx->dev,
>  				"Failed to configure external DPHY: %d\n", ret);
> -			goto err_disable_pclk;
> +			return ret;
>  		}
>  	}
>  
> @@ -400,12 +395,6 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>  	 * hence the reference counting.
>  	 */
>  	for (i = 0; i < csi2rx->max_streams; i++) {
> -		ret = clk_prepare_enable(csi2rx->pixel_clk[i]);
> -		if (ret)
> -			goto err_disable_pixclk;
> -
> -		reset_control_deassert(csi2rx->pixel_rst[i]);
> -
>  		writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF |
>  			       FIELD_PREP(CSI2RX_STREAM_CFG_NUM_PIXELS_MASK,
>  					  csi2rx->num_pixels[i]),
> @@ -418,30 +407,8 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>  		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
>  	}
>  
> -	ret = clk_prepare_enable(csi2rx->sys_clk);
> -	if (ret)
> -		goto err_disable_pixclk;
> -
> -	reset_control_deassert(csi2rx->sys_rst);
> -
> -	clk_disable_unprepare(csi2rx->p_clk);
>  
>  	return 0;
> -
> -err_disable_pixclk:
> -	for (; i > 0; i--) {
> -		reset_control_assert(csi2rx->pixel_rst[i - 1]);
> -		clk_disable_unprepare(csi2rx->pixel_clk[i - 1]);
> -	}
> -
> -	if (csi2rx->dphy) {
> -		writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG);
> -		phy_power_off(csi2rx->dphy);
> -	}
> -err_disable_pclk:
> -	clk_disable_unprepare(csi2rx->p_clk);
> -
> -	return ret;
>  }
>  
>  static void csi2rx_stop(struct csi2rx_priv *csi2rx)
> @@ -450,10 +417,6 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
>  	u32 val;
>  	int ret;
>  
> -	clk_prepare_enable(csi2rx->p_clk);
> -	reset_control_assert(csi2rx->sys_rst);
> -	clk_disable_unprepare(csi2rx->sys_clk);
> -
>  	writel(0, csi2rx->base + CSI2RX_ERROR_IRQS_MASK_REG);
>  
>  	for (i = 0; i < csi2rx->max_streams; i++) {
> @@ -468,14 +431,8 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
>  		if (ret)
>  			dev_warn(csi2rx->dev,
>  				 "Failed to stop streaming on pad%u\n", i);
> -
> -		reset_control_assert(csi2rx->pixel_rst[i]);
> -		clk_disable_unprepare(csi2rx->pixel_clk[i]);
>  	}
>  
> -	reset_control_assert(csi2rx->p_rst);
> -	clk_disable_unprepare(csi2rx->p_clk);
> -
>  	if (csi2rx->dphy) {
>  		writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG);
>  
> @@ -549,10 +506,15 @@ static int csi2rx_enable_streams(struct v4l2_subdev *subdev,
>  	 * enable the whole controller.
>  	 */
>  	if (!csi2rx->count) {
> +		ret = pm_runtime_resume_and_get(csi2rx->dev);
> +		if (ret < 0)
> +			goto err;
> +
>  		csi2rx_update_vc_select(csi2rx, state);
> +
>  		ret = csi2rx_start(csi2rx);
>  		if (ret)
> -			return ret;
> +			goto err_put_pm;
>  	}
>  
>  	/* Start streaming on the source */
> @@ -562,13 +524,20 @@ static int csi2rx_enable_streams(struct v4l2_subdev *subdev,
>  		dev_err(csi2rx->dev,
>  			"Failed to start streams %#llx on subdev\n",
>  			sink_streams);
> -		if (!csi2rx->count)
> -			csi2rx_stop(csi2rx);
> -		return ret;
> +		goto err_stop_csi;
>  	}
>  
>  	csi2rx->count++;
>  	return 0;
> +
> +err_stop_csi:
> +	if (!csi2rx->count)
> +		csi2rx_stop(csi2rx);
> +err_put_pm:
> +	if (!csi2rx->count)
> +		pm_runtime_put(csi2rx->dev);
> +err:
> +	return ret;
>  }
>  
>  static int csi2rx_disable_streams(struct v4l2_subdev *subdev,
> @@ -590,8 +559,10 @@ static int csi2rx_disable_streams(struct v4l2_subdev *subdev,
>  	csi2rx->count--;
>  
>  	/* Let the last user turn off the lights. */
> -	if (!csi2rx->count)
> +	if (!csi2rx->count) {
>  		csi2rx_stop(csi2rx);
> +		pm_runtime_put(csi2rx->dev);
> +	}
>  
>  	return 0;
>  }
> @@ -1078,6 +1049,7 @@ static int csi2rx_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_cleanup;
>  
> +	pm_runtime_enable(csi2rx->dev);
>  	ret = v4l2_async_register_subdev(&csi2rx->subdev);
>  	if (ret < 0)
>  		goto err_free_state;
> @@ -1092,6 +1064,7 @@ static int csi2rx_probe(struct platform_device *pdev)
>  
>  err_free_state:
>  	v4l2_subdev_cleanup(&csi2rx->subdev);
> +	pm_runtime_disable(csi2rx->dev);
>  err_cleanup:
>  	v4l2_async_nf_unregister(&csi2rx->notifier);
>  	v4l2_async_nf_cleanup(&csi2rx->notifier);
> @@ -1110,9 +1083,73 @@ static void csi2rx_remove(struct platform_device *pdev)
>  	v4l2_async_unregister_subdev(&csi2rx->subdev);
>  	v4l2_subdev_cleanup(&csi2rx->subdev);
>  	media_entity_cleanup(&csi2rx->subdev.entity);
> +	pm_runtime_disable(csi2rx->dev);
>  	kfree(csi2rx);
>  }
>  
> +static int csi2rx_runtime_suspend(struct device *dev)
> +{
> +	struct csi2rx_priv *csi2rx = dev_get_drvdata(dev);
> +	unsigned int i;
> +
> +	reset_control_assert(csi2rx->sys_rst);
> +	clk_disable_unprepare(csi2rx->sys_clk);
> +
> +	for (i = 0; i < csi2rx->max_streams; i++) {

You could declare i here.

> +		reset_control_assert(csi2rx->pixel_rst[i]);
> +		clk_disable_unprepare(csi2rx->pixel_clk[i]);
> +	}
> +
> +	reset_control_assert(csi2rx->p_rst);
> +	clk_disable_unprepare(csi2rx->p_clk);
> +
> +	return 0;
> +}
> +
> +static int csi2rx_runtime_resume(struct device *dev)
> +{
> +	struct csi2rx_priv *csi2rx = dev_get_drvdata(dev);
> +	unsigned int i;
> +	int ret;
> +
> +	ret = clk_prepare_enable(csi2rx->p_clk);
> +	if (ret)
> +		return ret;
> +
> +	reset_control_deassert(csi2rx->p_rst);
> +
> +	for (i = 0; i < csi2rx->max_streams; i++) {
> +		ret = clk_prepare_enable(csi2rx->pixel_clk[i]);
> +		if (ret)
> +			goto err_disable_pixclk;
> +
> +		reset_control_deassert(csi2rx->pixel_rst[i]);
> +	}
> +
> +	ret = clk_prepare_enable(csi2rx->sys_clk);
> +	if (ret)
> +		goto err_disable_pixclk;
> +
> +	reset_control_deassert(csi2rx->sys_rst);
> +
> +	return 0;
> +
> +err_disable_pixclk:
> +	for (; i > 0; i--) {

	while (i--) {

?

> +		reset_control_assert(csi2rx->pixel_rst[i - 1]);
> +		clk_disable_unprepare(csi2rx->pixel_clk[i - 1]);

Then you can refer to i here instead instead of i - 1.

> +	}
> +
> +	reset_control_assert(csi2rx->p_rst);
> +	clk_disable_unprepare(csi2rx->p_clk);
> +
> +	return ret;
> +}
> +
> +static const struct dev_pm_ops csi2rx_pm_ops = {
> +	RUNTIME_PM_OPS(csi2rx_runtime_suspend, csi2rx_runtime_resume, NULL)
> +};
> +
>  static const struct of_device_id csi2rx_of_table[] = {
>  	{ .compatible = "starfive,jh7110-csi2rx" },
>  	{ .compatible = "cdns,csi2rx" },
> @@ -1127,6 +1164,7 @@ static struct platform_driver csi2rx_driver = {
>  	.driver	= {
>  		.name		= "cdns-csi2rx",
>  		.of_match_table	= csi2rx_of_table,
> +		.pm		= &csi2rx_pm_ops,
>  	},
>  };
>  module_platform_driver(csi2rx_driver);

-- 
Regards,

Sakari Ailus