From: Helen Koike <helen.koike@collabora.com>
To: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>,
linux-media@vger.kernel.org
Cc: ezequiel@collabora.com, hverkuil@xs4all.nl, kernel@collabora.com,
dafna3@gmail.com, sakari.ailus@linux.intel.com,
linux-rockchip@lists.infradead.org, mchehab@kernel.org,
laurent.pinchart@ideasonboard.com
Subject: Re: [PATCH 1/5] media: staging: rkisp1: return IRQ_NONE in isr when irq isn't for ISP
Date: Wed, 20 May 2020 07:58:41 -0300 [thread overview]
Message-ID: <33703448-c89b-b1ba-eedb-3ac769beaca3@collabora.com> (raw)
In-Reply-To: <20200512120522.25960-2-dafna.hirschfeld@collabora.com>
Hi Dafna,
Thanks for the patch.
On 5/12/20 9:05 AM, Dafna Hirschfeld wrote:
> From: Helen Koike <helen.koike@collabora.com>
>
> rkisp1 shares the interrupt line, then it shouldn't always return
> IRQ_HANDLED, otherwise it can flag as handled an interrupt that wans't
> meant for ISP.
>
> return IRQ_NONE when the interrupt wans't meant for ISP
>
> Fixes: d65dd85281fb ("media: staging: rkisp1: add Rockchip ISP1 base driver")
>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
> drivers/staging/media/rkisp1/rkisp1-capture.c | 7 ++++++-
> drivers/staging/media/rkisp1/rkisp1-common.h | 6 +++---
> drivers/staging/media/rkisp1/rkisp1-dev.c | 14 ++++++++++----
> drivers/staging/media/rkisp1/rkisp1-isp.c | 12 ++++++++----
> 4 files changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index f69235f82c45..19021875e8a9 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -649,12 +649,15 @@ static void rkisp1_handle_buffer(struct rkisp1_capture *cap)
> rkisp1_set_next_buf(cap);
> }
>
> -void rkisp1_capture_isr(struct rkisp1_device *rkisp1)
> +irqreturn_t rkisp1_capture_isr(struct rkisp1_device *rkisp1)
> {
> unsigned int i;
> u32 status;
>
> status = rkisp1_read(rkisp1, RKISP1_CIF_MI_MIS);
> + if (!status)
> + return IRQ_NONE;
> +
> rkisp1_write(rkisp1, status, RKISP1_CIF_MI_ICR);
>
> for (i = 0; i < ARRAY_SIZE(rkisp1->capture_devs); ++i) {
> @@ -682,6 +685,8 @@ void rkisp1_capture_isr(struct rkisp1_device *rkisp1)
> cap->is_streaming = false;
> wake_up(&cap->done);
> }
> +
> + return IRQ_HANDLED;
> }
>
> /* ----------------------------------------------------------------------------
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 0c4fe503adc9..33dffe21c769 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -305,9 +305,9 @@ void rkisp1_isp_unregister(struct rkisp1_device *rkisp1);
>
> const struct rkisp1_isp_mbus_info *rkisp1_isp_mbus_info_get(u32 mbus_code);
>
> -void rkisp1_isp_isr(struct rkisp1_device *rkisp1);
> -void rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
> -void rkisp1_capture_isr(struct rkisp1_device *rkisp1);
> +irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1);
> +irqreturn_t rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
> +irqreturn_t rkisp1_capture_isr(struct rkisp1_device *rkisp1);
> void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris);
> void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis);
>
> diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
> index 9ac38bafb839..b7f43dab71c8 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-dev.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
> @@ -387,10 +387,13 @@ static int rkisp1_entities_register(struct rkisp1_device *rkisp1)
> return ret;
> }
>
> -static irqreturn_t rkisp1_isr(int irq, void *ctx)
> +irqreturn_t rkisp1_isr(int irq, void *ctx)
> {
> struct device *dev = ctx;
> struct rkisp1_device *rkisp1 = dev_get_drvdata(dev);
> + irqreturn_t isp_ret;
> + irqreturn_t cap_ret;
> + irqreturn_t mipi_ret;
Just cosmetics, you could declare them in a single line
irqreturn_t cap_ret, isp_ret, mipi_ret;
With or without this change:
Acked-by: Helen Koike <helen.koike@collabora.com>
Thanks
Helen
>
> /*
> * Call rkisp1_capture_isr() first to handle the frame that
> @@ -398,9 +401,12 @@ static irqreturn_t rkisp1_isr(int irq, void *ctx)
> * it is potentially incremented by rkisp1_isp_isr() in the vertical
> * sync.
> */
> - rkisp1_capture_isr(rkisp1);
> - rkisp1_isp_isr(rkisp1);
> - rkisp1_mipi_isr(rkisp1);
> + cap_ret = rkisp1_capture_isr(rkisp1);
> + isp_ret = rkisp1_isp_isr(rkisp1);
> + mipi_ret = rkisp1_mipi_isr(rkisp1);
> +
> + if (isp_ret == IRQ_NONE && cap_ret == IRQ_NONE && mipi_ret == IRQ_NONE)
> + return IRQ_NONE;
>
> return IRQ_HANDLED;
> }
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index dc2b59a0160a..19ab0ed323aa 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -1046,13 +1046,13 @@ void rkisp1_isp_unregister(struct rkisp1_device *rkisp1)
> * Interrupt handlers
> */
>
> -void rkisp1_mipi_isr(struct rkisp1_device *rkisp1)
> +irqreturn_t rkisp1_mipi_isr(struct rkisp1_device *rkisp1)
> {
> u32 val, status;
>
> status = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_MIS);
> if (!status)
> - return;
> + return IRQ_NONE;
>
> rkisp1_write(rkisp1, status, RKISP1_CIF_MIPI_ICR);
>
> @@ -1087,6 +1087,8 @@ void rkisp1_mipi_isr(struct rkisp1_device *rkisp1)
> } else {
> rkisp1->debug.mipi_error++;
> }
> +
> + return IRQ_HANDLED;
> }
>
> static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
> @@ -1106,13 +1108,13 @@ static void rkisp1_isp_queue_event_sof(struct rkisp1_isp *isp)
> v4l2_event_queue(isp->sd.devnode, &event);
> }
>
> -void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
> +irqreturn_t rkisp1_isp_isr(struct rkisp1_device *rkisp1)
> {
> u32 status, isp_err;
>
> status = rkisp1_read(rkisp1, RKISP1_CIF_ISP_MIS);
> if (!status)
> - return;
> + return IRQ_NONE;
>
> rkisp1_write(rkisp1, status, RKISP1_CIF_ISP_ICR);
>
> @@ -1148,4 +1150,6 @@ void rkisp1_isp_isr(struct rkisp1_device *rkisp1)
> * Do the updates in the order of the processing flow.
> */
> rkisp1_params_isr(rkisp1, status);
> +
> + return IRQ_HANDLED;
> }
>
next prev parent reply other threads:[~2020-05-20 10:58 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-12 12:05 [PATCH 0/5] media: staging: rkisp1: change workqueue to threaded irq in stats Dafna Hirschfeld
2020-05-12 12:05 ` [PATCH 1/5] media: staging: rkisp1: return IRQ_NONE in isr when irq isn't for ISP Dafna Hirschfeld
2020-05-12 15:42 ` kbuild test robot
2020-05-20 10:58 ` Helen Koike [this message]
2020-05-20 20:58 ` Laurent Pinchart
2020-05-12 12:05 ` [PATCH 2/5] media: staging: rkisp1: use a macro for the statistics flags mask Dafna Hirschfeld
2020-05-20 11:03 ` Helen Koike
2020-05-20 23:27 ` Laurent Pinchart
2020-05-12 12:05 ` [PATCH 3/5] media: staging: rkisp1: stats: use spin_lock_irqsave for irq_lock Dafna Hirschfeld
2020-05-20 11:11 ` Helen Koike
2020-05-20 19:22 ` Dafna Hirschfeld
2020-05-20 23:40 ` Laurent Pinchart
2020-05-12 12:05 ` [PATCH 4/5] media: staging: rkisp1: stats: replace locks wq_lock, irq_lock with one lock Dafna Hirschfeld
2020-05-20 23:48 ` Laurent Pinchart
2020-05-12 12:05 ` [PATCH 5/5] media: staging: rkisp1: replace workqueue with threaded irq for reading statistics registers Dafna Hirschfeld
2020-05-12 16:41 ` kbuild test robot
2020-05-21 0:09 ` Laurent Pinchart
2020-05-21 10:38 ` Tomasz Figa
2020-05-28 19:19 ` Dafna Hirschfeld
2020-05-28 19:35 ` Tomasz Figa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=33703448-c89b-b1ba-eedb-3ac769beaca3@collabora.com \
--to=helen.koike@collabora.com \
--cc=dafna.hirschfeld@collabora.com \
--cc=dafna3@gmail.com \
--cc=ezequiel@collabora.com \
--cc=hverkuil@xs4all.nl \
--cc=kernel@collabora.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=mchehab@kernel.org \
--cc=sakari.ailus@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).