Message ID | 1474276878-1021-1-git-send-email-u.kleine-koenig@pengutronix.de |
---|---|
State | Accepted |
Headers | show |
On Mon, 19 Sep 2016 11:21:18 +0200 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > For some error paths alloc_nand_resource() emitted an error message, for > others it didn't. Make it consistently print a message including the > error code where it's not constant and drop the hardly helpful > additional message printed by the caller of alloc_nand_resource. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> It looks good to me. Ezequiel, Robert, any objection to this patch? If not, can I have your Acked/Reviewed-by ? > --- > drivers/mtd/nand/pxa3xx_nand.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c > index 436dd6dc11f4..052e9725cf12 100644 > --- a/drivers/mtd/nand/pxa3xx_nand.c > +++ b/drivers/mtd/nand/pxa3xx_nand.c > @@ -1774,8 +1774,11 @@ static int alloc_nand_resource(struct platform_device *pdev) > int ret, irq, cs; > > pdata = dev_get_platdata(&pdev->dev); > - if (pdata->num_cs <= 0) > + if (pdata->num_cs <= 0) { > + dev_err(&pdev->dev, "broken number of chip selects\n"); > return -ENODEV; > + } > + > info = devm_kzalloc(&pdev->dev, > sizeof(*info) + sizeof(*host) * pdata->num_cs, > GFP_KERNEL); > @@ -1814,8 +1817,9 @@ static int alloc_nand_resource(struct platform_device *pdev) > init_waitqueue_head(&chip->controller->wq); > info->clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(info->clk)) { > - dev_err(&pdev->dev, "failed to get nand clock\n"); > - return PTR_ERR(info->clk); > + ret = PTR_ERR(info->clk); > + dev_err(&pdev->dev, "failed to get nand clock (%d)\n", ret); > + return ret; > } > ret = clk_prepare_enable(info->clk); > if (ret < 0) > @@ -1843,6 +1847,7 @@ static int alloc_nand_resource(struct platform_device *pdev) > info->mmio_base = devm_ioremap_resource(&pdev->dev, r); > if (IS_ERR(info->mmio_base)) { > ret = PTR_ERR(info->mmio_base); > + dev_err(&pdev->dev, "failed to map register space (%d)\n", ret); > goto fail_disable_clk; > } > info->mmio_phys = r->start; > @@ -1862,7 +1867,7 @@ static int alloc_nand_resource(struct platform_device *pdev) > pxa3xx_nand_irq_thread, IRQF_ONESHOT, > pdev->name, info); > if (ret < 0) { > - dev_err(&pdev->dev, "failed to request IRQ\n"); > + dev_err(&pdev->dev, "failed to request IRQ (%d)\n", ret); > goto fail_free_buf; > } > > @@ -1961,10 +1966,8 @@ static int pxa3xx_nand_probe(struct platform_device *pdev) > } > > ret = alloc_nand_resource(pdev); > - if (ret) { > - dev_err(&pdev->dev, "alloc nand resource failed\n"); > + if (ret) > return ret; > - } > > info = platform_get_drvdata(pdev); > probe_success = 0;
On 10/28/2016 10:04 AM, Boris Brezillon wrote: > On Mon, 19 Sep 2016 11:21:18 +0200 > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > >> For some error paths alloc_nand_resource() emitted an error message, for >> others it didn't. Make it consistently print a message including the >> error code where it's not constant and drop the hardly helpful >> additional message printed by the caller of alloc_nand_resource. >> >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > It looks good to me. Ezequiel, Robert, any objection to this patch? If > not, can I have your Acked/Reviewed-by ? > >> --- >> drivers/mtd/nand/pxa3xx_nand.c | 17 ++++++++++------- >> 1 file changed, 10 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c >> index 436dd6dc11f4..052e9725cf12 100644 >> --- a/drivers/mtd/nand/pxa3xx_nand.c >> +++ b/drivers/mtd/nand/pxa3xx_nand.c >> @@ -1774,8 +1774,11 @@ static int alloc_nand_resource(struct platform_device *pdev) >> int ret, irq, cs; >> >> pdata = dev_get_platdata(&pdev->dev); >> - if (pdata->num_cs <= 0) >> + if (pdata->num_cs <= 0) { >> + dev_err(&pdev->dev, "broken number of chip selects\n"); Not: Should be "invalid number of chip selects" . Also, if you turn the num_cs to unsigned type in the platdata, the test can be if (foo == 0) instead. >> return -ENODEV; >> + } >> + >> info = devm_kzalloc(&pdev->dev, >> sizeof(*info) + sizeof(*host) * pdata->num_cs, >> GFP_KERNEL); >> @@ -1814,8 +1817,9 @@ static int alloc_nand_resource(struct platform_device *pdev) >> init_waitqueue_head(&chip->controller->wq); >> info->clk = devm_clk_get(&pdev->dev, NULL); >> if (IS_ERR(info->clk)) { >> - dev_err(&pdev->dev, "failed to get nand clock\n"); >> - return PTR_ERR(info->clk); >> + ret = PTR_ERR(info->clk); >> + dev_err(&pdev->dev, "failed to get nand clock (%d)\n", ret); >> + return ret; >> } >> ret = clk_prepare_enable(info->clk); >> if (ret < 0) >> @@ -1843,6 +1847,7 @@ static int alloc_nand_resource(struct platform_device *pdev) >> info->mmio_base = devm_ioremap_resource(&pdev->dev, r); >> if (IS_ERR(info->mmio_base)) { >> ret = PTR_ERR(info->mmio_base); >> + dev_err(&pdev->dev, "failed to map register space (%d)\n", ret); >> goto fail_disable_clk; >> } >> info->mmio_phys = r->start; >> @@ -1862,7 +1867,7 @@ static int alloc_nand_resource(struct platform_device *pdev) >> pxa3xx_nand_irq_thread, IRQF_ONESHOT, >> pdev->name, info); >> if (ret < 0) { >> - dev_err(&pdev->dev, "failed to request IRQ\n"); >> + dev_err(&pdev->dev, "failed to request IRQ (%d)\n", ret); >> goto fail_free_buf; >> } >> >> @@ -1961,10 +1966,8 @@ static int pxa3xx_nand_probe(struct platform_device *pdev) >> } >> >> ret = alloc_nand_resource(pdev); >> - if (ret) { >> - dev_err(&pdev->dev, "alloc nand resource failed\n"); >> + if (ret) >> return ret; >> - } >> >> info = platform_get_drvdata(pdev); >> probe_success = 0; > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ >
On Sat, 29 Oct 2016 19:56:39 +0200 Marek Vasut <marek.vasut@gmail.com> wrote: > On 10/28/2016 10:04 AM, Boris Brezillon wrote: > > On Mon, 19 Sep 2016 11:21:18 +0200 > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > > >> For some error paths alloc_nand_resource() emitted an error message, for > >> others it didn't. Make it consistently print a message including the > >> error code where it's not constant and drop the hardly helpful > >> additional message printed by the caller of alloc_nand_resource. > >> > >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > It looks good to me. Ezequiel, Robert, any objection to this patch? If > > not, can I have your Acked/Reviewed-by ? > > > >> --- > >> drivers/mtd/nand/pxa3xx_nand.c | 17 ++++++++++------- > >> 1 file changed, 10 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c > >> index 436dd6dc11f4..052e9725cf12 100644 > >> --- a/drivers/mtd/nand/pxa3xx_nand.c > >> +++ b/drivers/mtd/nand/pxa3xx_nand.c > >> @@ -1774,8 +1774,11 @@ static int alloc_nand_resource(struct platform_device *pdev) > >> int ret, irq, cs; > >> > >> pdata = dev_get_platdata(&pdev->dev); > >> - if (pdata->num_cs <= 0) > >> + if (pdata->num_cs <= 0) { > >> + dev_err(&pdev->dev, "broken number of chip selects\n"); > > Not: Should be "invalid number of chip selects" . Correct. I can fix that when applying. > Also, if you turn the > num_cs to unsigned type in the platdata, the test can be if (foo == 0) > instead. If we really want to turn num_cs into an unsigned int, then it should be done in different patch, but honestly, that's not the first thing I would rework in this driver ;-). > > >> return -ENODEV; > >> + } > >> + > >> info = devm_kzalloc(&pdev->dev, > >> sizeof(*info) + sizeof(*host) * pdata->num_cs, > >> GFP_KERNEL); > >> @@ -1814,8 +1817,9 @@ static int alloc_nand_resource(struct platform_device *pdev) > >> init_waitqueue_head(&chip->controller->wq); > >> info->clk = devm_clk_get(&pdev->dev, NULL); > >> if (IS_ERR(info->clk)) { > >> - dev_err(&pdev->dev, "failed to get nand clock\n"); > >> - return PTR_ERR(info->clk); > >> + ret = PTR_ERR(info->clk); > >> + dev_err(&pdev->dev, "failed to get nand clock (%d)\n", ret); > >> + return ret; > >> } > >> ret = clk_prepare_enable(info->clk); > >> if (ret < 0) > >> @@ -1843,6 +1847,7 @@ static int alloc_nand_resource(struct platform_device *pdev) > >> info->mmio_base = devm_ioremap_resource(&pdev->dev, r); > >> if (IS_ERR(info->mmio_base)) { > >> ret = PTR_ERR(info->mmio_base); > >> + dev_err(&pdev->dev, "failed to map register space (%d)\n", ret); > >> goto fail_disable_clk; > >> } > >> info->mmio_phys = r->start; > >> @@ -1862,7 +1867,7 @@ static int alloc_nand_resource(struct platform_device *pdev) > >> pxa3xx_nand_irq_thread, IRQF_ONESHOT, > >> pdev->name, info); > >> if (ret < 0) { > >> - dev_err(&pdev->dev, "failed to request IRQ\n"); > >> + dev_err(&pdev->dev, "failed to request IRQ (%d)\n", ret); > >> goto fail_free_buf; > >> } > >> > >> @@ -1961,10 +1966,8 @@ static int pxa3xx_nand_probe(struct platform_device *pdev) > >> } > >> > >> ret = alloc_nand_resource(pdev); > >> - if (ret) { > >> - dev_err(&pdev->dev, "alloc nand resource failed\n"); > >> + if (ret) > >> return ret; > >> - } > >> > >> info = platform_get_drvdata(pdev); > >> probe_success = 0; > > > > > > ______________________________________________________ > > Linux MTD discussion mailing list > > http://lists.infradead.org/mailman/listinfo/linux-mtd/ > > > >
Marek Vasut <marek.vasut@gmail.com> writes: > On 10/28/2016 10:04 AM, Boris Brezillon wrote: >> On Mon, 19 Sep 2016 11:21:18 +0200 >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: >> >>> For some error paths alloc_nand_resource() emitted an error message, for >>> others it didn't. Make it consistently print a message including the >>> error code where it's not constant and drop the hardly helpful >>> additional message printed by the caller of alloc_nand_resource. >>> >>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >> >> It looks good to me. Ezequiel, Robert, any objection to this patch? If >> not, can I have your Acked/Reviewed-by ? Hi, Actually I was wondering if my mail system is utterly broken ... This is the first mail I receive, while it seems to me I was in the recipients in the former ones ... You can take my Reviewed-by, at least from: https://patchwork.ozlabs.org/patch/671644/ As a side note, normally for pxa architecture we use a printk format to display the return value as : pr_something("XXXX :%d\n", ret) As pxa3xx_nand is shared with other architectures this will be Boris/Ezequiel's call. Cheers. -- Robert
On Mon, 19 Sep 2016 11:21:18 +0200 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > For some error paths alloc_nand_resource() emitted an error message, for > others it didn't. Make it consistently print a message including the > error code where it's not constant and drop the hardly helpful > additional message printed by the caller of alloc_nand_resource. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Applied after addressing Marek and Robert comments. Thanks, Boris > --- > drivers/mtd/nand/pxa3xx_nand.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c > index 436dd6dc11f4..052e9725cf12 100644 > --- a/drivers/mtd/nand/pxa3xx_nand.c > +++ b/drivers/mtd/nand/pxa3xx_nand.c > @@ -1774,8 +1774,11 @@ static int alloc_nand_resource(struct platform_device *pdev) > int ret, irq, cs; > > pdata = dev_get_platdata(&pdev->dev); > - if (pdata->num_cs <= 0) > + if (pdata->num_cs <= 0) { > + dev_err(&pdev->dev, "broken number of chip selects\n"); > return -ENODEV; > + } > + > info = devm_kzalloc(&pdev->dev, > sizeof(*info) + sizeof(*host) * pdata->num_cs, > GFP_KERNEL); > @@ -1814,8 +1817,9 @@ static int alloc_nand_resource(struct platform_device *pdev) > init_waitqueue_head(&chip->controller->wq); > info->clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(info->clk)) { > - dev_err(&pdev->dev, "failed to get nand clock\n"); > - return PTR_ERR(info->clk); > + ret = PTR_ERR(info->clk); > + dev_err(&pdev->dev, "failed to get nand clock (%d)\n", ret); > + return ret; > } > ret = clk_prepare_enable(info->clk); > if (ret < 0) > @@ -1843,6 +1847,7 @@ static int alloc_nand_resource(struct platform_device *pdev) > info->mmio_base = devm_ioremap_resource(&pdev->dev, r); > if (IS_ERR(info->mmio_base)) { > ret = PTR_ERR(info->mmio_base); > + dev_err(&pdev->dev, "failed to map register space (%d)\n", ret); > goto fail_disable_clk; > } > info->mmio_phys = r->start; > @@ -1862,7 +1867,7 @@ static int alloc_nand_resource(struct platform_device *pdev) > pxa3xx_nand_irq_thread, IRQF_ONESHOT, > pdev->name, info); > if (ret < 0) { > - dev_err(&pdev->dev, "failed to request IRQ\n"); > + dev_err(&pdev->dev, "failed to request IRQ (%d)\n", ret); > goto fail_free_buf; > } > > @@ -1961,10 +1966,8 @@ static int pxa3xx_nand_probe(struct platform_device *pdev) > } > > ret = alloc_nand_resource(pdev); > - if (ret) { > - dev_err(&pdev->dev, "alloc nand resource failed\n"); > + if (ret) > return ret; > - } > > info = platform_get_drvdata(pdev); > probe_success = 0;
diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c index 436dd6dc11f4..052e9725cf12 100644 --- a/drivers/mtd/nand/pxa3xx_nand.c +++ b/drivers/mtd/nand/pxa3xx_nand.c @@ -1774,8 +1774,11 @@ static int alloc_nand_resource(struct platform_device *pdev) int ret, irq, cs; pdata = dev_get_platdata(&pdev->dev); - if (pdata->num_cs <= 0) + if (pdata->num_cs <= 0) { + dev_err(&pdev->dev, "broken number of chip selects\n"); return -ENODEV; + } + info = devm_kzalloc(&pdev->dev, sizeof(*info) + sizeof(*host) * pdata->num_cs, GFP_KERNEL); @@ -1814,8 +1817,9 @@ static int alloc_nand_resource(struct platform_device *pdev) init_waitqueue_head(&chip->controller->wq); info->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(info->clk)) { - dev_err(&pdev->dev, "failed to get nand clock\n"); - return PTR_ERR(info->clk); + ret = PTR_ERR(info->clk); + dev_err(&pdev->dev, "failed to get nand clock (%d)\n", ret); + return ret; } ret = clk_prepare_enable(info->clk); if (ret < 0) @@ -1843,6 +1847,7 @@ static int alloc_nand_resource(struct platform_device *pdev) info->mmio_base = devm_ioremap_resource(&pdev->dev, r); if (IS_ERR(info->mmio_base)) { ret = PTR_ERR(info->mmio_base); + dev_err(&pdev->dev, "failed to map register space (%d)\n", ret); goto fail_disable_clk; } info->mmio_phys = r->start; @@ -1862,7 +1867,7 @@ static int alloc_nand_resource(struct platform_device *pdev) pxa3xx_nand_irq_thread, IRQF_ONESHOT, pdev->name, info); if (ret < 0) { - dev_err(&pdev->dev, "failed to request IRQ\n"); + dev_err(&pdev->dev, "failed to request IRQ (%d)\n", ret); goto fail_free_buf; } @@ -1961,10 +1966,8 @@ static int pxa3xx_nand_probe(struct platform_device *pdev) } ret = alloc_nand_resource(pdev); - if (ret) { - dev_err(&pdev->dev, "alloc nand resource failed\n"); + if (ret) return ret; - } info = platform_get_drvdata(pdev); probe_success = 0;
For some error paths alloc_nand_resource() emitted an error message, for others it didn't. Make it consistently print a message including the error code where it's not constant and drop the hardly helpful additional message printed by the caller of alloc_nand_resource. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/mtd/nand/pxa3xx_nand.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)