usb: cdns3: platform_get_irq_byname_optional instead platform_get_irq_byname

Message ID 20200930065758.23740-1-pawell@cadence.com
State New
Headers show
Series
  • usb: cdns3: platform_get_irq_byname_optional instead platform_get_irq_byname
Related show

Commit Message

Pawel Laszczak Sept. 30, 2020, 6:57 a.m.
To avoid duplicate error information patch replaces platform_get_irq_byname
into platform_get_irq_byname_optional.

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

Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
 drivers/usb/cdns3/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Pawel Laszczak Sept. 30, 2020, 7:24 a.m. | #1
Hi,

My email server complains that delivery to peter.chan@nxp.org has failed. 

This address has generated by  get_maintainer.pl.  
I only updated email to correct:  peter.chan@nxp.com.

Regards,
Pawel

>To avoid duplicate error information patch replaces platform_get_irq_byname
>into platform_get_irq_byname_optional.
>
>A change was suggested during reviewing CDNSP driver by Chunfeng Yun.
>
>Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>---
> drivers/usb/cdns3/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>index a0f73d4711ae..a3f6dc44cf3a 100644
>--- a/drivers/usb/cdns3/core.c
>+++ b/drivers/usb/cdns3/core.c
>@@ -465,7 +465,7 @@ static int cdns3_probe(struct platform_device *pdev)
>
> 	cdns->xhci_res[1] = *res;
>
>-	cdns->dev_irq = platform_get_irq_byname(pdev, "peripheral");
>+	cdns->dev_irq = platform_get_irq_byname_optional(pdev, "peripheral");
> 	if (cdns->dev_irq == -EPROBE_DEFER)
> 		return cdns->dev_irq;
>
>@@ -477,7 +477,7 @@ static int cdns3_probe(struct platform_device *pdev)
> 		return PTR_ERR(regs);
> 	cdns->dev_regs	= regs;
>
>-	cdns->otg_irq = platform_get_irq_byname(pdev, "otg");
>+	cdns->otg_irq = platform_get_irq_byname_optional(pdev, "otg");
> 	if (cdns->otg_irq == -EPROBE_DEFER)
> 		return cdns->otg_irq;
>
>--
>2.17.1
Roger Quadros Oct. 2, 2020, 8:53 a.m. | #2
Pawel,

On 30/09/2020 09:57, Pawel Laszczak wrote:
> To avoid duplicate error information patch replaces platform_get_irq_byname
> into platform_get_irq_byname_optional.

What is duplicate error information?

> 
> A change was suggested during reviewing CDNSP driver by Chunfeng Yun.
> 
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
>   drivers/usb/cdns3/core.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index a0f73d4711ae..a3f6dc44cf3a 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -465,7 +465,7 @@ static int cdns3_probe(struct platform_device *pdev)
>   
>   	cdns->xhci_res[1] = *res;
>   
> -	cdns->dev_irq = platform_get_irq_byname(pdev, "peripheral");
> +	cdns->dev_irq = platform_get_irq_byname_optional(pdev, "peripheral");

As per DT binding document, these are mandatory properties

  - interrupts: Interrupts used by cdns3 controller:
         "host" - interrupt used by XHCI driver.
         "peripheral" - interrupt used by device driver
         "otg" - interrupt used by DRD/OTG  part of driver

for dr_mode == "otg" -> all 3 are mandatory.
for dr_mode == "host" -> "otg" and "peripheral" IRQs are not required.
for dr_mode == "periphearal" -> "otg" and "host" IRQs are not required.

>   	if (cdns->dev_irq == -EPROBE_DEFER)
>   		return cdns->dev_irq;
>   
> @@ -477,7 +477,7 @@ static int cdns3_probe(struct platform_device *pdev)
>   		return PTR_ERR(regs);
>   	cdns->dev_regs	= regs;
>   
> -	cdns->otg_irq = platform_get_irq_byname(pdev, "otg");
> +	cdns->otg_irq = platform_get_irq_byname_optional(pdev, "otg");
>   	if (cdns->otg_irq == -EPROBE_DEFER)
>   		return cdns->otg_irq;
>   
> 

