lkml.org 
[lkml]   [2020]   [Oct]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] usb: cdns3: platform_get_irq_byname_optional instead platform_get_irq_byname
    From
    Date
    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

    \
     
     \ /
      Last update: 2020-10-05 10:44    [W:4.191 / U:0.552 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site