| Message ID | 20211021030938.51884-1-tangbin@cmss.chinamobile.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | af88c2adbb72a09ab1bb5c37ba388c98fecca69b |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: rcar_fdp1: Fix the correct variable assignments | expand |
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? > 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> 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
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
diff --git a/drivers/media/platform/rcar_fdp1.c b/drivers/media/platform/rcar_fdp1.c index 89aac6006..d79bf1461 100644 --- 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);
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. Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> --- drivers/media/platform/rcar_fdp1.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)