cheers,
-roger
Pawel Laszczak Oct. 2, 2020, 9:08 a.m. | #3
Roger,

>

>On 30/09/2020 09:57, Pawel Laszczak wrote:

>> To avoid duplicate error information patch replaces platform_get_irq_byname

>> into platform_get_irq_byname_optional.

>

>What is duplicate error information?


The function platform_get_irq_byname print:
" dev_err(&dev->dev, "IRQ %s not found\n", name);" if error occurred. 

In core.c we have the another error message below invoking this function.
e.g 
	if (cdns->dev_irq < 0)
		dev_err(dev, "couldn't get peripheral irq\n");

So, it's looks like one dev_err is  redundant.

>

>>

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

>>

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

>> ---

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

>>   1 file changed, 2 insertions(+), 2 deletions(-)

>>

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

>> index a0f73d4711ae..a3f6dc44cf3a 100644

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

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

>> @@ -465,7 +465,7 @@ static int cdns3_probe(struct platform_device *pdev)

>>

>>   	cdns->xhci_res[1] = *res;

>>

>> -	cdns->dev_irq = platform_get_irq_byname(pdev, "peripheral");

>> +	cdns->dev_irq = platform_get_irq_byname_optional(pdev, "peripheral");

>

>As per DT binding document, these are mandatory properties


I think that name platform_get_irq_byname_optional is little confusing. 
Function descriptions show that both are almost identical:
/**
 * platform_get_irq_byname_optional - get an optional IRQ for a device by name
 * @dev: platform device
 * @name: IRQ name
 *
 * Get an optional IRQ by name like platform_get_irq_byname(). Except that it
 * does not print an error message if an IRQ can not be obtained.
 *
 * Return: non-zero IRQ number on success, negative error number on failure.
 */

>

>  - interrupts: Interrupts used by cdns3 controller:

>         "host" - interrupt used by XHCI driver.

>         "peripheral" - interrupt used by device driver

>         "otg" - interrupt used by DRD/OTG  part of driver

>

>for dr_mode == "otg" -> all 3 are mandatory.

>for dr_mode == "host" -> "otg" and "peripheral" IRQs are not required.

>for dr_mode == "periphearal" -> "otg" and "host" IRQs are not required.

>

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

>>   		return cdns->dev_irq;

>>

>> @@ -477,7 +477,7 @@ static int cdns3_probe(struct platform_device *pdev)

>>   		return PTR_ERR(regs);

>>   	cdns->dev_regs	= regs;

>>

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

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

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

>>   		return cdns->otg_irq;

>>

>>

>


Regards,
Pawel
Roger Quadros Oct. 2, 2020, 10:06 a.m. | #4
Pawel,

On 02/10/2020 12:08, Pawel Laszczak wrote:
> Roger,
> 
>>
>> On 30/09/2020 09:57, Pawel Laszczak wrote:
>>> To avoid duplicate error information patch replaces platform_get_irq_byname
>>> into platform_get_irq_byname_optional.
>>
>> What is duplicate error information?
> 
> The function platform_get_irq_byname print:
> " dev_err(&dev->dev, "IRQ %s not found\n", name);" if error occurred.
> 
> In core.c we have the another error message below invoking this function.
> e.g
> 	if (cdns->dev_irq < 0)
> 		dev_err(dev, "couldn't get peripheral irq\n");
> 
> So, it's looks like one dev_err is  redundant.

If we want all 3 IRQs to be valid irrespective of dr_mode then we should
use platform_get_irq_byname() and error out in probe if (ret < 0 && ret != -EPROBE_DEFER).

We can get rid of the irq check and duplicate error message in other places.

cheers,
-roger

