Re: [PATCH v8 16/18] media: cadence: csi2rx: Support runtime PM

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

On 01/12/25 18:41, Tomi Valkeinen wrote:
> Hi,


Hi Tomi,

Thank you for the review !

>
> On 12/11/2025 13:54, 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.
>>
>> 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>
>> 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 | 129 ++++++++++++-------
>>   2 files changed, 83 insertions(+), 47 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 833bc134f17cb..6447c225ba354 100644
>> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
>> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
>> @@ -337,11 +337,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)
>> @@ -382,7 +377,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;
>>   		}
>>   	}
>>   
>> @@ -397,12 +392,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]),
>> @@ -415,30 +404,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)
>> @@ -447,10 +414,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++) {
>> @@ -465,14 +428,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);
>>   
>> @@ -548,10 +505,17 @@ 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)
>> +			return ret;
>> +
>>   		csi2rx_update_vc_select(csi2rx, state);
>> +
>>   		ret = csi2rx_start(csi2rx);
>> -		if (ret)
>> +		if (ret) {
>> +			pm_runtime_put(csi2rx->dev);
>>   			return ret;
>> +		}
>>   	}
>>   
>>   	/* Start streaming on the source */
>> @@ -561,8 +525,10 @@ 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)
>> +		if (!csi2rx->count) {
>>   			csi2rx_stop(csi2rx);
>> +			pm_runtime_put(csi2rx->dev);
> Probably time to add 'goto' based error handling to this func... Maybe
> in this patch, maybe as a separate patch before this.


Okay, I will do it in this patch itself.


Rishikesh

>
>   Tomi
>
>> +		}
>>   		return ret;
>>   	}
>>   
>> @@ -589,8 +555,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;
>>   }
>> @@ -1092,6 +1060,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;
>> @@ -1106,6 +1075,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);
>> @@ -1124,9 +1094,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++) {
>> +		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--) {
>> +		reset_control_assert(csi2rx->pixel_rst[i - 1]);
>> +		clk_disable_unprepare(csi2rx->pixel_clk[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" },
>> @@ -1141,6 +1175,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);