[PATCH] media: rkisp1: request/free irqs in PM runtime ops

[PATCH] media: rkisp1: request/free irqs in PM runtime ops

[Daniel Scally]

The rkisp1 driver tracks whether the IRQ handlers should take any
action through the use of an "irqs_enabled" flag which is set true
at the end of .runtime_resume() and false at the start of
.runtime_suspend(). In .runtime_suspend(), after setting the flag,
there's then a short window during which the hardware has not yet
been disabled by the clock APIs but interrupts remain enabled. If an
interrupt is triggered during that window the IRQ handlers will return
IRQ_NONE and fail to clear the ISP's IRQ reset registers.

Instead, delay calling request_irq() from .probe() to the end of the
.runtime_resume() callback, and call free_irq() at the start of the
.runtime_suspend() callback. This will prevent the interrupt handlers
being called at all for the device once .runtime_suspend() has been
called for it.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
We noticed this problem when testing hibernation with the streams
running. In a typical use-case the stream might be stopped before the
runtime PM suspend callback is called, and so the problem is avoided,
but when hibernated with active streams there are sufficient interrupts
coming in to reliably land one in the window between the irqs_enabled
flag being set to false and the hardware being disabled through
clk_bulk_disable_unprepare().

I'm under the impression that requesting the IRQs when the device is
in use in the manner of this patch and releasing the when it is not
in use is preferred over requesting them in the .probe() function -
possibly an impression I picked up from Linux Device Drivers. It
doesn't seem to be a particularly common model though, so I thought
I'd flag the method here.
---
 .../platform/rockchip/rkisp1/rkisp1-capture.c      |  3 -
 .../media/platform/rockchip/rkisp1/rkisp1-common.h |  2 -
 .../media/platform/rockchip/rkisp1/rkisp1-csi.c    |  3 -
 .../media/platform/rockchip/rkisp1/rkisp1-dev.c    | 82 +++++++++++++++-------
 .../media/platform/rockchip/rkisp1/rkisp1-isp.c    |  3 -
 5 files changed, 57 insertions(+), 36 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
index 6dcefd144d5abe358323e37ac6133c6134ac636e..510d1e8d8bbc86e8b8be3579571e308e5ad9f260 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
@@ -820,9 +820,6 @@ irqreturn_t rkisp1_capture_isr(int irq, void *ctx)
 	unsigned int i;
 	u32 status;
 
