[PATCH 0/2] media: imx-mipi-csis: csis clock fixes

[PATCH 0/2] media: imx-mipi-csis: csis clock fixes

[Tomi Valkeinen]

Two fixes to the csis driver: One to fix remove() another to only enable
the clocks when needed.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
Tomi Valkeinen (2):
      media: imx-mipi-csis: Fix clock handling in remove()
      media: imx-mipi-csis: Drop extra clock enable at probe()

 drivers/media/platform/nxp/imx-mipi-csis.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)
---
base-commit: 1865913dd590ca6d5e3207b15099a1210dd62f29
change-id: 20231122-imx-csis-79d201dd51b9

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

[PATCH 1/2] media: imx-mipi-csis: Fix clock handling in remove()

[Tomi Valkeinen]

The driver always calls mipi_csis_runtime_suspend() and
mipi_csis_clk_disable() in remove(). This causes multiple WARNs from the
kernel, as the clocks get disabled too many times.

Fix the remove() to call mipi_csis_runtime_suspend() and
mipi_csis_clk_disable() in a way that reverses what is done in probe().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 6cb20b45e0a1..b39d7aeba750 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -1502,8 +1502,10 @@ static void mipi_csis_remove(struct platform_device *pdev)
 	v4l2_async_nf_cleanup(&csis->notifier);
 	v4l2_async_unregister_subdev(&csis->sd);
 
+	if (!pm_runtime_enabled(&pdev->dev))
+		mipi_csis_runtime_suspend(&pdev->dev);
+
 	pm_runtime_disable(&pdev->dev);
-	mipi_csis_runtime_suspend(&pdev->dev);
 	mipi_csis_clk_disable(csis);
 	v4l2_subdev_cleanup(&csis->sd);
 	media_entity_cleanup(&csis->sd.entity);

-- 
2.34.1

[PATCH 2/2] media: imx-mipi-csis: Drop extra clock enable at probe()

[Tomi Valkeinen]

The driver always enables the clocks at probe() and disables them only
at remove(). It is not clear why the driver does this, as it supports
runtime PM, and enables and disables the clocks in the runtime resume
and suspend callbacks. Also, in the case runtime PM is not available,
the driver calls the resume and suspend callbacks manually from probe()
and remove().

Drop the unnecessary clock enable, thus enabling the clocks only when
actually needed.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index b39d7aeba750..b08f6d2e7516 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -1435,24 +1435,18 @@ static int mipi_csis_probe(struct platform_device *pdev)
 	/* Reset PHY and enable the clocks. */
 	mipi_csis_phy_reset(csis);
 
-	ret = mipi_csis_clk_enable(csis);
-	if (ret < 0) {
-		dev_err(csis->dev, "failed to enable clocks: %d\n", ret);
-		return ret;
-	}
-
 	/* Now that the hardware is initialized, request the interrupt. */
 	ret = devm_request_irq(dev, irq, mipi_csis_irq_handler, 0,
 			       dev_name(dev), csis);
 	if (ret) {
 		dev_err(dev, "Interrupt request failed\n");
-		goto err_disable_clock;
+		return ret;
 	}
 
 	/* Initialize and register the subdev. */
 	ret = mipi_csis_subdev_init(csis);
 	if (ret < 0)
-		goto err_disable_clock;
+		return ret;
 
 	platform_set_drvdata(pdev, &csis->sd);
 
@@ -1486,8 +1480,6 @@ static int mipi_csis_probe(struct platform_device *pdev)
 	v4l2_async_nf_unregister(&csis->notifier);
 	v4l2_async_nf_cleanup(&csis->notifier);
 	v4l2_async_unregister_subdev(&csis->sd);
-err_disable_clock:
-	mipi_csis_clk_disable(csis);
 
 	return ret;
 }
@@ -1506,7 +1498,6 @@ static void mipi_csis_remove(struct platform_device *pdev)
 		mipi_csis_runtime_suspend(&pdev->dev);
 
 	pm_runtime_disable(&pdev->dev);
