| Message ID | Pine.LNX.4.64.1305290841080.14157@axis700.grange (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On 05/29/2013 02:58 PM, Guennadi Liakhovetski wrote: > Hi > > On Tue, 28 May 2013, Wei Yongjun wrote: > >> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> >> >> Fix to return a negative error code instead of 0 when we cannot get >> IRQ source by platform_get_irq(), as done elsewhere in this function. >> >> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > Thanks for the patch. Do you think the following version would be even a > bit simpler? > > diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c > index fe90853..76661b6 100644 > --- a/drivers/mmc/host/sh_mobile_sdhi.c > +++ b/drivers/mmc/host/sh_mobile_sdhi.c > @@ -255,18 +255,17 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev) > if (multiplexed_isr) { > while (1) { > irq = platform_get_irq(pdev, i); > - if (irq < 0) > - break; > + /* There must be at least one IRQ source */ > + if (irq < 0) { > + ret = irq; > + goto eirq; > + } > i++; > ret = devm_request_irq(&pdev->dev, irq, tmio_mmc_irq, 0, > dev_name(&pdev->dev), host); > if (ret) > goto eirq; > } > - > - /* There must be at least one IRQ source */ > - if (!i) > - goto eirq; > } > > dev_info(&pdev->dev, "%s base at 0x%08lx clock rate %u MHz\n", The orig code used while loop to get many irqs, but with your change, we will leave the loop util platform_get_irq() or request_irq() fail and nerver success. Do your means remove the while loop and this device need only one irq? > > If you agree, maybe you could send a v2 of your patch in this form, I'll > ack it then. > > Thanks > Guennadi > >> --- >> drivers/mmc/host/sh_mobile_sdhi.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c >> index cc4c872..a4316b3 100644 >> --- a/drivers/mmc/host/sh_mobile_sdhi.c >> +++ b/drivers/mmc/host/sh_mobile_sdhi.c >> @@ -273,8 +273,10 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev) >> } >> >> /* There must be at least one IRQ source */ >> - if (!i) >> + if (!i) { >> + ret = irq; >> goto eirq; >> + } >> } >> >> dev_info(&pdev->dev, "%s base at 0x%08lx clock rate %u MHz\n", >> > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 29 May 2013, Wei Yongjun wrote: > Hi > > On 05/29/2013 02:58 PM, Guennadi Liakhovetski wrote: > > Hi > > > > On Tue, 28 May 2013, Wei Yongjun wrote: > > > >> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > >> > >> Fix to return a negative error code instead of 0 when we cannot get > >> IRQ source by platform_get_irq(), as done elsewhere in this function. > >> > >> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > > Thanks for the patch. Do you think the following version would be even a > > bit simpler? > > > > diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c > > index fe90853..76661b6 100644 > > --- a/drivers/mmc/host/sh_mobile_sdhi.c > > +++ b/drivers/mmc/host/sh_mobile_sdhi.c > > @@ -255,18 +255,17 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev) > > if (multiplexed_isr) { > > while (1) { > > irq = platform_get_irq(pdev, i); > > - if (irq < 0) > > - break; > > + /* There must be at least one IRQ source */ > > + if (irq < 0) { > > + ret = irq; > > + goto eirq; > > + } > > i++; > > ret = devm_request_irq(&pdev->dev, irq, tmio_mmc_irq, 0, > > dev_name(&pdev->dev), host); > > if (ret) > > goto eirq; > > } > > - > > - /* There must be at least one IRQ source */ > > - if (!i) > > - goto eirq; > > } > > > > dev_info(&pdev->dev, "%s base at 0x%08lx clock rate %u MHz\n", > > The orig code used while loop to get many irqs, but with your change, > we will leave the loop util platform_get_irq() or request_irq() fail > and nerver success. Mmmh, ok, right. You mean, the original code wouldn't error out if at least 1 IRQ had been detected, whereas my change would, which is wrong. Sorry, forget about my version, yours was right. Acked-by: Guennadi Liakhovetski <g.liakhovetski+renesas@googlemail.com> Thanks Guennadi > Do your means remove the while loop and this device > need only one irq? > > > > > If you agree, maybe you could send a v2 of your patch in this form, I'll > > ack it then. > > > > Thanks > > Guennadi > > > >> --- > >> drivers/mmc/host/sh_mobile_sdhi.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c > >> index cc4c872..a4316b3 100644 > >> --- a/drivers/mmc/host/sh_mobile_sdhi.c > >> +++ b/drivers/mmc/host/sh_mobile_sdhi.c > >> @@ -273,8 +273,10 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev) > >> } > >> > >> /* There must be at least one IRQ source */ > >> - if (!i) > >> + if (!i) { > >> + ret = irq; > >> goto eirq; > >> + } > >> } > >> > >> dev_info(&pdev->dev, "%s base at 0x%08lx clock rate %u MHz\n", > >> --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c index fe90853..76661b6 100644 --- a/drivers/mmc/host/sh_mobile_sdhi.c +++ b/drivers/mmc/host/sh_mobile_sdhi.c @@ -255,18 +255,17 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev) if (multiplexed_isr) { while (1) { irq = platform_get_irq(pdev, i); - if (irq < 0) - break; + /* There must be at least one IRQ source */ + if (irq < 0) { + ret = irq; + goto eirq; + } i++; ret = devm_request_irq(&pdev->dev, irq, tmio_mmc_irq, 0, dev_name(&pdev->dev), host); if (ret) goto eirq; } - - /* There must be at least one IRQ source */ - if (!i) - goto eirq; } dev_info(&pdev->dev, "%s base at 0x%08lx clock rate %u MHz\n",