On Tue, Dec 5, 2023 at 6:02 AM Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> wrote: > > On 05/12/2023 13:57, Adam Ford wrote: > > On Tue, Dec 5, 2023 at 2:10 AM Tomi Valkeinen > > <tomi.valkeinen@xxxxxxxxxxxxxxxx> wrote: > >> > >> The IRQ handler rkisp1_isr() calls sub-handlers, all of which returns an > >> irqreturn_t value, but rkisp1_isr() ignores those values and always > >> returns IRQ_HANDLED. > >> > >> Fix this by collecting the return values, and returning IRQ_HANDLED or > >> IRQ_NONE as appropriate. > >> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> > >> --- > >> drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 18 ++++++++++++++---- > >> 1 file changed, 14 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > >> index 76f93614b4cf..1d60f4b8bd09 100644 > >> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > >> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > >> @@ -445,17 +445,27 @@ static int rkisp1_entities_register(struct rkisp1_device *rkisp1) > >> > >> static irqreturn_t rkisp1_isr(int irq, void *ctx) > >> { > >> + irqreturn_t ret; > >> + > >> /* > >> * Call rkisp1_capture_isr() first to handle the frame that > >> * potentially completed using the current frame_sequence number before > >> * it is potentially incremented by rkisp1_isp_isr() in the vertical > >> * sync. > >> */ > >> - rkisp1_capture_isr(irq, ctx); > >> - rkisp1_isp_isr(irq, ctx); > >> - rkisp1_csi_isr(irq, ctx); > >> > >> - return IRQ_HANDLED; > >> + ret = IRQ_NONE; > >> + > >> + if (rkisp1_capture_isr(irq, ctx) == IRQ_HANDLED) > >> + ret = IRQ_HANDLED; > >> + > >> + if (rkisp1_isp_isr(irq, ctx) == IRQ_HANDLED) > >> + ret = IRQ_HANDLED; > >> + > >> + if (rkisp1_csi_isr(irq, ctx) == IRQ_HANDLED) > >> + ret = IRQ_HANDLED; > >> + > > > > It seems like we're throwing away the value of ret each time the > > subsequent if statement is evaluated. Whether or not they return > > didn't matter before, and the only one that seems using the return > > code is the last one. > > > > Wouldn't it be simpler to use ret = rkisp1_capture_isr(irq, ctx), ret > > = rkisp1_isp_isr(irq, ctx) and ret = rkisp1_csi_isr(irq, ctx) if we > > care about the return code? > > > > How do you expect this to return if one of the first two don't return > > IRQ_HANDLED? > > I'm sorry, I don't quite follow what you mean. Can you elaborate a bit? > > We want the rkisp1_isr() to return IRQ_NONE if none of the sub-handlers > handled the interrupt. Otherwise, if any of the sub-handlers return > IRQ_HANDLED, rkisp1_isr() returns IRQ_HANDLED. OK. I understand your explanation. I retract my comment. I'll try to test this series tonight on my imx8mp adam > > Tomi >