-	mipi_csis_clk_disable(csis);
 	v4l2_subdev_cleanup(&csis->sd);
 	media_entity_cleanup(&csis->sd.entity);
 	pm_runtime_set_suspended(&pdev->dev);

-- 
2.34.1

Re: [PATCH 0/2] media: imx-mipi-csis: csis clock fixes

[Fabio Estevam]

Hi Tomi,

On Wed, Nov 22, 2023 at 10:14 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> Two fixes to the csis driver: One to fix remove() another to only enable
> the clocks when needed.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
> Tomi Valkeinen (2):
>       media: imx-mipi-csis: Fix clock handling in remove()
>       media: imx-mipi-csis: Drop extra clock enable at probe()

Shouldn't both patches contain a Fixes tag?

Re: [PATCH 0/2] media: imx-mipi-csis: csis clock fixes

[Tomi Valkeinen]

On 22/11/2023 15:21, Fabio Estevam wrote:
> Hi Tomi,
> 
> On Wed, Nov 22, 2023 at 10:14 AM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>>
>> Two fixes to the csis driver: One to fix remove() another to only enable
>> the clocks when needed.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>> Tomi Valkeinen (2):
>>        media: imx-mipi-csis: Fix clock handling in remove()
>>        media: imx-mipi-csis: Drop extra clock enable at probe()
> 
> Shouldn't both patches contain a Fixes tag?

Indeed, yes, I'll add that.

  Tomi

Re: [PATCH 0/2] media: imx-mipi-csis: csis clock fixes

[Tomi Valkeinen]

On 22/11/2023 15:21, Fabio Estevam wrote:
> Hi Tomi,
> 
> On Wed, Nov 22, 2023 at 10:14 AM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>>
>> Two fixes to the csis driver: One to fix remove() another to only enable
>> the clocks when needed.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>> Tomi Valkeinen (2):
>>        media: imx-mipi-csis: Fix clock handling in remove()
>>        media: imx-mipi-csis: Drop extra clock enable at probe()
> 
> Shouldn't both patches contain a Fixes tag?

I think the issue is there in the original commit adding the driver:

7807063b862b ("media: staging/imx7: add MIPI CSI-2 receiver subdev for 
i.MX7")

However, the driver has changed along the way, and I'm not sure if the 
original one had an actual bug. Nevertheless, the same pattern (wrt. 
clocks and runtime) is there in the original one, and I think that 
pattern is not correct even if it wouldn't have caused any visible issue.

So I'll add that commit as Fixes-tag, but if someone with more knowledge 
about the driver can verify this, that'd be great.

  Tomi

Re: [PATCH 1/2] media: imx-mipi-csis: Fix clock handling in remove()

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Nov 22, 2023 at 03:13:48PM +0200, Tomi Valkeinen wrote:
> The driver always calls mipi_csis_runtime_suspend() and
> mipi_csis_clk_disable() in remove(). This causes multiple WARNs from the
> kernel, as the clocks get disabled too many times.

Did you try to unload the driver ? What a weird idea :-)

> Fix the remove() to call mipi_csis_runtime_suspend() and
> mipi_csis_clk_disable() in a way that reverses what is done in probe().
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

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

> ---
>  drivers/media/platform/nxp/imx-mipi-csis.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index 6cb20b45e0a1..b39d7aeba750 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -1502,8 +1502,10 @@ static void mipi_csis_remove(struct platform_device *pdev)
>  	v4l2_async_nf_cleanup(&csis->notifier);
>  	v4l2_async_unregister_subdev(&csis->sd);
>  
> +	if (!pm_runtime_enabled(&pdev->dev))
> +		mipi_csis_runtime_suspend(&pdev->dev);
> +
>  	pm_runtime_disable(&pdev->dev);
> -	mipi_csis_runtime_suspend(&pdev->dev);
>  	mipi_csis_clk_disable(csis);
>  	v4l2_subdev_cleanup(&csis->sd);
>  	media_entity_cleanup(&csis->sd.entity);
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: imx-mipi-csis: Drop extra clock enable at probe()

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Nov 22, 2023 at 03:13:49PM +0200, Tomi Valkeinen wrote:
> The driver always enables the clocks at probe() and disables them only
> at remove(). It is not clear why the driver does this, as it supports
> runtime PM, and enables and disables the clocks in the runtime resume
> and suspend callbacks. Also, in the case runtime PM is not available,
> the driver calls the resume and suspend callbacks manually from probe()
> and remove().