> 
>>
>>>
>>> A change was suggested during reviewing CDNSP driver by Chunfeng Yun.
>>>
>>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>>> ---
>>>    drivers/usb/cdns3/core.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>> index a0f73d4711ae..a3f6dc44cf3a 100644
>>> --- a/drivers/usb/cdns3/core.c
>>> +++ b/drivers/usb/cdns3/core.c
>>> @@ -465,7 +465,7 @@ static int cdns3_probe(struct platform_device *pdev)
>>>
>>>    	cdns->xhci_res[1] = *res;
>>>
>>> -	cdns->dev_irq = platform_get_irq_byname(pdev, "peripheral");
>>> +	cdns->dev_irq = platform_get_irq_byname_optional(pdev, "peripheral");
>>
>> As per DT binding document, these are mandatory properties
> 
> I think that name platform_get_irq_byname_optional is little confusing.
> Function descriptions show that both are almost identical:
> /**
>   * platform_get_irq_byname_optional - get an optional IRQ for a device by name
>   * @dev: platform device
>   * @name: IRQ name
>   *
>   * Get an optional IRQ by name like platform_get_irq_byname(). Except that it
>   * does not print an error message if an IRQ can not be obtained.
>   *
>   * Return: non-zero IRQ number on success, negative error number on failure.
>   */
> 
>>
>>   - interrupts: Interrupts used by cdns3 controller:
>>          "host" - interrupt used by XHCI driver.
>>          "peripheral" - interrupt used by device driver
>>          "otg" - interrupt used by DRD/OTG  part of driver
>>
>> for dr_mode == "otg" -> all 3 are mandatory.
>> for dr_mode == "host" -> "otg" and "peripheral" IRQs are not required.
>> for dr_mode == "periphearal" -> "otg" and "host" IRQs are not required.
>>
>>>    	if (cdns->dev_irq == -EPROBE_DEFER)
>>>    		return cdns->dev_irq;
>>>
>>> @@ -477,7 +477,7 @@ static int cdns3_probe(struct platform_device *pdev)
>>>    		return PTR_ERR(regs);
>>>    	cdns->dev_regs	= regs;
>>>
>>> -	cdns->otg_irq = platform_get_irq_byname(pdev, "otg");
>>> +	cdns->otg_irq = platform_get_irq_byname_optional(pdev, "otg");
>>>    	if (cdns->otg_irq == -EPROBE_DEFER)
>>>    		return cdns->otg_irq;
>>>
>>>
>>
> 
> Regards,
> Pawel
>
Pawel Laszczak Oct. 5, 2020, 5:54 a.m. | #5
Roger,
>

>Pawel,

>

>On 02/10/2020 12:08, Pawel Laszczak wrote:

>> Roger,

>>

>>>

>>> On 30/09/2020 09:57, Pawel Laszczak wrote:

>>>> To avoid duplicate error information patch replaces platform_get_irq_byname

>>>> into platform_get_irq_byname_optional.

>>>

>>> What is duplicate error information?

>>

>> The function platform_get_irq_byname print:

>> " dev_err(&dev->dev, "IRQ %s not found\n", name);" if error occurred.

>>

>> In core.c we have the another error message below invoking this function.

>> e.g

>> 	if (cdns->dev_irq < 0)

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

>>

>> So, it's looks like one dev_err is  redundant.

>

>If we want all 3 IRQs to be valid irrespective of dr_mode then we should

>use platform_get_irq_byname() and error out in probe if (ret < 0 && ret != -EPROBE_DEFER).

>

>We can get rid of the irq check and duplicate error message in other places.


To be sure we understand each other correctly.

Are You suggesting  to leave the  platform_get_irq_byname()
and rid of from core.c the following lines :

if (cdns->dev_irq < 0)
	dev_err(dev, "couldn't get peripheral irq\n");
	
and

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

A word of explanation why this patch has been sent.
During reviewing the cdnsp driver Chunfeng Yun add such comment:

