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@xxxxxxxxxxx> >>>>>>> --- >>>>>>> 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
