| Message ID | 20200927134218.20028-1-tangbin@cmss.chinamobile.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: bdc: Remove duplicate error message in bdc_probe() | expand |
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
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>
在 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 > >
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 >
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
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
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
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
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;