"
> +	cdns->dev_irq = platform_get_irq_byname(pdev, "peripheral");

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

> +		return cdns->dev_irq;

> +

> +	if (cdns->dev_irq < 0)

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

Use platform_get_irq_byname_optional? otherwise no need print this log,
platform_get_irq_byname() will print it. 
"

In this patch I've chosen the platform_get_irq_byname_optional because both
function do the same but the error message from core.c tell us little more then
generic message from platform_get_irq_byname.

Cheers
Pawel Laszczak

>

>cheers,

>-roger

>

>>

>>>

>>>>

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

>>>>

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

>>>> ---

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

>>>>    1 file changed, 2 insertions(+), 2 deletions(-)

>>>>

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

>>>> index a0f73d4711ae..a3f6dc44cf3a 100644

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

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

>>>> @@ -465,7 +465,7 @@ static int cdns3_probe(struct platform_device *pdev)

>>>>

>>>>    	cdns->xhci_res[1] = *res;

>>>>

>>>> -	cdns->dev_irq = platform_get_irq_byname(pdev, "peripheral");

>>>> +	cdns->dev_irq = platform_get_irq_byname_optional(pdev, "peripheral");

>>>

>>> As per DT binding document, these are mandatory properties

>>

>> I think that name platform_get_irq_byname_optional is little confusing.

>> Function descriptions show that both are almost identical:

>> /**

>>   * platform_get_irq_byname_optional - get an optional IRQ for a device by name

>>   * @dev: platform device

>>   * @name: IRQ name

>>   *

>>   * Get an optional IRQ by name like platform_get_irq_byname(). Except that it

>>   * does not print an error message if an IRQ can not be obtained.

>>   *

>>   * Return: non-zero IRQ number on success, negative error number on failure.

>>   */

>>

>>>

>>>   - interrupts: Interrupts used by cdns3 controller:

>>>          "host" - interrupt used by XHCI driver.

>>>          "peripheral" - interrupt used by device driver

>>>          "otg" - interrupt used by DRD/OTG  part of driver

>>>

>>> for dr_mode == "otg" -> all 3 are mandatory.

>>> for dr_mode == "host" -> "otg" and "peripheral" IRQs are not required.

>>> for dr_mode == "periphearal" -> "otg" and "host" IRQs are not required.

>>>

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

>>>>    		return cdns->dev_irq;

>>>>

>>>> @@ -477,7 +477,7 @@ static int cdns3_probe(struct platform_device *pdev)

>>>>    		return PTR_ERR(regs);

>>>>    	cdns->dev_regs	= regs;

>>>>

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

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

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

>>>>    		return cdns->otg_irq;

>>>>

>>>>

>>>

>>

>> Regards,

>> Pawel

>>

>

>--

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

>Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Roger Quadros Oct. 5, 2020, 8:43 a.m. | #6
Pawel,

On 05/10/2020 08:54, Pawel Laszczak wrote:
> Roger,
>>
>> Pawel,
>>
>> On 02/10/2020 12:08, Pawel Laszczak wrote:
>>> Roger,
>>>
>>>>
>>>> On 30/09/2020 09:57, Pawel Laszczak wrote:
>>>>> To avoid duplicate error information patch replaces platform_get_irq_byname
>>>>> into platform_get_irq_byname_optional.
>>>>
>>>> What is duplicate error information?
>>>
>>> The function platform_get_irq_byname print:
>>> " dev_err(&dev->dev, "IRQ %s not found\n", name);" if error occurred.
>>>
>>> In core.c we have the another error message below invoking this function.
>>> e.g
>>> 	if (cdns->dev_irq < 0)
>>> 		dev_err(dev, "couldn't get peripheral irq\n");
>>>
>>> So, it's looks like one dev_err is  redundant.
>>
>> If we want all 3 IRQs to be valid irrespective of dr_mode then we should
>> use platform_get_irq_byname() and error out in probe if (ret < 0 && ret != -EPROBE_DEFER).
>>
>> We can get rid of the irq check and duplicate error message in other places.
> 
> To be sure we understand each other correctly.
> 
> Are You suggesting  to leave the  platform_get_irq_byname()
> and rid of from core.c the following lines :
> 
> if (cdns->dev_irq < 0)
> 	dev_err(dev, "couldn't get peripheral irq\n");
> 	
> and
> 
> dev_err(dev, "couldn't get otg irq\n");
> ?

