[v2] usb: cdns3: Rids of duplicate error message

Message ID 20201012064256.8344-1-pawell@cadence.com
State New
Headers show
Series
  • [v2] usb: cdns3: Rids of duplicate error message
Related show

Commit Message

Pawel Laszczak Oct. 12, 2020, 6:42 a.m.
On failure, the platform_get_irq_byname prints an error message
so, patch removes error message related to this function from
core.c file.

A change was suggested during reviewing CDNSP driver by Chunfeng Yun.

Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
Changelog:
v2
- simplified code as sugested by Roger Quadros.

 drivers/usb/cdns3/core.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

Comments

Roger Quadros Oct. 12, 2020, 8:16 a.m. | #1
Hi Pawel,

On 12/10/2020 09:42, Pawel Laszczak wrote:
> On failure, the platform_get_irq_byname prints an error message
> so, patch removes error message related to this function from
> core.c file.
> 
> A change was suggested during reviewing CDNSP driver by Chunfeng Yun.
> 
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
> Changelog:
> v2
> - simplified code as sugested by Roger Quadros.
> 
>   drivers/usb/cdns3/core.c | 10 +---------
>   1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index a0f73d4711ae..85ef3025b293 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -469,22 +469,14 @@ static int cdns3_probe(struct platform_device *pdev)
>   	if (cdns->dev_irq == -EPROBE_DEFER)

Shouldn't this be
	if (cdns->dev_irq < 0)
?

>   		return cdns->dev_irq;
>   
> -	if (cdns->dev_irq < 0)
> -		dev_err(dev, "couldn't get peripheral irq\n");
> -
>   	regs = devm_platform_ioremap_resource_byname(pdev, "dev");
>   	if (IS_ERR(regs))
>   		return PTR_ERR(regs);
>   	cdns->dev_regs	= regs;
>   
>   	cdns->otg_irq = platform_get_irq_byname(pdev, "otg");
> -	if (cdns->otg_irq == -EPROBE_DEFER)
> -		return cdns->otg_irq;
> -
> -	if (cdns->otg_irq < 0) {
> -		dev_err(dev, "couldn't get otg irq\n");
> +	if (cdns->otg_irq < 0)
>   		return cdns->otg_irq;
> -	}
>   
>   	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "otg");
>   	if (!res) {
>
Pawel Laszczak Oct. 12, 2020, 11:12 a.m. | #2
Hi Roger,

On 12/10/2020 09:42, Pawel Laszczak wrote:
> On failure, the platform_get_irq_byname prints an error message

> so, patch removes error message related to this function from

> core.c file.

> 

> A change was suggested during reviewing CDNSP driver by Chunfeng Yun.

> 

> Signed-off-by: Pawel Laszczak <pawell@cadence.com>

> ---

> Changelog:

> v2

> - simplified code as sugested by Roger Quadros.

> 

>   drivers/usb/cdns3/core.c | 10 +---------

>   1 file changed, 1 insertion(+), 9 deletions(-)

> 

> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c

> index a0f73d4711ae..85ef3025b293 100644

> --- a/drivers/usb/cdns3/core.c

> +++ b/drivers/usb/cdns3/core.c

> @@ -469,22 +469,14 @@ static int cdns3_probe(struct platform_device *pdev)

>   	if (cdns->dev_irq == -EPROBE_DEFER)


Shouldn't this be
	if (cdns->dev_irq < 0)
?

No, such line will change the original behavior of driver.

Current patch allows to run driver when we support only host. 
In such case the dev_irq can be < 0 and we still can use host  side.

Regards,
Pawel Laszczak

>   		return cdns->dev_irq;

>   

> -	if (cdns->dev_irq < 0)

> -		dev_err(dev, "couldn't get peripheral irq\n");

> -

>   	regs = devm_platform_ioremap_resource_byname(pdev, "dev");

>   	if (IS_ERR(regs))

>   		return PTR_ERR(regs);

>   	cdns->dev_regs	= regs;

>   

>   	cdns->otg_irq = platform_get_irq_byname(pdev, "otg");

> -	if (cdns->otg_irq == -EPROBE_DEFER)

> -		return cdns->otg_irq;

> -

> -	if (cdns->otg_irq < 0) {

> -		dev_err(dev, "couldn't get otg irq\n");

> +	if (cdns->otg_irq < 0)

>   		return cdns->otg_irq;

> -	}

>   

>   	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "otg");