-	if (!rkisp1->irqs_enabled)
-		return IRQ_NONE;
-
 	status = rkisp1_read(rkisp1, RKISP1_CIF_MI_MIS);
 	if (!status)
 		return IRQ_NONE;
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
index 5e6a4d5f6fd12baf45a0083eff75de1095162b2d..2a5f6f69b217cdba2fa7c4d1f230ede5aff49149 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
@@ -507,7 +507,6 @@ struct rkisp1_debug {
  * @debug:	   debug params to be exposed on debugfs
  * @info:	   version-specific ISP information
  * @irqs:          IRQ line numbers
- * @irqs_enabled:  the hardware is enabled and can cause interrupts
  */
 struct rkisp1_device {
 	void __iomem *base_addr;
@@ -532,7 +531,6 @@ struct rkisp1_device {
 	struct rkisp1_debug debug;
 	const struct rkisp1_info *info;
 	int irqs[RKISP1_NUM_IRQS];
-	bool irqs_enabled;
 };
 
 /*
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
index ddc6182f3e4bdacdd1962c86f6259334b16aa505..bfc33365ad9d09ccace4ccbb2b19a2fbe1b77eb2 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
@@ -196,9 +196,6 @@ irqreturn_t rkisp1_csi_isr(int irq, void *ctx)
 	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
 	u32 val, status;
 
-	if (!rkisp1->irqs_enabled)
-		return IRQ_NONE;
-
 	status = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_MIS);
 	if (!status)
 		return IRQ_NONE;
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
index 1791c02a40ae18205f5eb2fd6edca6cda6b459bf..6fa76423bacf3e92cbbb4ef1ceb55e194b88d963 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
@@ -307,28 +307,62 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1)
  * Power
  */
 
-static int __maybe_unused rkisp1_runtime_suspend(struct device *dev)
+static void rkisp1_free_irqs(struct rkisp1_device *rkisp1)
 {
-	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
+	for (unsigned int i = 0; i < ARRAY_SIZE(rkisp1->irqs); i++) {
+		if (rkisp1->irqs[i] == -1)
+			continue;
 
-	rkisp1->irqs_enabled = false;
-	/* Make sure the IRQ handler will see the above */
-	mb();
+		if (irq_has_action(rkisp1->irqs[i]))
+			free_irq(rkisp1->irqs[i], rkisp1->dev);
+	}
+}
 
-	/*
-	 * Wait until any running IRQ handler has returned. The IRQ handler
-	 * may get called even after this (as it's a shared interrupt line)
-	 * but the 'irqs_enabled' flag will make the handler return immediately.
-	 */
-	for (unsigned int il = 0; il < ARRAY_SIZE(rkisp1->irqs); ++il) {
-		if (rkisp1->irqs[il] == -1)
+static int rkisp1_request_irqs(struct rkisp1_device *rkisp1)
+{
+	const struct rkisp1_info *info = rkisp1->info;
+	int ret;
+
+	for (unsigned int irqn = 0; irqn < ARRAY_SIZE(rkisp1->irqs); irqn++) {
+		unsigned int isrn;
+
+		if (rkisp1->irqs[irqn] == -1)
 			continue;
 
-		/* Skip if the irq line is the same as previous */
-		if (il == 0 || rkisp1->irqs[il - 1] != rkisp1->irqs[il])
-			synchronize_irq(rkisp1->irqs[il]);
+		if (irq_has_action(rkisp1->irqs[irqn]))
+			continue;
+
+		for (isrn = 0; isrn < info->isr_size; isrn++)
+			if ((info->isrs[isrn].line_mask & BIT(irqn)))
+				break;
+
+		if (isrn == info->isr_size) {
+			dev_err(rkisp1->dev, "Failed to find IRQ handler\n");
+			return -EINVAL;
+		}
+
+		ret = request_irq(rkisp1->irqs[irqn], info->isrs[isrn].isr,
+				  IRQF_SHARED, dev_driver_string(rkisp1->dev),
+				  rkisp1->dev);
+		if (ret) {
+			dev_err(rkisp1->dev, "Failed to request IRQ\n");
+			goto err_free_irqs;
+		}
 	}
 
+	return 0;
+
+err_free_irqs:
+	rkisp1_free_irqs(rkisp1);
+	return ret;
+}
+
+static int __maybe_unused rkisp1_runtime_suspend(struct device *dev)
+{
+	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
+
+	rkisp1_free_irqs(rkisp1);
+
 	clk_bulk_disable_unprepare(rkisp1->clk_size, rkisp1->clks);
 	return pinctrl_pm_select_sleep_state(dev);
 }
@@ -345,11 +379,16 @@ static int __maybe_unused rkisp1_runtime_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	rkisp1->irqs_enabled = true;
-	/* Make sure the IRQ handler will see the above */
-	mb();
+	ret = rkisp1_request_irqs(rkisp1);
+	if (ret)
+		goto err_clk_disable;
 
 	return 0;
+
+err_clk_disable:
+	clk_bulk_disable_unprepare(rkisp1->clk_size, rkisp1->clks);
+
+	return ret;
 }
 
 static const struct dev_pm_ops rkisp1_pm_ops = {
@@ -694,13 +733,6 @@ static int rkisp1_probe(struct platform_device *pdev)
 			if (info->isrs[i].line_mask & BIT(il))
 				rkisp1->irqs[il] = irq;
 		}
-
-		ret = devm_request_irq(dev, irq, info->isrs[i].isr, IRQF_SHARED,
-				       dev_driver_string(dev), dev);
-		if (ret) {
-			dev_err(dev, "request irq failed: %d\n", ret);
-			return ret;
-		}
 	}
 
 	ret = rkisp1_init_clocks(rkisp1);
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
index 2311672cedb1b6c9dd7f1b883adcac1516a685ae..c6b1ecd2d0c260f6739726c9f32562b98ca31364 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
@@ -1106,9 +1106,6 @@ irqreturn_t rkisp1_isp_isr(int irq, void *ctx)
 	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
 	u32 status, isp_err;
 
-	if (!rkisp1->irqs_enabled)
-		return IRQ_NONE;
-
 	status = rkisp1_read(rkisp1, RKISP1_CIF_ISP_MIS);
 	if (!status)
 		return IRQ_NONE;

---
base-commit: ee5b462b97162dbb6c536e18a37b3048f6520019
change-id: 20260105-rkisp1-irqs-8af5a1e0b887

Best regards,
-- 
Daniel Scally <dan.scally@ideasonboard.com>

Re: [PATCH] media: rkisp1: request/free irqs in PM runtime ops

[Laurent Pinchart]

On Mon, Jan 05, 2026 at 04:19:08PM +0000, Daniel Scally wrote:
> The rkisp1 driver tracks whether the IRQ handlers should take any
> action through the use of an "irqs_enabled" flag which is set true
> at the end of .runtime_resume() and false at the start of
> .runtime_suspend(). In .runtime_suspend(), after setting the flag,
> there's then a short window during which the hardware has not yet
> been disabled by the clock APIs but interrupts remain enabled. If an
> interrupt is triggered during that window the IRQ handlers will return
> IRQ_NONE and fail to clear the ISP's IRQ reset registers.
> 
> Instead, delay calling request_irq() from .probe() to the end of the
> .runtime_resume() callback, and call free_irq() at the start of the
> .runtime_suspend() callback. This will prevent the interrupt handlers
> being called at all for the device once .runtime_suspend() has been
> called for it.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> We noticed this problem when testing hibernation with the streams
> running. In a typical use-case the stream might be stopped before the
> runtime PM suspend callback is called, and so the problem is avoided,
> but when hibernated with active streams there are sufficient interrupts
> coming in to reliably land one in the window between the irqs_enabled
> flag being set to false and the hardware being disabled through
> clk_bulk_disable_unprepare().
> 
> I'm under the impression that requesting the IRQs when the device is
> in use in the manner of this patch and releasing the when it is not
> in use is preferred over requesting them in the .probe() function -
> possibly an impression I picked up from Linux Device Drivers. It
> doesn't seem to be a particularly common model though, so I thought
> I'd flag the method here.

I'm not aware of drivers doing this commonly, so the approach taken in
this patch seems wrong.

> ---
>  .../platform/rockchip/rkisp1/rkisp1-capture.c      |  3 -
>  .../media/platform/rockchip/rkisp1/rkisp1-common.h |  2 -
>  .../media/platform/rockchip/rkisp1/rkisp1-csi.c    |  3 -
>  .../media/platform/rockchip/rkisp1/rkisp1-dev.c    | 82 +++++++++++++++-------
>  .../media/platform/rockchip/rkisp1/rkisp1-isp.c    |  3 -
>  5 files changed, 57 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> index 6dcefd144d5abe358323e37ac6133c6134ac636e..510d1e8d8bbc86e8b8be3579571e308e5ad9f260 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> @@ -820,9 +820,6 @@ irqreturn_t rkisp1_capture_isr(int irq, void *ctx)
>  	unsigned int i;
>  	u32 status;
>  
> -	if (!rkisp1->irqs_enabled)
> -		return IRQ_NONE;
> -
>  	status = rkisp1_read(rkisp1, RKISP1_CIF_MI_MIS);
>  	if (!status)
>  		return IRQ_NONE;
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index 5e6a4d5f6fd12baf45a0083eff75de1095162b2d..2a5f6f69b217cdba2fa7c4d1f230ede5aff49149 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -507,7 +507,6 @@ struct rkisp1_debug {
>   * @debug:	   debug params to be exposed on debugfs
>   * @info:	   version-specific ISP information
>   * @irqs:          IRQ line numbers
> - * @irqs_enabled:  the hardware is enabled and can cause interrupts
>   */
>  struct rkisp1_device {
>  	void __iomem *base_addr;
> @@ -532,7 +531,6 @@ struct rkisp1_device {
>  	struct rkisp1_debug debug;
>  	const struct rkisp1_info *info;
>  	int irqs[RKISP1_NUM_IRQS];
> -	bool irqs_enabled;
>  };
>  
>  /*
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> index ddc6182f3e4bdacdd1962c86f6259334b16aa505..bfc33365ad9d09ccace4ccbb2b19a2fbe1b77eb2 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> @@ -196,9 +196,6 @@ irqreturn_t rkisp1_csi_isr(int irq, void *ctx)
>  	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
>  	u32 val, status;
>  
> -	if (!rkisp1->irqs_enabled)
> -		return IRQ_NONE;
> -
>  	status = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_MIS);
>  	if (!status)
>  		return IRQ_NONE;
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> index 1791c02a40ae18205f5eb2fd6edca6cda6b459bf..6fa76423bacf3e92cbbb4ef1ceb55e194b88d963 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -307,28 +307,62 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1)
>   * Power
>   */
>  
> -static int __maybe_unused rkisp1_runtime_suspend(struct device *dev)
> +static void rkisp1_free_irqs(struct rkisp1_device *rkisp1)
>  {
> -	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
> +	for (unsigned int i = 0; i < ARRAY_SIZE(rkisp1->irqs); i++) {
> +		if (rkisp1->irqs[i] == -1)
> +			continue;
>  
> -	rkisp1->irqs_enabled = false;
> -	/* Make sure the IRQ handler will see the above */
> -	mb();
> +		if (irq_has_action(rkisp1->irqs[i]))
> +			free_irq(rkisp1->irqs[i], rkisp1->dev);
> +	}
> +}
>  
> -	/*
> -	 * Wait until any running IRQ handler has returned. The IRQ handler
> -	 * may get called even after this (as it's a shared interrupt line)
> -	 * but the 'irqs_enabled' flag will make the handler return immediately.
> -	 */
> -	for (unsigned int il = 0; il < ARRAY_SIZE(rkisp1->irqs); ++il) {
> -		if (rkisp1->irqs[il] == -1)
> +static int rkisp1_request_irqs(struct rkisp1_device *rkisp1)
> +{
> +	const struct rkisp1_info *info = rkisp1->info;
> +	int ret;
> +
> +	for (unsigned int irqn = 0; irqn < ARRAY_SIZE(rkisp1->irqs); irqn++) {
> +		unsigned int isrn;
> +
> +		if (rkisp1->irqs[irqn] == -1)
>  			continue;
>  
> -		/* Skip if the irq line is the same as previous */
> -		if (il == 0 || rkisp1->irqs[il - 1] != rkisp1->irqs[il])
> -			synchronize_irq(rkisp1->irqs[il]);
> +		if (irq_has_action(rkisp1->irqs[irqn]))
> +			continue;
> +
> +		for (isrn = 0; isrn < info->isr_size; isrn++)
> +			if ((info->isrs[isrn].line_mask & BIT(irqn)))
> +				break;
> +
> +		if (isrn == info->isr_size) {
> +			dev_err(rkisp1->dev, "Failed to find IRQ handler\n");
> +			return -EINVAL;
> +		}
> +
> +		ret = request_irq(rkisp1->irqs[irqn], info->isrs[isrn].isr,
> +				  IRQF_SHARED, dev_driver_string(rkisp1->dev),
> +				  rkisp1->dev);
> +		if (ret) {
> +			dev_err(rkisp1->dev, "Failed to request IRQ\n");
> +			goto err_free_irqs;
> +		}
>  	}
>  
> +	return 0;
> +
> +err_free_irqs:
> +	rkisp1_free_irqs(rkisp1);
> +	return ret;
> +}
> +
> +static int __maybe_unused rkisp1_runtime_suspend(struct device *dev)
> +{
> +	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
> +
> +	rkisp1_free_irqs(rkisp1);
> +
>  	clk_bulk_disable_unprepare(rkisp1->clk_size, rkisp1->clks);
>  	return pinctrl_pm_select_sleep_state(dev);
>  }
> @@ -345,11 +379,16 @@ static int __maybe_unused rkisp1_runtime_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	rkisp1->irqs_enabled = true;
> -	/* Make sure the IRQ handler will see the above */
> -	mb();
> +	ret = rkisp1_request_irqs(rkisp1);
> +	if (ret)
> +		goto err_clk_disable;
>  
>  	return 0;
> +
> +err_clk_disable:
> +	clk_bulk_disable_unprepare(rkisp1->clk_size, rkisp1->clks);
> +
> +	return ret;
>  }
>  
>  static const struct dev_pm_ops rkisp1_pm_ops = {
> @@ -694,13 +733,6 @@ static int rkisp1_probe(struct platform_device *pdev)
>  			if (info->isrs[i].line_mask & BIT(il))
>  				rkisp1->irqs[il] = irq;
>  		}
> -
> -		ret = devm_request_irq(dev, irq, info->isrs[i].isr, IRQF_SHARED,
> -				       dev_driver_string(dev), dev);
> -		if (ret) {
> -			dev_err(dev, "request irq failed: %d\n", ret);
> -			return ret;
> -		}
>  	}
>  
>  	ret = rkisp1_init_clocks(rkisp1);
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> index 2311672cedb1b6c9dd7f1b883adcac1516a685ae..c6b1ecd2d0c260f6739726c9f32562b98ca31364 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> @@ -1106,9 +1106,6 @@ irqreturn_t rkisp1_isp_isr(int irq, void *ctx)
>  	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
>  	u32 status, isp_err;
>  
> -	if (!rkisp1->irqs_enabled)
> -		return IRQ_NONE;
> -
>  	status = rkisp1_read(rkisp1, RKISP1_CIF_ISP_MIS);
>  	if (!status)
>  		return IRQ_NONE;
> 
> ---
> base-commit: ee5b462b97162dbb6c536e18a37b3048f6520019
> change-id: 20260105-rkisp1-irqs-8af5a1e0b887

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: rkisp1: request/free irqs in PM runtime ops

[Tomi Valkeinen]

Hi,

On 05/01/2026 18:19, Daniel Scally wrote:
> The rkisp1 driver tracks whether the IRQ handlers should take any
> action through the use of an "irqs_enabled" flag which is set true
> at the end of .runtime_resume() and false at the start of
> .runtime_suspend(). In .runtime_suspend(), after setting the flag,
> there's then a short window during which the hardware has not yet
> been disabled by the clock APIs but interrupts remain enabled. If an
> interrupt is triggered during that window the IRQ handlers will return
> IRQ_NONE and fail to clear the ISP's IRQ reset registers.
> 
> Instead, delay calling request_irq() from .probe() to the end of the
> .runtime_resume() callback, and call free_irq() at the start of the
> .runtime_suspend() callback. This will prevent the interrupt handlers
> being called at all for the device once .runtime_suspend() has been
> called for it.

Shouldn't we usually always properly disable the IP before suspend? I've
seen IPs that definitely did not like at all cutting the clocks
arbitrarily when it's active.

And I don't think clk disable should be considered "disabling the
hardware". The clocks may not even be disabled at that time, if they're
shared clocks or always on clocks. So if the driver assumes that after
clk disable call it won't get any interrupts... I don't think that right.

If we can't sensibly disable the IP, I think we can at least mask the
IP's interrupts in its interrupt enable register (which we probably
should do even if we can disable the IP), and wait for any possible irq
handler to stop running. After that we won't get any interrupts,
regardless of the clocks.

 Tomi

> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> We noticed this problem when testing hibernation with the streams
> running. In a typical use-case the stream might be stopped before the
> runtime PM suspend callback is called, and so the problem is avoided,
> but when hibernated with active streams there are sufficient interrupts
> coming in to reliably land one in the window between the irqs_enabled
> flag being set to false and the hardware being disabled through
> clk_bulk_disable_unprepare().
> 
> I'm under the impression that requesting the IRQs when the device is
> in use in the manner of this patch and releasing the when it is not
> in use is preferred over requesting them in the .probe() function -
> possibly an impression I picked up from Linux Device Drivers. It
> doesn't seem to be a particularly common model though, so I thought
> I'd flag the method here.
> ---
>  .../platform/rockchip/rkisp1/rkisp1-capture.c      |  3 -
>  .../media/platform/rockchip/rkisp1/rkisp1-common.h |  2 -
>  .../media/platform/rockchip/rkisp1/rkisp1-csi.c    |  3 -
>  .../media/platform/rockchip/rkisp1/rkisp1-dev.c    | 82 +++++++++++++++-------
>  .../media/platform/rockchip/rkisp1/rkisp1-isp.c    |  3 -
>  5 files changed, 57 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> index 6dcefd144d5abe358323e37ac6133c6134ac636e..510d1e8d8bbc86e8b8be3579571e308e5ad9f260 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> @@ -820,9 +820,6 @@ irqreturn_t rkisp1_capture_isr(int irq, void *ctx)
>  	unsigned int i;
>  	u32 status;
>  
> -	if (!rkisp1->irqs_enabled)
> -		return IRQ_NONE;
> -
>  	status = rkisp1_read(rkisp1, RKISP1_CIF_MI_MIS);
>  	if (!status)
>  		return IRQ_NONE;
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index 5e6a4d5f6fd12baf45a0083eff75de1095162b2d..2a5f6f69b217cdba2fa7c4d1f230ede5aff49149 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -507,7 +507,6 @@ struct rkisp1_debug {
>   * @debug:	   debug params to be exposed on debugfs
>   * @info:	   version-specific ISP information
>   * @irqs:          IRQ line numbers
> - * @irqs_enabled:  the hardware is enabled and can cause interrupts
>   */
>  struct rkisp1_device {
>  	void __iomem *base_addr;
> @@ -532,7 +531,6 @@ struct rkisp1_device {
>  	struct rkisp1_debug debug;
>  	const struct rkisp1_info *info;
>  	int irqs[RKISP1_NUM_IRQS];
> -	bool irqs_enabled;
>  };
>  
>  /*
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> index ddc6182f3e4bdacdd1962c86f6259334b16aa505..bfc33365ad9d09ccace4ccbb2b19a2fbe1b77eb2 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> @@ -196,9 +196,6 @@ irqreturn_t rkisp1_csi_isr(int irq, void *ctx)
>  	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
>  	u32 val, status;
>  
> -	if (!rkisp1->irqs_enabled)
> -		return IRQ_NONE;
> -
>  	status = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_MIS);
>  	if (!status)
>  		return IRQ_NONE;
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> index 1791c02a40ae18205f5eb2fd6edca6cda6b459bf..6fa76423bacf3e92cbbb4ef1ceb55e194b88d963 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -307,28 +307,62 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1)
>   * Power
>   */
>  
> -static int __maybe_unused rkisp1_runtime_suspend(struct device *dev)
> +static void rkisp1_free_irqs(struct rkisp1_device *rkisp1)
>  {
> -	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
> +	for (unsigned int i = 0; i < ARRAY_SIZE(rkisp1->irqs); i++) {
> +		if (rkisp1->irqs[i] == -1)
> +			continue;
>  
> -	rkisp1->irqs_enabled = false;
> -	/* Make sure the IRQ handler will see the above */
> -	mb();
> +		if (irq_has_action(rkisp1->irqs[i]))
> +			free_irq(rkisp1->irqs[i], rkisp1->dev);
> +	}
> +}
>  
> -	/*
> -	 * Wait until any running IRQ handler has returned. The IRQ handler
> -	 * may get called even after this (as it's a shared interrupt line)
> -	 * but the 'irqs_enabled' flag will make the handler return immediately.
> -	 */
> -	for (unsigned int il = 0; il < ARRAY_SIZE(rkisp1->irqs); ++il) {
> -		if (rkisp1->irqs[il] == -1)
> +static int rkisp1_request_irqs(struct rkisp1_device *rkisp1)
> +{
> +	const struct rkisp1_info *info = rkisp1->info;
> +	int ret;
> +
> +	for (unsigned int irqn = 0; irqn < ARRAY_SIZE(rkisp1->irqs); irqn++) {
> +		unsigned int isrn;
> +
> +		if (rkisp1->irqs[irqn] == -1)
>  			continue;
>  
> -		/* Skip if the irq line is the same as previous */
> -		if (il == 0 || rkisp1->irqs[il - 1] != rkisp1->irqs[il])
> -			synchronize_irq(rkisp1->irqs[il]);
> +		if (irq_has_action(rkisp1->irqs[irqn]))
> +			continue;
> +
> +		for (isrn = 0; isrn < info->isr_size; isrn++)
> +			if ((info->isrs[isrn].line_mask & BIT(irqn)))
> +				break;
> +
> +		if (isrn == info->isr_size) {
> +			dev_err(rkisp1->dev, "Failed to find IRQ handler\n");
> +			return -EINVAL;
> +		}
> +
> +		ret = request_irq(rkisp1->irqs[irqn], info->isrs[isrn].isr,
> +				  IRQF_SHARED, dev_driver_string(rkisp1->dev),
> +				  rkisp1->dev);
> +		if (ret) {
> +			dev_err(rkisp1->dev, "Failed to request IRQ\n");
> +			goto err_free_irqs;
> +		}
>  	}
>  
> +	return 0;
> +
> +err_free_irqs:
> +	rkisp1_free_irqs(rkisp1);
> +	return ret;
> +}
> +
> +static int __maybe_unused rkisp1_runtime_suspend(struct device *dev)
> +{
> +	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
> +
> +	rkisp1_free_irqs(rkisp1);
> +
>  	clk_bulk_disable_unprepare(rkisp1->clk_size, rkisp1->clks);
>  	return pinctrl_pm_select_sleep_state(dev);
>  }
> @@ -345,11 +379,16 @@ static int __maybe_unused rkisp1_runtime_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	rkisp1->irqs_enabled = true;
> -	/* Make sure the IRQ handler will see the above */
> -	mb();
> +	ret = rkisp1_request_irqs(rkisp1);
> +	if (ret)
> +		goto err_clk_disable;
>  
>  	return 0;
> +
> +err_clk_disable:
> +	clk_bulk_disable_unprepare(rkisp1->clk_size, rkisp1->clks);
> +
> +	return ret;
>  }
>  
>  static const struct dev_pm_ops rkisp1_pm_ops = {
> @@ -694,13 +733,6 @@ static int rkisp1_probe(struct platform_device *pdev)
>  			if (info->isrs[i].line_mask & BIT(il))
>  				rkisp1->irqs[il] = irq;
>  		}
> -
> -		ret = devm_request_irq(dev, irq, info->isrs[i].isr, IRQF_SHARED,
> -				       dev_driver_string(dev), dev);
> -		if (ret) {
> -			dev_err(dev, "request irq failed: %d\n", ret);
> -			return ret;
> -		}
>  	}
>  
>  	ret = rkisp1_init_clocks(rkisp1);
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> index 2311672cedb1b6c9dd7f1b883adcac1516a685ae..c6b1ecd2d0c260f6739726c9f32562b98ca31364 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> @@ -1106,9 +1106,6 @@ irqreturn_t rkisp1_isp_isr(int irq, void *ctx)
>  	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
>  	u32 status, isp_err;
>  
> -	if (!rkisp1->irqs_enabled)
> -		return IRQ_NONE;
> -
>  	status = rkisp1_read(rkisp1, RKISP1_CIF_ISP_MIS);
>  	if (!status)
>  		return IRQ_NONE;
> 
> ---
> base-commit: ee5b462b97162dbb6c536e18a37b3048f6520019
> change-id: 20260105-rkisp1-irqs-8af5a1e0b887
> 
> Best regards,

Re: [PATCH] media: rkisp1: request/free irqs in PM runtime ops

[Laurent Pinchart]

On Tue, Jan 06, 2026 at 06:01:38PM +0200, Tomi Valkeinen wrote:
> Hi,
> 
> On 05/01/2026 18:19, Daniel Scally wrote:
> > The rkisp1 driver tracks whether the IRQ handlers should take any
> > action through the use of an "irqs_enabled" flag which is set true
> > at the end of .runtime_resume() and false at the start of
> > .runtime_suspend(). In .runtime_suspend(), after setting the flag,
> > there's then a short window during which the hardware has not yet
> > been disabled by the clock APIs but interrupts remain enabled. If an
> > interrupt is triggered during that window the IRQ handlers will return
> > IRQ_NONE and fail to clear the ISP's IRQ reset registers.
> > 
> > Instead, delay calling request_irq() from .probe() to the end of the
> > .runtime_resume() callback, and call free_irq() at the start of the
> > .runtime_suspend() callback. This will prevent the interrupt handlers
> > being called at all for the device once .runtime_suspend() has been
> > called for it.
> 
> Shouldn't we usually always properly disable the IP before suspend? I've
> seen IPs that definitely did not like at all cutting the clocks
> arbitrarily when it's active.

Yes we should. The driver should have real system suspend/resume
handlers that stop and restart streaming.

> And I don't think clk disable should be considered "disabling the
> hardware". The clocks may not even be disabled at that time, if they're
> shared clocks or always on clocks. So if the driver assumes that after
> clk disable call it won't get any interrupts... I don't think that right.
> 
> If we can't sensibly disable the IP, I think we can at least mask the
> IP's interrupts in its interrupt enable register (which we probably
> should do even if we can disable the IP), and wait for any possible irq
> handler to stop running. After that we won't get any interrupts,
> regardless of the clocks.
> 
>  Tomi
> 
> > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> > ---
> > We noticed this problem when testing hibernation with the streams
> > running. In a typical use-case the stream might be stopped before the
> > runtime PM suspend callback is called, and so the problem is avoided,
> > but when hibernated with active streams there are sufficient interrupts
> > coming in to reliably land one in the window between the irqs_enabled
> > flag being set to false and the hardware being disabled through
> > clk_bulk_disable_unprepare().
> > 
> > I'm under the impression that requesting the IRQs when the device is
> > in use in the manner of this patch and releasing the when it is not
> > in use is preferred over requesting them in the .probe() function -
> > possibly an impression I picked up from Linux Device Drivers. It
> > doesn't seem to be a particularly common model though, so I thought
> > I'd flag the method here.
> > ---
> >  .../platform/rockchip/rkisp1/rkisp1-capture.c      |  3 -
> >  .../media/platform/rockchip/rkisp1/rkisp1-common.h |  2 -
> >  .../media/platform/rockchip/rkisp1/rkisp1-csi.c    |  3 -
> >  .../media/platform/rockchip/rkisp1/rkisp1-dev.c    | 82 +++++++++++++++-------
> >  .../media/platform/rockchip/rkisp1/rkisp1-isp.c    |  3 -
> >  5 files changed, 57 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > index 6dcefd144d5abe358323e37ac6133c6134ac636e..510d1e8d8bbc86e8b8be3579571e308e5ad9f260 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > @@ -820,9 +820,6 @@ irqreturn_t rkisp1_capture_isr(int irq, void *ctx)
> >  	unsigned int i;
> >  	u32 status;
> >  
> > -	if (!rkisp1->irqs_enabled)
> > -		return IRQ_NONE;
> > -
> >  	status = rkisp1_read(rkisp1, RKISP1_CIF_MI_MIS);
> >  	if (!status)
> >  		return IRQ_NONE;
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > index 5e6a4d5f6fd12baf45a0083eff75de1095162b2d..2a5f6f69b217cdba2fa7c4d1f230ede5aff49149 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > @@ -507,7 +507,6 @@ struct rkisp1_debug {
> >   * @debug:	   debug params to be exposed on debugfs
> >   * @info:	   version-specific ISP information
> >   * @irqs:          IRQ line numbers
> > - * @irqs_enabled:  the hardware is enabled and can cause interrupts
> >   */
> >  struct rkisp1_device {
> >  	void __iomem *base_addr;
> > @@ -532,7 +531,6 @@ struct rkisp1_device {
> >  	struct rkisp1_debug debug;
> >  	const struct rkisp1_info *info;
> >  	int irqs[RKISP1_NUM_IRQS];
> > -	bool irqs_enabled;
> >  };
> >  
> >  /*
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> > index ddc6182f3e4bdacdd1962c86f6259334b16aa505..bfc33365ad9d09ccace4ccbb2b19a2fbe1b77eb2 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
> > @@ -196,9 +196,6 @@ irqreturn_t rkisp1_csi_isr(int irq, void *ctx)
> >  	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
> >  	u32 val, status;
> >  
> > -	if (!rkisp1->irqs_enabled)
> > -		return IRQ_NONE;
> > -
> >  	status = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_MIS);
> >  	if (!status)
> >  		return IRQ_NONE;
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > index 1791c02a40ae18205f5eb2fd6edca6cda6b459bf..6fa76423bacf3e92cbbb4ef1ceb55e194b88d963 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > @@ -307,28 +307,62 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1)
> >   * Power
> >   */
> >  
> > -static int __maybe_unused rkisp1_runtime_suspend(struct device *dev)
> > +static void rkisp1_free_irqs(struct rkisp1_device *rkisp1)
> >  {
> > -	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
> > +	for (unsigned int i = 0; i < ARRAY_SIZE(rkisp1->irqs); i++) {
> > +		if (rkisp1->irqs[i] == -1)
> > +			continue;
> >  
> > -	rkisp1->irqs_enabled = false;
> > -	/* Make sure the IRQ handler will see the above */
> > -	mb();
> > +		if (irq_has_action(rkisp1->irqs[i]))
> > +			free_irq(rkisp1->irqs[i], rkisp1->dev);
> > +	}
> > +}
> >  
> > -	/*
> > -	 * Wait until any running IRQ handler has returned. The IRQ handler
> > -	 * may get called even after this (as it's a shared interrupt line)
> > -	 * but the 'irqs_enabled' flag will make the handler return immediately.
> > -	 */
> > -	for (unsigned int il = 0; il < ARRAY_SIZE(rkisp1->irqs); ++il) {
> > -		if (rkisp1->irqs[il] == -1)
> > +static int rkisp1_request_irqs(struct rkisp1_device *rkisp1)
> > +{
> > +	const struct rkisp1_info *info = rkisp1->info;
> > +	int ret;
> > +
> > +	for (unsigned int irqn = 0; irqn < ARRAY_SIZE(rkisp1->irqs); irqn++) {
> > +		unsigned int isrn;
> > +
> > +		if (rkisp1->irqs[irqn] == -1)
> >  			continue;
> >  
> > -		/* Skip if the irq line is the same as previous */
> > -		if (il == 0 || rkisp1->irqs[il - 1] != rkisp1->irqs[il])
> > -			synchronize_irq(rkisp1->irqs[il]);
> > +		if (irq_has_action(rkisp1->irqs[irqn]))
> > +			continue;
> > +
> > +		for (isrn = 0; isrn < info->isr_size; isrn++)
> > +			if ((info->isrs[isrn].line_mask & BIT(irqn)))
> > +				break;
> > +
> > +		if (isrn == info->isr_size) {
> > +			dev_err(rkisp1->dev, "Failed to find IRQ handler\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		ret = request_irq(rkisp1->irqs[irqn], info->isrs[isrn].isr,
> > +				  IRQF_SHARED, dev_driver_string(rkisp1->dev),
> > +				  rkisp1->dev);
> > +		if (ret) {
> > +			dev_err(rkisp1->dev, "Failed to request IRQ\n");
> > +			goto err_free_irqs;
> > +		}
> >  	}
> >  
> > +	return 0;
> > +
> > +err_free_irqs:
> > +	rkisp1_free_irqs(rkisp1);
> > +	return ret;
> > +}
> > +
> > +static int __maybe_unused rkisp1_runtime_suspend(struct device *dev)
> > +{
> > +	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
> > +
> > +	rkisp1_free_irqs(rkisp1);
> > +
> >  	clk_bulk_disable_unprepare(rkisp1->clk_size, rkisp1->clks);
> >  	return pinctrl_pm_select_sleep_state(dev);
> >  }
> > @@ -345,11 +379,16 @@ static int __maybe_unused rkisp1_runtime_resume(struct device *dev)
> >  	if (ret)
> >  		return ret;
> >  
> > -	rkisp1->irqs_enabled = true;
> > -	/* Make sure the IRQ handler will see the above */
> > -	mb();
> > +	ret = rkisp1_request_irqs(rkisp1);
> > +	if (ret)
> > +		goto err_clk_disable;
> >  
> >  	return 0;
> > +
> > +err_clk_disable:
> > +	clk_bulk_disable_unprepare(rkisp1->clk_size, rkisp1->clks);
> > +
> > +	return ret;
> >  }
> >  
> >  static const struct dev_pm_ops rkisp1_pm_ops = {
> > @@ -694,13 +733,6 @@ static int rkisp1_probe(struct platform_device *pdev)
> >  			if (info->isrs[i].line_mask & BIT(il))
> >  				rkisp1->irqs[il] = irq;
> >  		}
> > -
> > -		ret = devm_request_irq(dev, irq, info->isrs[i].isr, IRQF_SHARED,
> > -				       dev_driver_string(dev), dev);
> > -		if (ret) {
> > -			dev_err(dev, "request irq failed: %d\n", ret);
> > -			return ret;
> > -		}
> >  	}
> >  
> >  	ret = rkisp1_init_clocks(rkisp1);
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > index 2311672cedb1b6c9dd7f1b883adcac1516a685ae..c6b1ecd2d0c260f6739726c9f32562b98ca31364 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > @@ -1106,9 +1106,6 @@ irqreturn_t rkisp1_isp_isr(int irq, void *ctx)
> >  	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
> >  	u32 status, isp_err;
> >  
> > -	if (!rkisp1->irqs_enabled)
> > -		return IRQ_NONE;
> > -
> >  	status = rkisp1_read(rkisp1, RKISP1_CIF_ISP_MIS);
> >  	if (!status)
> >  		return IRQ_NONE;
> > 
> > ---
> > base-commit: ee5b462b97162dbb6c536e18a37b3048f6520019
> > change-id: 20260105-rkisp1-irqs-8af5a1e0b887

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: rkisp1: request/free irqs in PM runtime ops

[Dan Scally]

Hi Laurent, Tomi

On 06/01/2026 16:03, Laurent Pinchart wrote:
> On Tue, Jan 06, 2026 at 06:01:38PM +0200, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 05/01/2026 18:19, Daniel Scally wrote:
>>> The rkisp1 driver tracks whether the IRQ handlers should take any
>>> action through the use of an "irqs_enabled" flag which is set true
>>> at the end of .runtime_resume() and false at the start of
>>> .runtime_suspend(). In .runtime_suspend(), after setting the flag,
>>> there's then a short window during which the hardware has not yet
>>> been disabled by the clock APIs but interrupts remain enabled. If an
>>> interrupt is triggered during that window the IRQ handlers will return
>>> IRQ_NONE and fail to clear the ISP's IRQ reset registers.
>>>
>>> Instead, delay calling request_irq() from .probe() to the end of the
>>> .runtime_resume() callback, and call free_irq() at the start of the
>>> .runtime_suspend() callback. This will prevent the interrupt handlers
>>> being called at all for the device once .runtime_suspend() has been
>>> called for it.
>>
>> Shouldn't we usually always properly disable the IP before suspend? I've
>> seen IPs that definitely did not like at all cutting the clocks
>> arbitrarily when it's active.
> 
> Yes we should. The driver should have real system suspend/resume
> handlers that stop and restart streaming.

I'm looking at this again now after having parked it for a while. We discussed earlier about the 
changes that the driver needs to better handle a system suspend event whilst streaming, and I think 
that those changes will solve the original problem I was having (interrupts arriving when the driver 
didn't expect them, basically), but I wonder whether this change might not still be useful for a 
different reason. The original discussion [1] that made the change adding the irqs_enabled flag to 
the rkisp1 driver includes some discussion about a more generalisable way to ensure that the 
handlers don't try to access the hardware if they run as a result of a different device triggering a 
shared IRQ line - it seems to me that freeing the handler when it shouldn't be generating anything 
would tackle that. Obviously it's something of a solved problem for this driver, but is it 
worthwhile generally?

Thanks
Dan

[1] https://lore.kernel.org/all/20231218-rkisp-shirq-fix-v1-2-173007628248@ideasonboard.com/


> 
>> And I don't think clk disable should be considered "disabling the
>> hardware". The clocks may not even be disabled at that time, if they're
>> shared clocks or always on clocks. So if the driver assumes that after
>> clk disable call it won't get any interrupts... I don't think that right.
>>
>> If we can't sensibly disable the IP, I think we can at least mask the
>> IP's interrupts in its interrupt enable register (which we probably
>> should do even if we can disable the IP), and wait for any possible irq
>> handler to stop running. After that we won't get any interrupts,
>> regardless of the clocks.
>>
>>   Tomi
>>
>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>>> ---
>>> We noticed this problem when testing hibernation with the streams
>>> running. In a typical use-case the stream might be stopped before the
>>> runtime PM suspend callback is called, and so the problem is avoided,
>>> but when hibernated with active streams there are sufficient interrupts
>>> coming in to reliably land one in the window between the irqs_enabled
>>> flag being set to false and the hardware being disabled through
>>> clk_bulk_disable_unprepare().
>>>
>>> I'm under the impression that requesting the IRQs when the device is
>>> in use in the manner of this patch and releasing the when it is not
>>> in use is preferred over requesting them in the .probe() function -
>>> possibly an impression I picked up from Linux Device Drivers. It
>>> doesn't seem to be a particularly common model though, so I thought
>>> I'd flag the method here.
>>> ---
>>>   .../platform/rockchip/rkisp1/rkisp1-capture.c      |  3 -
>>>   .../media/platform/rockchip/rkisp1/rkisp1-common.h |  2 -
>>>   .../media/platform/rockchip/rkisp1/rkisp1-csi.c    |  3 -
>>>   .../media/platform/rockchip/rkisp1/rkisp1-dev.c    | 82 +++++++++++++++-------
>>>   .../media/platform/rockchip/rkisp1/rkisp1-isp.c    |  3 -
>>>   5 files changed, 57 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
>>> index 6dcefd144d5abe358323e37ac6133c6134ac636e..510d1e8d8bbc86e8b8be3579571e308e5ad9f260 100644
>>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
>>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
>>> @@ -820,9 +820,6 @@ irqreturn_t rkisp1_capture_isr(int irq, void *ctx)
>>>   	unsigned int i;
>>>   	u32 status;
>>>   
>>> -	if (!rkisp1->irqs_enabled)
>>> -		return IRQ_NONE;
>>> -
>>>   	status = rkisp1_read(rkisp1, RKISP1_CIF_MI_MIS);
>>>   	if (!status)
>>>   		return IRQ_NONE;
>>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
>>> index 5e6a4d5f6fd12baf45a0083eff75de1095162b2d..2a5f6f69b217cdba2fa7c4d1f230ede5aff49149 100644
>>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
>>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
>>> @@ -507,7 +507,6 @@ struct rkisp1_debug {
>>>    * @debug:	   debug params to be exposed on debugfs
>>>    * @info:	   version-specific ISP information
>>>    * @irqs:          IRQ line numbers
>>> - * @irqs_enabled:  the hardware is enabled and can cause interrupts
>>>    */
>>>   struct rkisp1_device {
>>>   	void __iomem *base_addr;
>>> @@ -532,7 +531,6 @@ struct rkisp1_device {
>>>   	struct rkisp1_debug debug;
>>>   	const struct rkisp1_info *info;
>>>   	int irqs[RKISP1_NUM_IRQS];
>>> -	bool irqs_enabled;
>>>   };
>>>   
>>>   /*
>>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
>>> index ddc6182f3e4bdacdd1962c86f6259334b16aa505..bfc33365ad9d09ccace4ccbb2b19a2fbe1b77eb2 100644
>>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
>>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
>>> @@ -196,9 +196,6 @@ irqreturn_t rkisp1_csi_isr(int irq, void *ctx)
>>>   	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
>>>   	u32 val, status;
>>>   
>>> -	if (!rkisp1->irqs_enabled)
>>> -		return IRQ_NONE;
>>> -
>>>   	status = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_MIS);
>>>   	if (!status)
>>>   		return IRQ_NONE;
>>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
>>> index 1791c02a40ae18205f5eb2fd6edca6cda6b459bf..6fa76423bacf3e92cbbb4ef1ceb55e194b88d963 100644
>>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
>>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
>>> @@ -307,28 +307,62 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1)
>>>    * Power
>>>    */
>>>   
>>> -static int __maybe_unused rkisp1_runtime_suspend(struct device *dev)
>>> +static void rkisp1_free_irqs(struct rkisp1_device *rkisp1)
>>>   {
>>> -	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
>>> +	for (unsigned int i = 0; i < ARRAY_SIZE(rkisp1->irqs); i++) {
>>> +		if (rkisp1->irqs[i] == -1)
>>> +			continue;
>>>   
>>> -	rkisp1->irqs_enabled = false;
>>> -	/* Make sure the IRQ handler will see the above */
>>> -	mb();
>>> +		if (irq_has_action(rkisp1->irqs[i]))
>>> +			free_irq(rkisp1->irqs[i], rkisp1->dev);
>>> +	}
>>> +}
>>>   
>>> -	/*
>>> -	 * Wait until any running IRQ handler has returned. The IRQ handler
>>> -	 * may get called even after this (as it's a shared interrupt line)
>>> -	 * but the 'irqs_enabled' flag will make the handler return immediately.
>>> -	 */
>>> -	for (unsigned int il = 0; il < ARRAY_SIZE(rkisp1->irqs); ++il) {
>>> -		if (rkisp1->irqs[il] == -1)
>>> +static int rkisp1_request_irqs(struct rkisp1_device *rkisp1)
>>> +{
>>> +	const struct rkisp1_info *info = rkisp1->info;
>>> +	int ret;
>>> +
>>> +	for (unsigned int irqn = 0; irqn < ARRAY_SIZE(rkisp1->irqs); irqn++) {
>>> +		unsigned int isrn;
>>> +
>>> +		if (rkisp1->irqs[irqn] == -1)
>>>   			continue;
>>>   
>>> -		/* Skip if the irq line is the same as previous */
>>> -		if (il == 0 || rkisp1->irqs[il - 1] != rkisp1->irqs[il])
>>> -			synchronize_irq(rkisp1->irqs[il]);
>>> +		if (irq_has_action(rkisp1->irqs[irqn]))
>>> +			continue;
>>> +
>>> +		for (isrn = 0; isrn < info->isr_size; isrn++)
>>> +			if ((info->isrs[isrn].line_mask & BIT(irqn)))
>>> +				break;
>>> +
>>> +		if (isrn == info->isr_size) {
>>> +			dev_err(rkisp1->dev, "Failed to find IRQ handler\n");
>>> +			return -EINVAL;
>>> +		}
>>> +
>>> +		ret = request_irq(rkisp1->irqs[irqn], info->isrs[isrn].isr,
>>> +				  IRQF_SHARED, dev_driver_string(rkisp1->dev),
>>> +				  rkisp1->dev);
>>> +		if (ret) {
>>> +			dev_err(rkisp1->dev, "Failed to request IRQ\n");
>>> +			goto err_free_irqs;
>>> +		}
>>>   	}
>>>   
>>> +	return 0;
>>> +
>>> +err_free_irqs:
>>> +	rkisp1_free_irqs(rkisp1);
>>> +	return ret;
>>> +}
>>> +
>>> +static int __maybe_unused rkisp1_runtime_suspend(struct device *dev)
>>> +{
>>> +	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
>>> +
>>> +	rkisp1_free_irqs(rkisp1);
>>> +
>>>   	clk_bulk_disable_unprepare(rkisp1->clk_size, rkisp1->clks);
>>>   	return pinctrl_pm_select_sleep_state(dev);
>>>   }
>>> @@ -345,11 +379,16 @@ static int __maybe_unused rkisp1_runtime_resume(struct device *dev)
>>>   	if (ret)
>>>   		return ret;
>>>   
>>> -	rkisp1->irqs_enabled = true;
>>> -	/* Make sure the IRQ handler will see the above */
>>> -	mb();
>>> +	ret = rkisp1_request_irqs(rkisp1);
>>> +	if (ret)
>>> +		goto err_clk_disable;
>>>   
>>>   	return 0;
>>> +
>>> +err_clk_disable:
>>> +	clk_bulk_disable_unprepare(rkisp1->clk_size, rkisp1->clks);
>>> +
>>> +	return ret;
>>>   }
>>>   
>>>   static const struct dev_pm_ops rkisp1_pm_ops = {
>>> @@ -694,13 +733,6 @@ static int rkisp1_probe(struct platform_device *pdev)
>>>   			if (info->isrs[i].line_mask & BIT(il))
>>>   				rkisp1->irqs[il] = irq;
>>>   		}
>>> -
>>> -		ret = devm_request_irq(dev, irq, info->isrs[i].isr, IRQF_SHARED,
>>> -				       dev_driver_string(dev), dev);
>>> -		if (ret) {
>>> -			dev_err(dev, "request irq failed: %d\n", ret);
>>> -			return ret;
>>> -		}
>>>   	}
>>>   
>>>   	ret = rkisp1_init_clocks(rkisp1);
>>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>>> index 2311672cedb1b6c9dd7f1b883adcac1516a685ae..c6b1ecd2d0c260f6739726c9f32562b98ca31364 100644
>>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>>> @@ -1106,9 +1106,6 @@ irqreturn_t rkisp1_isp_isr(int irq, void *ctx)
>>>   	struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
>>>   	u32 status, isp_err;
>>>   
>>> -	if (!rkisp1->irqs_enabled)
>>> -		return IRQ_NONE;
>>> -
>>>   	status = rkisp1_read(rkisp1, RKISP1_CIF_ISP_MIS);
>>>   	if (!status)
>>>   		return IRQ_NONE;
>>>
>>> ---
>>> base-commit: ee5b462b97162dbb6c536e18a37b3048f6520019
>>> change-id: 20260105-rkisp1-irqs-8af5a1e0b887
>