Yes.

> 
> A word of explanation why this patch has been sent.
> During reviewing the cdnsp driver Chunfeng Yun add such comment:
> 
> "
>> +	cdns->dev_irq = platform_get_irq_byname(pdev, "peripheral");
>> +	if (cdns->dev_irq == -EPROBE_DEFER)
>> +		return cdns->dev_irq;
>> +
>> +	if (cdns->dev_irq < 0)
>> +		dev_err(dev, "couldn't get peripheral irq\n");
> Use platform_get_irq_byname_optional? otherwise no need print this log,
> platform_get_irq_byname() will print it.
> "
> 
> In this patch I've chosen the platform_get_irq_byname_optional because both
> function do the same but the error message from core.c tell us little more then
> generic message from platform_get_irq_byname.

Using platform_get_irq_byname_optional() says driver expects it is optional but
only to fail later. It will be confusing to new reader that's all. I leave it to
you to decide what approach to take.

cheers,
-roger

>>
>>>
>>>>
>>>>>
>>>>> A change was suggested during reviewing CDNSP driver by Chunfeng Yun.
>>>>>
>>>>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>>>>> ---
>>>>>     drivers/usb/cdns3/core.c | 4 ++--
>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>>>> index a0f73d4711ae..a3f6dc44cf3a 100644
>>>>> --- a/drivers/usb/cdns3/core.c
>>>>> +++ b/drivers/usb/cdns3/core.c
>>>>> @@ -465,7 +465,7 @@ static int cdns3_probe(struct platform_device *pdev)
>>>>>
>>>>>     	cdns->xhci_res[1] = *res;
>>>>>
>>>>> -	cdns->dev_irq = platform_get_irq_byname(pdev, "peripheral");
>>>>> +	cdns->dev_irq = platform_get_irq_byname_optional(pdev, "peripheral");
>>>>
>>>> As per DT binding document, these are mandatory properties
>>>
>>> I think that name platform_get_irq_byname_optional is little confusing.
>>> Function descriptions show that both are almost identical:
>>> /**
>>>    * platform_get_irq_byname_optional - get an optional IRQ for a device by name
>>>    * @dev: platform device
>>>    * @name: IRQ name
>>>    *
>>>    * Get an optional IRQ by name like platform_get_irq_byname(). Except that it
>>>    * does not print an error message if an IRQ can not be obtained.
>>>    *
>>>    * Return: non-zero IRQ number on success, negative error number on failure.
>>>    */
>>>
>>>>
>>>>    - interrupts: Interrupts used by cdns3 controller:
>>>>           "host" - interrupt used by XHCI driver.
>>>>           "peripheral" - interrupt used by device driver
>>>>           "otg" - interrupt used by DRD/OTG  part of driver
>>>>
>>>> for dr_mode == "otg" -> all 3 are mandatory.
>>>> for dr_mode == "host" -> "otg" and "peripheral" IRQs are not required.
>>>> for dr_mode == "periphearal" -> "otg" and "host" IRQs are not required.
>>>>
>>>>>     	if (cdns->dev_irq == -EPROBE_DEFER)
>>>>>     		return cdns->dev_irq;
>>>>>
>>>>> @@ -477,7 +477,7 @@ static int cdns3_probe(struct platform_device *pdev)
>>>>>     		return PTR_ERR(regs);
>>>>>     	cdns->dev_regs	= regs;
>>>>>
>>>>> -	cdns->otg_irq = platform_get_irq_byname(pdev, "otg");
>>>>> +	cdns->otg_irq = platform_get_irq_byname_optional(pdev, "otg");
>>>>>     	if (cdns->otg_irq == -EPROBE_DEFER)
>>>>>     		return cdns->otg_irq;
>>>>>
>>>>>
>>>>
>>>
>>> Regards,
>>> Pawel
>>>
>>
>> --
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Pawel Laszczak Oct. 5, 2020, 8:49 a.m. | #7
Roger,

