* [PATCH] usb: cdns3: platform_get_irq_byname_optional instead platform_get_irq_byname
@ 2020-09-30 6:57 Pawel Laszczak
2020-09-30 7:24 ` Pawel Laszczak
2020-10-02 8:53 ` Roger Quadros
0 siblings, 2 replies; 9+ messages in thread
From: Pawel Laszczak @ 2020-09-30 6:57 UTC (permalink / raw)
To: balbi
Cc: peter.chen, pawell, rogerq, gregkh, linux-usb, linux-kernel, kurahul
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCH] usb: cdns3: platform_get_irq_byname_optional instead platform_get_irq_byname
2020-09-30 6:57 [PATCH] usb: cdns3: platform_get_irq_byname_optional instead platform_get_irq_byname Pawel Laszczak
@ 2020-09-30 7:24 ` Pawel Laszczak
2020-10-02 8:53 ` Roger Quadros
1 sibling, 0 replies; 9+ messages in thread
From: Pawel Laszczak @ 2020-09-30 7:24 UTC (permalink / raw)
To: balbi
Cc: peter.chen, rogerq, gregkh, linux-usb, linux-kernel, Rahul Kumar,
Pawel Laszczak
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: cdns3: platform_get_irq_byname_optional instead platform_get_irq_byname
2020-09-30 6:57 [PATCH] usb: cdns3: platform_get_irq_byname_optional instead platform_get_irq_byname Pawel Laszczak
2020-09-30 7:24 ` Pawel Laszczak
@ 2020-10-02 8:53 ` Roger Quadros
2020-10-02 9:08 ` Pawel Laszczak
1 sibling, 1 reply; 9+ messages in thread
From: Roger Quadros @ 2020-10-02 8:53 UTC (permalink / raw)
To: Pawel Laszczak, balbi
Cc: peter.chen, gregkh, linux-usb, linux-kernel, kurahul
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
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] usb: cdns3: platform_get_irq_byname_optional instead platform_get_irq_byname
2020-10-02 8:53 ` Roger Quadros
@ 2020-10-02 9:08 ` Pawel Laszczak
2020-10-02 10:06 ` Roger Quadros
0 siblings, 1 reply; 9+ messages in thread
From: Pawel Laszczak @ 2020-10-02 9:08 UTC (permalink / raw)
To: Roger Quadros, balbi
Cc: peter.chen, gregkh, linux-usb, linux-kernel, Rahul Kumar
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: cdns3: platform_get_irq_byname_optional instead platform_get_irq_byname
2020-10-02 9:08 ` Pawel Laszczak
@ 2020-10-02 10:06 ` Roger Quadros
2020-10-05 5:54 ` Pawel Laszczak
0 siblings, 1 reply; 9+ messages in thread
From: Roger Quadros @ 2020-10-02 10:06 UTC (permalink / raw)
To: Pawel Laszczak, balbi
Cc: peter.chen, gregkh, linux-usb, linux-kernel, Rahul Kumar
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
>
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] usb: cdns3: platform_get_irq_byname_optional instead platform_get_irq_byname
2020-10-02 10:06 ` Roger Quadros
@ 2020-10-05 5:54 ` Pawel Laszczak
2020-10-05 8:43 ` Roger Quadros
0 siblings, 1 reply; 9+ messages in thread
From: Pawel Laszczak @ 2020-10-05 5:54 UTC (permalink / raw)
To: Roger Quadros, balbi
Cc: peter.chen, gregkh, linux-usb, linux-kernel, Rahul Kumar
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: cdns3: platform_get_irq_byname_optional instead platform_get_irq_byname
2020-10-05 5:54 ` Pawel Laszczak
@ 2020-10-05 8:43 ` Roger Quadros
2020-10-05 8:49 ` Pawel Laszczak
0 siblings, 1 reply; 9+ messages in thread
From: Roger Quadros @ 2020-10-05 8:43 UTC (permalink / raw)
To: Pawel Laszczak, balbi
Cc: peter.chen, gregkh, linux-usb, linux-kernel, Rahul Kumar
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
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] usb: cdns3: platform_get_irq_byname_optional instead platform_get_irq_byname
2020-10-05 8:43 ` Roger Quadros
@ 2020-10-05 8:49 ` Pawel Laszczak
2020-10-07 3:12 ` Pawel Laszczak
0 siblings, 1 reply; 9+ messages in thread
From: Pawel Laszczak @ 2020-10-05 8:49 UTC (permalink / raw)
To: Roger Quadros, balbi
Cc: Peter Chen, gregkh, linux-usb, linux-kernel, Rahul Kumar
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] usb: cdns3: platform_get_irq_byname_optional instead platform_get_irq_byname
2020-10-05 8:49 ` Pawel Laszczak
@ 2020-10-07 3:12 ` Pawel Laszczak
0 siblings, 0 replies; 9+ messages in thread
From: Pawel Laszczak @ 2020-10-07 3:12 UTC (permalink / raw)
To: Roger Quadros, balbi
Cc: Peter Chen, gregkh, linux-usb, linux-kernel, Rahul Kumar
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-10-07 3:13 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-30 6:57 [PATCH] usb: cdns3: platform_get_irq_byname_optional instead platform_get_irq_byname Pawel Laszczak
2020-09-30 7:24 ` Pawel Laszczak
2020-10-02 8:53 ` Roger Quadros
2020-10-02 9:08 ` Pawel Laszczak
2020-10-02 10:06 ` Roger Quadros
2020-10-05 5:54 ` Pawel Laszczak
2020-10-05 8:43 ` Roger Quadros
2020-10-05 8:49 ` Pawel Laszczak
2020-10-07 3:12 ` Pawel Laszczak
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.