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

[Rishikesh Donadkar <r-donadkar@ti.com>]

On 19/05/26 17:59, Sakari Ailus wrote:
> Hi Rishikes,

Hi Sakari,

Thank you for the review !

>
> 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--) {
>
> ?


Yes, will do this


Rishikesh

>
>> +		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);