>Pawel,

>

>On 05/10/2020 08:54, Pawel Laszczak wrote:

>> Roger,

>>>

>>> Pawel,

>>>

>>> On 02/10/2020 12:08, Pawel Laszczak wrote:

>>>> Roger,

>>>>

>>>>>

>>>>> On 30/09/2020 09:57, Pawel Laszczak wrote:

>>>>>> To avoid duplicate error information patch replaces platform_get_irq_byname

>>>>>> into platform_get_irq_byname_optional.

>>>>>

>>>>> What is duplicate error information?

>>>>

>>>> The function platform_get_irq_byname print:

>>>> " dev_err(&dev->dev, "IRQ %s not found\n", name);" if error occurred.

>>>>

>>>> In core.c we have the another error message below invoking this function.

>>>> e.g

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

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

>>>>

>>>> So, it's looks like one dev_err is  redundant.

>>>

>>> If we want all 3 IRQs to be valid irrespective of dr_mode then we should

>>> use platform_get_irq_byname() and error out in probe if (ret < 0 && ret != -EPROBE_DEFER).

>>>

>>> We can get rid of the irq check and duplicate error message in other places.

>>

>> To be sure we understand each other correctly.

>>

>> Are You suggesting  to leave the  platform_get_irq_byname()

>> and rid of from core.c the following lines :

>>

>> if (cdns->dev_irq < 0)

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

>>

>> and

>>

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

>> ?

>

>Yes.

>

>>

>> A word of explanation why this patch has been sent.

>> During reviewing the cdnsp driver Chunfeng Yun add such comment:

>>

>> "

>>> +	cdns->dev_irq = platform_get_irq_byname(pdev, "peripheral");

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

>>> +		return cdns->dev_irq;

>>> +

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

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

>> Use platform_get_irq_byname_optional? otherwise no need print this log,

>> platform_get_irq_byname() will print it.

>> "

>>

>> In this patch I've chosen the platform_get_irq_byname_optional because both

>> function do the same but the error message from core.c tell us little more then

>> generic message from platform_get_irq_byname.

>

>Using platform_get_irq_byname_optional() says driver expects it is optional but

>only to fail later. It will be confusing to new reader that's all. I leave it to

>you to decide what approach to take.


Thanks for clarification. 
I will send  new patch with platform_get_irq_byname.

Cheers,
Pawel

>

>>>

>>>>

>>>>>

>>>>>>

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

>>>>>>

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

>>>>>> ---

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

>>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)

>>>>>>

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

>>>>>> index a0f73d4711ae..a3f6dc44cf3a 100644

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

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

>>>>>> @@ -465,7 +465,7 @@ static int cdns3_probe(struct platform_device *pdev)

>>>>>>

>>>>>>     	cdns->xhci_res[1] = *res;

>>>>>>

>>>>>> -	cdns->dev_irq = platform_get_irq_byname(pdev, "peripheral");

>>>>>> +	cdns->dev_irq = platform_get_irq_byname_optional(pdev, "peripheral");

>>>>>

>>>>> As per DT binding document, these are mandatory properties

>>>>

>>>> I think that name platform_get_irq_byname_optional is little confusing.

>>>> Function descriptions show that both are almost identical:

