* [PATCH] usb: bdc: Remove duplicate error message in bdc_probe() @ 2020-09-27 13:42 Tang Bin 2020-09-27 13:45 ` Greg KH 2020-09-28 6:49 ` Chunfeng Yun 0 siblings, 2 replies; 9+ messages in thread From: Tang Bin @ 2020-09-27 13:42 UTC (permalink / raw) To: balbi, gregkh; +Cc: linux-usb, linux-kernel, Tang Bin, Zhang Shengju In this function, we don't need dev_err() message because when something goes wrong, devm_platform_ioremap_resource() can print an error message itself, so remove the redundant one. Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> --- drivers/usb/gadget/udc/bdc/bdc_core.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c b/drivers/usb/gadget/udc/bdc/bdc_core.c index 02a3a7746..9454f179e 100644 --- a/drivers/usb/gadget/udc/bdc/bdc_core.c +++ b/drivers/usb/gadget/udc/bdc/bdc_core.c @@ -508,10 +508,8 @@ static int bdc_probe(struct platform_device *pdev) bdc->clk = clk; bdc->regs = devm_platform_ioremap_resource(pdev, 0); - if (IS_ERR(bdc->regs)) { - dev_err(dev, "ioremap error\n"); + if (IS_ERR(bdc->regs)) return -ENOMEM; - } irq = platform_get_irq(pdev, 0); if (irq < 0) return irq; -- 2.20.1.windows.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: bdc: Remove duplicate error message in bdc_probe() 2020-09-27 13:42 [PATCH] usb: bdc: Remove duplicate error message in bdc_probe() Tang Bin @ 2020-09-27 13:45 ` Greg KH 2020-09-28 8:45 ` Tang Bin 2020-09-28 6:49 ` Chunfeng Yun 1 sibling, 1 reply; 9+ messages in thread From: Greg KH @ 2020-09-27 13:45 UTC (permalink / raw) To: Tang Bin; +Cc: balbi, linux-usb, linux-kernel, Zhang Shengju On Sun, Sep 27, 2020 at 09:42:18PM +0800, Tang Bin wrote: > In this function, we don't need dev_err() message because > when something goes wrong, devm_platform_ioremap_resource() > can print an error message itself, so remove the redundant > one. > > Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> > Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> > --- > drivers/usb/gadget/udc/bdc/bdc_core.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c b/drivers/usb/gadget/udc/bdc/bdc_core.c > index 02a3a7746..9454f179e 100644 > --- a/drivers/usb/gadget/udc/bdc/bdc_core.c > +++ b/drivers/usb/gadget/udc/bdc/bdc_core.c > @@ -508,10 +508,8 @@ static int bdc_probe(struct platform_device *pdev) > bdc->clk = clk; > > bdc->regs = devm_platform_ioremap_resource(pdev, 0); > - if (IS_ERR(bdc->regs)) { > - dev_err(dev, "ioremap error\n"); > + if (IS_ERR(bdc->regs)) > return -ENOMEM; Why not return the error given to us? thanks, greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: bdc: Remove duplicate error message in bdc_probe() 2020-09-27 13:45 ` Greg KH @ 2020-09-28 8:45 ` Tang Bin 2020-09-28 9:40 ` Felipe Balbi 0 siblings, 1 reply; 9+ messages in thread From: Tang Bin @ 2020-09-28 8:45 UTC (permalink / raw) To: Greg KH; +Cc: balbi, linux-usb, linux-kernel, Florian Fainelli Hi Greg KH: 在 2020/9/27 21:45, Greg KH 写道: > On Sun, Sep 27, 2020 at 09:42:18PM +0800, Tang Bin wrote: >> In this function, we don't need dev_err() message because >> when something goes wrong, devm_platform_ioremap_resource() >> can print an error message itself, so remove the redundant >> one. >> >> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> >> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> >> --- >> drivers/usb/gadget/udc/bdc/bdc_core.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c b/drivers/usb/gadget/udc/bdc/bdc_core.c >> index 02a3a7746..9454f179e 100644 >> --- a/drivers/usb/gadget/udc/bdc/bdc_core.c >> +++ b/drivers/usb/gadget/udc/bdc/bdc_core.c >> @@ -508,10 +508,8 @@ static int bdc_probe(struct platform_device *pdev) >> bdc->clk = clk; >> >> bdc->regs = devm_platform_ioremap_resource(pdev, 0); >> - if (IS_ERR(bdc->regs)) { >> - dev_err(dev, "ioremap error\n"); >> + if (IS_ERR(bdc->regs)) >> return -ENOMEM; > Why not return the error given to us? Because when get ioremap failed, devm_platform_ioremap_resource() can print error message "dev_err(dev, "ioremap failed for resource %pR\n", res)" in it's called function. So I think this's place's dev_err(dev, "ioremap error\n") is redundant. Thanks Tang Bin > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: bdc: Remove duplicate error message in bdc_probe() 2020-09-28 8:45 ` Tang Bin @ 2020-09-28 9:40 ` Felipe Balbi 2020-09-28 10:55 ` Tang Bin 0 siblings, 1 reply; 9+ messages in thread From: Felipe Balbi @ 2020-09-28 9:40 UTC (permalink / raw) To: Tang Bin, Greg KH; +Cc: linux-usb, linux-kernel, Florian Fainelli [-- Attachment #1: Type: text/plain, Size: 1754 bytes --] Hi, Tang Bin <tangbin@cmss.chinamobile.com> writes: > Hi Greg KH: > > 在 2020/9/27 21:45, Greg KH 写道: >> On Sun, Sep 27, 2020 at 09:42:18PM +0800, Tang Bin wrote: >>> In this function, we don't need dev_err() message because >>> when something goes wrong, devm_platform_ioremap_resource() >>> can print an error message itself, so remove the redundant >>> one. >>> >>> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> >>> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> >>> --- >>> drivers/usb/gadget/udc/bdc/bdc_core.c | 4 +--- >>> 1 file changed, 1 insertion(+), 3 deletions(-) >>> >>> diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c b/drivers/usb/gadget/udc/bdc/bdc_core.c >>> index 02a3a7746..9454f179e 100644 >>> --- a/drivers/usb/gadget/udc/bdc/bdc_core.c >>> +++ b/drivers/usb/gadget/udc/bdc/bdc_core.c >>> @@ -508,10 +508,8 @@ static int bdc_probe(struct platform_device *pdev) >>> bdc->clk = clk; >>> >>> bdc->regs = devm_platform_ioremap_resource(pdev, 0); >>> - if (IS_ERR(bdc->regs)) { >>> - dev_err(dev, "ioremap error\n"); >>> + if (IS_ERR(bdc->regs)) >>> return -ENOMEM; >> Why not return the error given to us? > > Because when get ioremap failed, devm_platform_ioremap_resource() can > print error message > > "dev_err(dev, "ioremap failed for resource %pR\n", res)" in it's called > function. So I think this's place's > > dev_err(dev, "ioremap error\n") is redundant. that's not what Greg point you at, though. Greg's concern is valid in that instead of passing along the error within bdc->regs, you always return -ENOMEM. OTW, your code should read like so: if (IS_ERR(bdc->regs)) return PTR_ERR(bdc->regs); -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 857 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: bdc: Remove duplicate error message in bdc_probe() 2020-09-28 9:40 ` Felipe Balbi @ 2020-09-28 10:55 ` Tang Bin 2020-09-28 11:23 ` Greg KH 0 siblings, 1 reply; 9+ messages in thread From: Tang Bin @ 2020-09-28 10:55 UTC (permalink / raw) To: Felipe Balbi, Greg KH; +Cc: linux-usb, linux-kernel, Florian Fainelli Hi Balbi： 在 2020/9/28 17:40, Felipe Balbi 写道: > Hi, > > Tang Bin <tangbin@cmss.chinamobile.com> writes: >> Hi Greg KH: >> >> 在 2020/9/27 21:45, Greg KH 写道: >>> On Sun, Sep 27, 2020 at 09:42:18PM +0800, Tang Bin wrote: >>>> In this function, we don't need dev_err() message because >>>> when something goes wrong, devm_platform_ioremap_resource() >>>> can print an error message itself, so remove the redundant >>>> one. >>>> >>>> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> >>>> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> >>>> --- >>>> drivers/usb/gadget/udc/bdc/bdc_core.c | 4 +--- >>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c b/drivers/usb/gadget/udc/bdc/bdc_core.c >>>> index 02a3a7746..9454f179e 100644 >>>> --- a/drivers/usb/gadget/udc/bdc/bdc_core.c >>>> +++ b/drivers/usb/gadget/udc/bdc/bdc_core.c >>>> @@ -508,10 +508,8 @@ static int bdc_probe(struct platform_device *pdev) >>>> bdc->clk = clk; >>>> >>>> bdc->regs = devm_platform_ioremap_resource(pdev, 0); >>>> - if (IS_ERR(bdc->regs)) { >>>> - dev_err(dev, "ioremap error\n"); >>>> + if (IS_ERR(bdc->regs)) >>>> return -ENOMEM; >>> Why not return the error given to us? >> Because when get ioremap failed, devm_platform_ioremap_resource() can >> print error message >> >> "dev_err(dev, "ioremap failed for resource %pR\n", res)" in it's called >> function. So I think this's place's >> >> dev_err(dev, "ioremap error\n") is redundant. > that's not what Greg point you at, though. Greg's concern is valid in > that instead of passing along the error within bdc->regs, you always > return -ENOMEM. OTW, your code should read like so: > > if (IS_ERR(bdc->regs)) > return PTR_ERR(bdc->regs); Thanks for your explain，when I send the patch yesterday, my point is at dev_err(), and not aimed at IS_ERR() & PTR_ERR(), if it's Greg's point, I will change this patch after his reply. Thank you very much. Tang Bin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: bdc: Remove duplicate error message in bdc_probe() 2020-09-28 10:55 ` Tang Bin @ 2020-09-28 11:23 ` Greg KH 2020-09-28 11:31 ` Tang Bin 0 siblings, 1 reply; 9+ messages in thread From: Greg KH @ 2020-09-28 11:23 UTC (permalink / raw) To: Tang Bin; +Cc: Felipe Balbi, linux-usb, linux-kernel, Florian Fainelli On Mon, Sep 28, 2020 at 06:55:26PM +0800, Tang Bin wrote: > Hi Balbi： > > 在 2020/9/28 17:40, Felipe Balbi 写道: > > Hi, > > > > Tang Bin <tangbin@cmss.chinamobile.com> writes: > > > Hi Greg KH: > > > > > > 在 2020/9/27 21:45, Greg KH 写道: > > > > On Sun, Sep 27, 2020 at 09:42:18PM +0800, Tang Bin wrote: > > > > > In this function, we don't need dev_err() message because > > > > > when something goes wrong, devm_platform_ioremap_resource() > > > > > can print an error message itself, so remove the redundant > > > > > one. > > > > > > > > > > Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> > > > > > Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> > > > > > --- > > > > > drivers/usb/gadget/udc/bdc/bdc_core.c | 4 +--- > > > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c b/drivers/usb/gadget/udc/bdc/bdc_core.c > > > > > index 02a3a7746..9454f179e 100644 > > > > > --- a/drivers/usb/gadget/udc/bdc/bdc_core.c > > > > > +++ b/drivers/usb/gadget/udc/bdc/bdc_core.c > > > > > @@ -508,10 +508,8 @@ static int bdc_probe(struct platform_device *pdev) > > > > > bdc->clk = clk; > > > > > bdc->regs = devm_platform_ioremap_resource(pdev, 0); > > > > > - if (IS_ERR(bdc->regs)) { > > > > > - dev_err(dev, "ioremap error\n"); > > > > > + if (IS_ERR(bdc->regs)) > > > > > return -ENOMEM; > > > > Why not return the error given to us? > > > Because when get ioremap failed, devm_platform_ioremap_resource() can > > > print error message > > > > > > "dev_err(dev, "ioremap failed for resource %pR\n", res)" in it's called > > > function. So I think this's place's > > > > > > dev_err(dev, "ioremap error\n") is redundant. > > that's not what Greg point you at, though. Greg's concern is valid in > > that instead of passing along the error within bdc->regs, you always > > return -ENOMEM. OTW, your code should read like so: > > > > if (IS_ERR(bdc->regs)) > > return PTR_ERR(bdc->regs); > > Thanks for your explain，when I send the patch yesterday, my point is at > dev_err(), and not aimed at IS_ERR() & PTR_ERR(), > > if it's Greg's point, I will change this patch after his reply. Felipe is correct, and also, you should listen to him over me as he is the maintainer of this part of the kernel :) greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: bdc: Remove duplicate error message in bdc_probe() 2020-09-28 11:23 ` Greg KH @ 2020-09-28 11:31 ` Tang Bin 0 siblings, 0 replies; 9+ messages in thread From: Tang Bin @ 2020-09-28 11:31 UTC (permalink / raw) To: Greg KH, Felipe Balbi; +Cc: linux-usb, linux-kernel, Florian Fainelli Hi 在 2020/9/28 19:23, Greg KH 写道: > On Mon, Sep 28, 2020 at 06:55:26PM +0800, Tang Bin wrote: >> Hi Balbi： >> >> 在 2020/9/28 17:40, Felipe Balbi 写道: >>> Hi, >>> >>> Tang Bin <tangbin@cmss.chinamobile.com> writes: >>>> Hi Greg KH: >>>> >>>> 在 2020/9/27 21:45, Greg KH 写道: >>>>> On Sun, Sep 27, 2020 at 09:42:18PM +0800, Tang Bin wrote: >>>>>> In this function, we don't need dev_err() message because >>>>>> when something goes wrong, devm_platform_ioremap_resource() >>>>>> can print an error message itself, so remove the redundant >>>>>> one. >>>>>> >>>>>> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> >>>>>> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> >>>>>> --- >>>>>> drivers/usb/gadget/udc/bdc/bdc_core.c | 4 +--- >>>>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c b/drivers/usb/gadget/udc/bdc/bdc_core.c >>>>>> index 02a3a7746..9454f179e 100644 >>>>>> --- a/drivers/usb/gadget/udc/bdc/bdc_core.c >>>>>> +++ b/drivers/usb/gadget/udc/bdc/bdc_core.c >>>>>> @@ -508,10 +508,8 @@ static int bdc_probe(struct platform_device *pdev) >>>>>> bdc->clk = clk; >>>>>> bdc->regs = devm_platform_ioremap_resource(pdev, 0); >>>>>> - if (IS_ERR(bdc->regs)) { >>>>>> - dev_err(dev, "ioremap error\n"); >>>>>> + if (IS_ERR(bdc->regs)) >>>>>> return -ENOMEM; >>>>> Why not return the error given to us? >>>> Because when get ioremap failed, devm_platform_ioremap_resource() can >>>> print error message >>>> >>>> "dev_err(dev, "ioremap failed for resource %pR\n", res)" in it's called >>>> function. So I think this's place's >>>> >>>> dev_err(dev, "ioremap error\n") is redundant. >>> that's not what Greg point you at, though. Greg's concern is valid in >>> that instead of passing along the error within bdc->regs, you always >>> return -ENOMEM. OTW, your code should read like so: >>> >>> if (IS_ERR(bdc->regs)) >>> return PTR_ERR(bdc->regs); >> Thanks for your explain，when I send the patch yesterday, my point is at >> dev_err(), and not aimed at IS_ERR() & PTR_ERR(), >> >> if it's Greg's point, I will change this patch after his reply. > Felipe is correct, and also, you should listen to him over me as he is > the maintainer of this part of the kernel :) > Thanks for your reply, I will send v2 for all of you. For me, all of you are the teachers, I am glad to learn from you. I respect every teacher, I think Felipe should be able to feel. Because the premise of the patch, Felipe is also teaching me the question. So I feel I am lucky. Thanks for all of you. Thanks Tang Bin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: bdc: Remove duplicate error message in bdc_probe() 2020-09-27 13:42 [PATCH] usb: bdc: Remove duplicate error message in bdc_probe() Tang Bin 2020-09-27 13:45 ` Greg KH @ 2020-09-28 6:49 ` Chunfeng Yun 2020-09-28 8:40 ` Tang Bin 1 sibling, 1 reply; 9+ messages in thread From: Chunfeng Yun @ 2020-09-28 6:49 UTC (permalink / raw) To: Tang Bin, Florian Fainelli Cc: balbi, gregkh, linux-usb, linux-kernel, Zhang Shengju On Sun, 2020-09-27 at 21:42 +0800, Tang Bin wrote: > In this function, we don't need dev_err() message because > when something goes wrong, devm_platform_ioremap_resource() > can print an error message itself, so remove the redundant > one. > > Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> > Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> > --- > drivers/usb/gadget/udc/bdc/bdc_core.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c b/drivers/usb/gadget/udc/bdc/bdc_core.c > index 02a3a7746..9454f179e 100644 > --- a/drivers/usb/gadget/udc/bdc/bdc_core.c > +++ b/drivers/usb/gadget/udc/bdc/bdc_core.c > @@ -508,10 +508,8 @@ static int bdc_probe(struct platform_device *pdev) > bdc->clk = clk; > > bdc->regs = devm_platform_ioremap_resource(pdev, 0); > - if (IS_ERR(bdc->regs)) { > - dev_err(dev, "ioremap error\n"); > + if (IS_ERR(bdc->regs)) > return -ENOMEM; > - } > irq = platform_get_irq(pdev, 0); > if (irq < 0) > return irq; Cc bdc's maintainer: Florian Fainelli <f.fainelli@gmail.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: bdc: Remove duplicate error message in bdc_probe() 2020-09-28 6:49 ` Chunfeng Yun @ 2020-09-28 8:40 ` Tang Bin 0 siblings, 0 replies; 9+ messages in thread From: Tang Bin @ 2020-09-28 8:40 UTC (permalink / raw) To: Chunfeng Yun, Florian Fainelli; +Cc: balbi, gregkh, linux-usb, linux-kernel 在 2020/9/28 14:49, Chunfeng Yun 写道: > On Sun, 2020-09-27 at 21:42 +0800, Tang Bin wrote: >> In this function, we don't need dev_err() message because >> when something goes wrong, devm_platform_ioremap_resource() >> can print an error message itself, so remove the redundant >> one. >> >> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> >> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> >> --- >> drivers/usb/gadget/udc/bdc/bdc_core.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c b/drivers/usb/gadget/udc/bdc/bdc_core.c >> index 02a3a7746..9454f179e 100644 >> --- a/drivers/usb/gadget/udc/bdc/bdc_core.c >> +++ b/drivers/usb/gadget/udc/bdc/bdc_core.c >> @@ -508,10 +508,8 @@ static int bdc_probe(struct platform_device *pdev) >> bdc->clk = clk; >> >> bdc->regs = devm_platform_ioremap_resource(pdev, 0); >> - if (IS_ERR(bdc->regs)) { >> - dev_err(dev, "ioremap error\n"); >> + if (IS_ERR(bdc->regs)) >> return -ENOMEM; >> - } >> irq = platform_get_irq(pdev, 0); >> if (irq < 0) >> return irq; > Cc bdc's maintainer: Florian Fainelli <f.fainelli@gmail.com> Got it, thanks. Tang Bin > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-09-28 11:31 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-09-27 13:42 [PATCH] usb: bdc: Remove duplicate error message in bdc_probe() Tang Bin 2020-09-27 13:45 ` Greg KH 2020-09-28 8:45 ` Tang Bin 2020-09-28 9:40 ` Felipe Balbi 2020-09-28 10:55 ` Tang Bin 2020-09-28 11:23 ` Greg KH 2020-09-28 11:31 ` Tang Bin 2020-09-28 6:49 ` Chunfeng Yun 2020-09-28 8:40 ` Tang Bin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).