Messages in this thread | | | Subject | Re: [PATCH] media: rcar_fdp1: Fix the correct variable assignments | From | Kieran Bingham <> | Date | Thu, 21 Oct 2021 10:08:43 +0100 |
| |
Hi Tang,
Quoting Geert Uytterhoeven (2021-10-21 08:59:18) > Hi Tang, > > Thanks for your patch! > > On Thu, Oct 21, 2021 at 5:10 AM Tang Bin <tangbin@cmss.chinamobile.com> wrote: > > In the function fdp1_probe(), when get irq failed, the > > function platform_get_irq() log an error message, so > > remove redundant message here. And the variable type > > of "ret" is int, the "fdp1->irq" is unsigned int, when > > irq failed, this place maybe wrong, thus fix it. > > The second issue is not actually present, as the error check > operates on ret, not fdp1->irq?
Agreed, the error print is redundant.
In fact it would have erroneously print on ret=-EPROBE_DEFER cases too, so it's not just redundant, but inaccurate too.
I don't think the assignment of fdp1->irq = ret at the same time is an issue, because if ret < 0, fdp1->irq wouldn't ever get read, as the call returns.
But .. I have no objection to setting it after instead.
> > Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> > > > --- a/drivers/media/platform/rcar_fdp1.c > > +++ b/drivers/media/platform/rcar_fdp1.c > > @@ -2289,11 +2289,10 @@ static int fdp1_probe(struct platform_device *pdev) > > return PTR_ERR(fdp1->regs); > > > > /* Interrupt service routine registration */ > > - fdp1->irq = ret = platform_get_irq(pdev, 0); > > - if (ret < 0) { > > - dev_err(&pdev->dev, "cannot find IRQ\n"); > > + ret = platform_get_irq(pdev, 0); > > + if (ret < 0) > > return ret; > > - } > > + fdp1->irq = ret; > > > > ret = devm_request_irq(&pdev->dev, fdp1->irq, fdp1_irq_handler, 0, > > dev_name(&pdev->dev), fdp1); > > Anyway, the code is correct, so: > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Perhaps with the commit message updated/simplified, but either way:
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
| |