Probably a historical mistake. It predates my involvement with the
driver :-)

> Drop the unnecessary clock enable, thus enabling the clocks only when
> actually needed.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

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

> ---
>  drivers/media/platform/nxp/imx-mipi-csis.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index b39d7aeba750..b08f6d2e7516 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -1435,24 +1435,18 @@ static int mipi_csis_probe(struct platform_device *pdev)
>  	/* Reset PHY and enable the clocks. */
>  	mipi_csis_phy_reset(csis);
>  
> -	ret = mipi_csis_clk_enable(csis);
> -	if (ret < 0) {
> -		dev_err(csis->dev, "failed to enable clocks: %d\n", ret);
> -		return ret;
> -	}
> -
>  	/* Now that the hardware is initialized, request the interrupt. */
>  	ret = devm_request_irq(dev, irq, mipi_csis_irq_handler, 0,
>  			       dev_name(dev), csis);
>  	if (ret) {
>  		dev_err(dev, "Interrupt request failed\n");
> -		goto err_disable_clock;
> +		return ret;
>  	}
>  
>  	/* Initialize and register the subdev. */
>  	ret = mipi_csis_subdev_init(csis);
>  	if (ret < 0)
> -		goto err_disable_clock;
> +		return ret;
>  
>  	platform_set_drvdata(pdev, &csis->sd);
>  
> @@ -1486,8 +1480,6 @@ static int mipi_csis_probe(struct platform_device *pdev)
>  	v4l2_async_nf_unregister(&csis->notifier);
>  	v4l2_async_nf_cleanup(&csis->notifier);
>  	v4l2_async_unregister_subdev(&csis->sd);
> -err_disable_clock:
> -	mipi_csis_clk_disable(csis);
>  
>  	return ret;
>  }
> @@ -1506,7 +1498,6 @@ static void mipi_csis_remove(struct platform_device *pdev)
>  		mipi_csis_runtime_suspend(&pdev->dev);
>  
>  	pm_runtime_disable(&pdev->dev);
> -	mipi_csis_clk_disable(csis);
>  	v4l2_subdev_cleanup(&csis->sd);
>  	media_entity_cleanup(&csis->sd.entity);
>  	pm_runtime_set_suspended(&pdev->dev);
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/2] media: imx-mipi-csis: csis clock fixes

[Laurent Pinchart]

On Wed, Nov 22, 2023 at 03:44:33PM +0200, Tomi Valkeinen wrote:
> On 22/11/2023 15:21, Fabio Estevam wrote:
> > On Wed, Nov 22, 2023 at 10:14 AM Tomi Valkeinen wrote:
> >>
> >> Two fixes to the csis driver: One to fix remove() another to only enable
> >> the clocks when needed.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> ---
> >> Tomi Valkeinen (2):
> >>        media: imx-mipi-csis: Fix clock handling in remove()
> >>        media: imx-mipi-csis: Drop extra clock enable at probe()
> > 
> > Shouldn't both patches contain a Fixes tag?
> 
> I think the issue is there in the original commit adding the driver:
> 
> 7807063b862b ("media: staging/imx7: add MIPI CSI-2 receiver subdev for 
> i.MX7")
> 
> However, the driver has changed along the way, and I'm not sure if the 
> original one had an actual bug. Nevertheless, the same pattern (wrt. 
> clocks and runtime) is there in the original one, and I think that 
> pattern is not correct even if it wouldn't have caused any visible issue.
> 
> So I'll add that commit as Fixes-tag, but if someone with more knowledge 
> about the driver can verify this, that'd be great.

Sounds fine to me. I assume you'll send a v2.

-- 
Regards,

Laurent Pinchart