Re: [PATCH v9 5/5] usb: dwc3: core: cleanup IRQ resources

[Roger Quadros <rogerq@ti.com>]

On 02/06/16 14:52, Grygorii Strashko wrote:
> On 06/01/2016 10:46 AM, Roger Quadros wrote:
>> Implementations might use different IRQs for
>> host, gadget and OTG so use named interrupt resources
>> to allow Device tree to specify the 3 interrupts.
>>
>> Following are the interrupt names
>>
>> Peripheral Interrupt - peripheral
>> HOST Interrupt - host
>> OTG Interrupt - otg
> 
> or "dwc_usb3"??

That is for backward compatibility only. I could explicitly
mention it in the next line.

> 
>>
>> We still maintain backward compatibility for a single named
>> interrupt for all 3 interrupts (e.g. for dwc3-pci) and
>> single unnamed interrupt for all 3 interrupts (e.g. old DT).
> 
> bindings

OK.
> 
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>> v9: rebased on top of balbi/testing/next
>>
>>   drivers/usb/dwc3/core.c   | 10 ----------
>>   drivers/usb/dwc3/gadget.c | 20 ++++++++++++++++++--
>>   drivers/usb/dwc3/host.c   | 19 +++++++++++++++++++
>>   3 files changed, 37 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 9c4e1d8d..5cedf3d 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -843,16 +843,6 @@ static int dwc3_probe(struct platform_device *pdev)
>>       dwc->mem = mem;
>>       dwc->dev = dev;
>>
>> -    res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> -    if (!res) {
>> -        dev_err(dev, "missing IRQ\n");
>> -        return -ENODEV;
>> -    }
>> -    dwc->xhci_resources[1].start = res->start;
>> -    dwc->xhci_resources[1].end = res->end;
>> -    dwc->xhci_resources[1].flags = res->flags;
>> -    dwc->xhci_resources[1].name = res->name;
>> -
>>       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>       if (!res) {
>>           dev_err(dev, "missing memory resource\n");
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index c37168d..c18c72f 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1726,7 +1726,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>>       int            ret = 0;
>>       int            irq;
>>
>> -    irq = platform_get_irq(to_platform_device(dwc->dev), 0);
>> +    irq = dwc->irq_gadget;
>>       ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
>>               IRQF_SHARED, "dwc3", dwc->ev_buf);
>>       if (ret) {
>> @@ -1734,7 +1734,6 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>>                   irq, ret);
>>           goto err0;
>>       }
>> -    dwc->irq_gadget = irq;
>>
>>       spin_lock_irqsave(&dwc->lock, flags);
>>       if (dwc->gadget_driver) {
>> @@ -2853,6 +2852,23 @@ static irqreturn_t dwc3_interrupt(int irq, void *_evt)
>>   int dwc3_gadget_init(struct dwc3 *dwc)
>>   {
>>       int                    ret;
>> +    struct resource *res;
>> +    struct platform_device *dwc3_pdev = to_platform_device(dwc->dev);
>> +
>> +    dwc->irq_gadget = platform_get_irq_byname(dwc3_pdev, "peripheral");
>> +    if (dwc->irq_gadget <= 0) {
> 
> Is it expected to get -EPROBE_DEFER here?

Probably not as we don't have any chance of deferring probe here. We've already
probed successfully and are just turning on the gadget mode here.

> 
>> +        dwc->irq_gadget = platform_get_irq_byname(dwc3_pdev,
>> +                              "dwc_usb3");
>> +        if (dwc->irq_gadget <= 0) {
>> +            res = platform_get_resource(dwc3_pdev, IORESOURCE_IRQ,
>> +                            0);
> 
> It's better to use platform_get_irq().

OK.
> 
>> +            if (!res) {
>> +                dev_err(dwc->dev, "missing peripheral IRQ\n");
>> +                return -ENODEV;
>> +            }
>> +            dwc->irq_gadget = res->start;
>> +        }
>> +    }
>>
>>       dwc->ctrl_req = dma_alloc_coherent(dwc->dev, sizeof(*dwc->ctrl_req),
>>               &dwc->ctrl_req_addr, GFP_KERNEL);
>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>> index c679f63..f2b60a4 100644
>> --- a/drivers/usb/dwc3/host.c
>> +++ b/drivers/usb/dwc3/host.c
>> @@ -25,6 +25,25 @@ int dwc3_host_init(struct dwc3 *dwc)
>>       struct platform_device    *xhci;
>>       struct usb_xhci_pdata    pdata;
>>       int            ret;
>> +    struct resource        *res;
>> +    struct platform_device    *dwc3_pdev = to_platform_device(dwc->dev);
>> +
>> +    res = platform_get_resource_byname(dwc3_pdev, IORESOURCE_IRQ, "host");
>> +    if (!res) {
>> +        res = platform_get_resource_byname(dwc3_pdev, IORESOURCE_IRQ,
>> +                           "dwc_usb3");
>> +        if (!res) {
>> +            res = platform_get_resource(dwc3_pdev, IORESOURCE_IRQ,
>> +                            0);
>> +            if (!res)
> 
>> +                return -ENOMEM;
>> +        }
>> +    }
> 
> Is it expected to have more than one IRQ here?

No.
> 
> if not - it will better to use platform_get_irq[_byname]().

OK.
> 
> 
>> +
>> +    dwc->xhci_resources[1].start = res->start;
>> +    dwc->xhci_resources[1].end = res->end;
>> +    dwc->xhci_resources[1].flags = res->flags;
>> +    dwc->xhci_resources[1].name = res->name;
>>
>>       xhci = platform_device_alloc("xhci-hcd", PLATFORM_DEVID_AUTO);
>>       if (!xhci) {
>>
> 
> 

cheers,
-roger