>   	if (!res) {

>
Roger Quadros Oct. 12, 2020, 11:38 a.m. | #3
Pawel,

On 12/10/2020 14:12, Pawel Laszczak wrote:
> Hi Roger,
> 
> On 12/10/2020 09:42, Pawel Laszczak wrote:
>> On failure, the platform_get_irq_byname prints an error message
>> so, patch removes error message related to this function from
>> core.c file.
>>
>> A change was suggested during reviewing CDNSP driver by Chunfeng Yun.
>>
>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> ---
>> Changelog:
>> v2
>> - simplified code as sugested by Roger Quadros.
>>
>>    drivers/usb/cdns3/core.c | 10 +---------
>>    1 file changed, 1 insertion(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>> index a0f73d4711ae..85ef3025b293 100644
>> --- a/drivers/usb/cdns3/core.c
>> +++ b/drivers/usb/cdns3/core.c
>> @@ -469,22 +469,14 @@ static int cdns3_probe(struct platform_device *pdev)
>>    	if (cdns->dev_irq == -EPROBE_DEFER)
> 
> Shouldn't this be
> 	if (cdns->dev_irq < 0)
> ?
> 
> No, such line will change the original behavior of driver.
> 
> Current patch allows to run driver when we support only host.
> In such case the dev_irq can be < 0 and we still can use host  side.

In that case should we check if we need dev_irq and if so then error out.
i.e. if mode is "peripheral" or "otg"

Also DT binding document says all 3 IRQs are mandatory. Do we need to update that?

cheers,
-roger

> 
> Regards,
> Pawel Laszczak
> 
>>    		return cdns->dev_irq;
>>    
>> -	if (cdns->dev_irq < 0)
>> -		dev_err(dev, "couldn't get peripheral irq\n");
>> -
>>    	regs = devm_platform_ioremap_resource_byname(pdev, "dev");
>>    	if (IS_ERR(regs))
>>    		return PTR_ERR(regs);
>>    	cdns->dev_regs	= regs;
>>    
>>    	cdns->otg_irq = platform_get_irq_byname(pdev, "otg");
>> -	if (cdns->otg_irq == -EPROBE_DEFER)
>> -		return cdns->otg_irq;
>> -
>> -	if (cdns->otg_irq < 0) {
>> -		dev_err(dev, "couldn't get otg irq\n");
>> +	if (cdns->otg_irq < 0)
>>    		return cdns->otg_irq;
>> -	}
>>    
>>    	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "otg");
>>    	if (!res) {
>>
>
Peter Chen Oct. 12, 2020, 12:18 p.m. | #4
On 20-10-12 14:38:16, Roger Quadros wrote:
> Pawel,

> 

> On 12/10/2020 14:12, Pawel Laszczak wrote:

> > Hi Roger,

> > 

> > On 12/10/2020 09:42, Pawel Laszczak wrote:

> > > On failure, the platform_get_irq_byname prints an error message

> > > so, patch removes error message related to this function from

> > > core.c file.

> > > 

> > > A change was suggested during reviewing CDNSP driver by Chunfeng Yun.

> > > 

> > > Signed-off-by: Pawel Laszczak <pawell@cadence.com>

> > > ---

> > > Changelog:

> > > v2

> > > - simplified code as sugested by Roger Quadros.

> > > 

> > >    drivers/usb/cdns3/core.c | 10 +---------

> > >    1 file changed, 1 insertion(+), 9 deletions(-)

> > > 

> > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c

> > > index a0f73d4711ae..85ef3025b293 100644

> > > --- a/drivers/usb/cdns3/core.c

> > > +++ b/drivers/usb/cdns3/core.c

> > > @@ -469,22 +469,14 @@ static int cdns3_probe(struct platform_device *pdev)

> > >    	if (cdns->dev_irq == -EPROBE_DEFER)

> > 

> > Shouldn't this be

> > 	if (cdns->dev_irq < 0)

> > ?

> > 

> > No, such line will change the original behavior of driver.

> > 

> > Current patch allows to run driver when we support only host.

> > In such case the dev_irq can be < 0 and we still can use host  side.

> 

> In that case should we check if we need dev_irq and if so then error out.

> i.e. if mode is "peripheral" or "otg"

> 

> Also DT binding document says all 3 IRQs are mandatory. Do we need to update that?

> 


I agree with Roger, or you change the driver behaviour that return error
if the dev_irq is < 0.

Peter
> 

> > 

> > Regards,

> > Pawel Laszczak

> > 

> > >    		return cdns->dev_irq;

> > > -	if (cdns->dev_irq < 0)

> > > -		dev_err(dev, "couldn't get peripheral irq\n");

> > > -

> > >    	regs = devm_platform_ioremap_resource_byname(pdev, "dev");

> > >    	if (IS_ERR(regs))

> > >    		return PTR_ERR(regs);

> > >    	cdns->dev_regs	= regs;

> > >    	cdns->otg_irq = platform_get_irq_byname(pdev, "otg");

> > > -	if (cdns->otg_irq == -EPROBE_DEFER)

> > > -		return cdns->otg_irq;

> > > -

> > > -	if (cdns->otg_irq < 0) {

> > > -		dev_err(dev, "couldn't get otg irq\n");

> > > +	if (cdns->otg_irq < 0)

> > >    		return cdns->otg_irq;

> > > -	}

> > >    	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "otg");

> > >    	if (!res) {

> > > 

> > 

> 

> -- 

> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.

> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki


-- 

Thanks,
Peter Chen
Pawel Laszczak Oct. 12, 2020, 12:26 p.m. | #5
>
>On 20-10-12 14:38:16, Roger Quadros wrote:
>> Pawel,
>>
>> On 12/10/2020 14:12, Pawel Laszczak wrote:
>> > Hi Roger,
>> >
>> > On 12/10/2020 09:42, Pawel Laszczak wrote:
>> > > On failure, the platform_get_irq_byname prints an error message
>> > > so, patch removes error message related to this function from
>> > > core.c file.
>> > >
>> > > A change was suggested during reviewing CDNSP driver by Chunfeng Yun.
>> > >
>> > > Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> > > ---
>> > > Changelog:
>> > > v2
>> > > - simplified code as sugested by Roger Quadros.
>> > >
>> > >    drivers/usb/cdns3/core.c | 10 +---------
>> > >    1 file changed, 1 insertion(+), 9 deletions(-)
>> > >
>> > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>> > > index a0f73d4711ae..85ef3025b293 100644
>> > > --- a/drivers/usb/cdns3/core.c
>> > > +++ b/drivers/usb/cdns3/core.c
>> > > @@ -469,22 +469,14 @@ static int cdns3_probe(struct platform_device *pdev)
>> > >    	if (cdns->dev_irq == -EPROBE_DEFER)
>> >
>> > Shouldn't this be
>> > 	if (cdns->dev_irq < 0)
>> > ?
>> >
>> > No, such line will change the original behavior of driver.
>> >
>> > Current patch allows to run driver when we support only host.
>> > In such case the dev_irq can be < 0 and we still can use host  side.
>>
>> In that case should we check if we need dev_irq and if so then error out.
>> i.e. if mode is "peripheral" or "otg"
>>
>> Also DT binding document says all 3 IRQs are mandatory. Do we need to update that?
>>
>
>I agree with Roger, or you change the driver behaviour that return error
>if the dev_irq is < 0.

Ok, I will  change the  condition as suggested by Roger. 

Pawel
>
>Peter
>>
>> >
>> > Regards,
>> > Pawel Laszczak
>> >
>> > >    		return cdns->dev_irq;
>> > > -	if (cdns->dev_irq < 0)
>> > > -		dev_err(dev, "couldn't get peripheral irq\n");
>> > > -
>> > >    	regs = devm_platform_ioremap_resource_byname(pdev, "dev");
>> > >    	if (IS_ERR(regs))
>> > >    		return PTR_ERR(regs);
>> > >    	cdns->dev_regs	= regs;
>> > >    	cdns->otg_irq = platform_get_irq_byname(pdev, "otg");
>> > > -	if (cdns->otg_irq == -EPROBE_DEFER)
>> > > -		return cdns->otg_irq;
>> > > -
>> > > -	if (cdns->otg_irq < 0) {
>> > > -		dev_err(dev, "couldn't get otg irq\n");
>> > > +	if (cdns->otg_irq < 0)
>> > >    		return cdns->otg_irq;
>> > > -	}
>> > >    	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "otg");
>> > >    	if (!res) {
>> > >
>> >
>>
>> --
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>
>--
>
>Thanks,
>Peter Chen

Patch

diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index a0f73d4711ae..85ef3025b293 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -469,22 +469,14 @@  static int cdns3_probe(struct platform_device *pdev)
 	if (cdns->dev_irq == -EPROBE_DEFER)
 		return cdns->dev_irq;
 
-	if (cdns->dev_irq < 0)
-		dev_err(dev, "couldn't get peripheral irq\n");
-
 	regs = devm_platform_ioremap_resource_byname(pdev, "dev");
 	if (IS_ERR(regs))
 		return PTR_ERR(regs);
 	cdns->dev_regs	= regs;
 
 	cdns->otg_irq = platform_get_irq_byname(pdev, "otg");
-	if (cdns->otg_irq == -EPROBE_DEFER)
-		return cdns->otg_irq;
-
-	if (cdns->otg_irq < 0) {
-		dev_err(dev, "couldn't get otg irq\n");
+	if (cdns->otg_irq < 0)
 		return cdns->otg_irq;
-	}
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "otg");
 	if (!res) {