>>>> /**

>>>>    * platform_get_irq_byname_optional - get an optional IRQ for a device by name

>>>>    * @dev: platform device

>>>>    * @name: IRQ name

>>>>    *

>>>>    * Get an optional IRQ by name like platform_get_irq_byname(). Except that it

>>>>    * does not print an error message if an IRQ can not be obtained.

>>>>    *

>>>>    * Return: non-zero IRQ number on success, negative error number on failure.

>>>>    */

>>>>

>>>>>

>>>>>    - interrupts: Interrupts used by cdns3 controller:

>>>>>           "host" - interrupt used by XHCI driver.

>>>>>           "peripheral" - interrupt used by device driver

>>>>>           "otg" - interrupt used by DRD/OTG  part of driver

>>>>>

>>>>> for dr_mode == "otg" -> all 3 are mandatory.

>>>>> for dr_mode == "host" -> "otg" and "peripheral" IRQs are not required.

>>>>> for dr_mode == "periphearal" -> "otg" and "host" IRQs are not required.

>>>>>

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

>>>>>>     		return cdns->dev_irq;

>>>>>>

>>>>>> @@ -477,7 +477,7 @@ static int cdns3_probe(struct platform_device *pdev)

>>>>>>     		return PTR_ERR(regs);

>>>>>>     	cdns->dev_regs	= regs;

>>>>>>

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

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

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

>>>>>>     		return cdns->otg_irq;

>>>>>>

>>>>>>

>>>>>

>>>>

>>>> Regards,

>>>> Pawel

>>>>

>>>

>>> --

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

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

>

>--

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

>Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Pawel Laszczak Oct. 7, 2020, 3:12 a.m. | #8
Felipe, 

Please  ignore this patch. I will create the new one. 
It's no sense to send the v2 because I have to change the patch name,
Description and contents.

Regards,
Pawel,

>Roger,

>

>>Pawel,

>>

>>On 05/10/2020 08:54, Pawel Laszczak wrote:

>>> Roger,

>>>>

>>>> Pawel,

>>>>

>>>> On 02/10/2020 12:08, Pawel Laszczak wrote:

>>>>> Roger,

>>>>>

>>>>>>

>>>>>> On 30/09/2020 09:57, Pawel Laszczak wrote:

>>>>>>> To avoid duplicate error information patch replaces platform_get_irq_byname

>>>>>>> into platform_get_irq_byname_optional.

>>>>>>

>>>>>> What is duplicate error information?

>>>>>

>>>>> The function platform_get_irq_byname print:

>>>>> " dev_err(&dev->dev, "IRQ %s not found\n", name);" if error occurred.

>>>>>

>>>>> In core.c we have the another error message below invoking this function.

>>>>> e.g

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

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

>>>>>

>>>>> So, it's looks like one dev_err is  redundant.

>>>>

>>>> If we want all 3 IRQs to be valid irrespective of dr_mode then we should

>>>> use platform_get_irq_byname() and error out in probe if (ret < 0 && ret != -EPROBE_DEFER).

>>>>

>>>> We can get rid of the irq check and duplicate error message in other places.

>>>

>>> To be sure we understand each other correctly.

>>>

>>> Are You suggesting  to leave the  platform_get_irq_byname()

>>> and rid of from core.c the following lines :

>>>

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

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

>>>

>>> and

>>>

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

>>> ?

>>

>>Yes.

>>

>>>

>>> A word of explanation why this patch has been sent.

>>> During reviewing the cdnsp driver Chunfeng Yun add such comment:

>>>

>>> "

>>>> +	cdns->dev_irq = platform_get_irq_byname(pdev, "peripheral");

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

>>>> +		return cdns->dev_irq;

>>>> +

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

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

>>> Use platform_get_irq_byname_optional? otherwise no need print this log,

>>> platform_get_irq_byname() will print it.

>>> "

>>>

>>> In this patch I've chosen the platform_get_irq_byname_optional because both

>>> function do the same but the error message from core.c tell us little more then

>>> generic message from platform_get_irq_byname.

