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

Laurent Pinchart
2026-01-05T18:07:54Z

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