>>

>>Using platform_get_irq_byname_optional() says driver expects it is optional but

>>only to fail later. It will be confusing to new reader that's all. I leave it to

>>you to decide what approach to take.

>

>Thanks for clarification.

>I will send  new patch with platform_get_irq_byname.

>

>Cheers,

>Pawel

>

>>

>>>>

>>>>>

>>>>>>

>>>>>>>

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

>>>>>>>

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

>>>>>>> ---

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

>>>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)

>>>>>>>

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

>>>>>>> index a0f73d4711ae..a3f6dc44cf3a 100644

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

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

>>>>>>> @@ -465,7 +465,7 @@ static int cdns3_probe(struct platform_device *pdev)

>>>>>>>

>>>>>>>     	cdns->xhci_res[1] = *res;

>>>>>>>

>>>>>>> -	cdns->dev_irq = platform_get_irq_byname(pdev, "peripheral");

>>>>>>> +	cdns->dev_irq = platform_get_irq_byname_optional(pdev, "peripheral");

>>>>>>

>>>>>> As per DT binding document, these are mandatory properties

>>>>>

>>>>> I think that name platform_get_irq_byname_optional is little confusing.

>>>>> Function descriptions show that both are almost identical:

>>>>> /**

>>>>>    * platform_get_irq_byname_optional - get an optional IRQ for a device by name

>>>>>    * @dev: platform device

>>>>>    * @name: IRQ name

>>>>>    *

>>>>>    * Get an optional IRQ by name like platform_get_irq_byname(). Except that it

>>>>>    * does not print an error message if an IRQ can not be obtained.

>>>>>    *

>>>>>    * Return: non-zero IRQ number on success, negative error number on failure.

>>>>>    */

>>>>>

>>>>>>

>>>>>>    - interrupts: Interrupts used by cdns3 controller:

>>>>>>           "host" - interrupt used by XHCI driver.

>>>>>>           "peripheral" - interrupt used by device driver

>>>>>>           "otg" - interrupt used by DRD/OTG  part of driver

>>>>>>

>>>>>> for dr_mode == "otg" -> all 3 are mandatory.

>>>>>> for dr_mode == "host" -> "otg" and "peripheral" IRQs are not required.

>>>>>> for dr_mode == "periphearal" -> "otg" and "host" IRQs are not required.

>>>>>>

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

>>>>>>>     		return cdns->dev_irq;

>>>>>>>

>>>>>>> @@ -477,7 +477,7 @@ static int cdns3_probe(struct platform_device *pdev)

>>>>>>>     		return PTR_ERR(regs);

>>>>>>>     	cdns->dev_regs	= regs;

>>>>>>>

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

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

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

>>>>>>>     		return cdns->otg_irq;

>>>>>>>

>>>>>>>

>>>>>>

>>>>>

>>>>> Regards,

>>>>> Pawel

>>>>>

>>>>

>>>> --

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

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

>>

>>--

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

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

Patch

diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index a0f73d4711ae..a3f6dc44cf3a 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -465,7 +465,7 @@  static int cdns3_probe(struct platform_device *pdev)
 
 	cdns->xhci_res[1] = *res;
 
-	cdns->dev_irq = platform_get_irq_byname(pdev, "peripheral");
+	cdns->dev_irq = platform_get_irq_byname_optional(pdev, "peripheral");
 	if (cdns->dev_irq == -EPROBE_DEFER)
 		return cdns->dev_irq;
 
@@ -477,7 +477,7 @@  static int cdns3_probe(struct platform_device *pdev)
 		return PTR_ERR(regs);
 	cdns->dev_regs	= regs;
 
-	cdns->otg_irq = platform_get_irq_byname(pdev, "otg");
+	cdns->otg_irq = platform_get_irq_byname_optional(pdev, "otg");
 	if (cdns->otg_irq == -EPROBE_DEFER)
 		return cdns->otg_irq;