[PATCHv6 2/5] USB: gadget: atmel_usba_udc: Request an auto disabled Vbus signal IRQ instead of an auto enabled IRQ request followed by IRQ disable
Vbus IRQ handler needs a started UDC driver to work because it uses udc->driver, which is set by the UDC start handler. The previous way chosen was to return from interrupt if udc->driver is NULL using a spinlock around the check. We now request an auto disabled (IRQ_NOAUTOEN) Vbus signal IRQ instead of an auto enabled IRQ followed by disable_irq(). This way we remove the very small timeslot of enabled IRQ which existed previously between request() and disable(). We don't need anymore to check if udc->driver is NULL in IRQ handler. Signed-off-by: Sylvain Rochet Suggested-by: Boris Brezillon Acked-by: Boris Brezillon --- drivers/usb/gadget/udc/atmel_usba_udc.c | 9 ++--- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c index 40e5fc7..f9f305f 100644 --- a/drivers/usb/gadget/udc/atmel_usba_udc.c +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c @@ -1745,10 +1745,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid) spin_lock(&udc->lock); - /* May happen if Vbus pin toggles during probe() */ - if (!udc->driver) - goto out; - vbus = vbus_is_present(udc); if (vbus != udc->vbus_prev) { if (vbus) { @@ -1769,7 +1765,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid) udc->vbus_prev = vbus; } -out: spin_unlock(&udc->lock); return IRQ_HANDLED; @@ -2109,6 +2104,8 @@ static int usba_udc_probe(struct platform_device *pdev) if (gpio_is_valid(udc->vbus_pin)) { if (!devm_gpio_request(&pdev->dev, udc->vbus_pin, "atmel_usba_udc")) { + irq_set_status_flags(gpio_to_irq(udc->vbus_pin), + IRQ_NOAUTOEN); ret = devm_request_irq(&pdev->dev, gpio_to_irq(udc->vbus_pin), usba_vbus_irq, 0, @@ -2118,8 +2115,6 @@ static int usba_udc_probe(struct platform_device *pdev) dev_warn(&udc->pdev->dev, "failed to request vbus irq; " "assuming always on\n"); - } else { - disable_irq(gpio_to_irq(udc->vbus_pin)); } } else { /* gpio_request fail so use -EINVAL for gpio_is_valid */ -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 2/5] USB: gadget: atmel_usba_udc: Request an auto disabled Vbus signal IRQ instead of an auto enabled IRQ request followed by IRQ disable
Le 23/01/2015 08:43, Jean-Christophe PLAGNIOL-VILLARD a écrit : > >> On Jan 23, 2015, at 12:56 AM, Sylvain Rochet >> wrote: >> >> Vbus IRQ handler needs a started UDC driver to work because it uses >> udc->driver, which is set by the UDC start handler. The previous way >> chosen was to return from interrupt if udc->driver is NULL using a >> spinlock around the check. >> >> We now request an auto disabled (IRQ_NOAUTOEN) Vbus signal IRQ instead >> of an auto enabled IRQ followed by disable_irq(). This way we remove the >> very small timeslot of enabled IRQ which existed previously between >> request() and disable(). We don't need anymore to check if udc->driver >> is NULL in IRQ handler. >> >> Signed-off-by: Sylvain Rochet >> Suggested-by: Boris Brezillon >> Acked-by: Boris Brezillon >> --- >> drivers/usb/gadget/udc/atmel_usba_udc.c | 9 ++--- >> 1 file changed, 2 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c >> b/drivers/usb/gadget/udc/atmel_usba_udc.c >> index 40e5fc7..f9f305f 100644 >> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c >> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c >> @@ -1745,10 +1745,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid) >> >> spin_lock(&udc->lock); >> >> -/* May happen if Vbus pin toggles during probe() */ >> -if (!udc->driver) >> -goto out; >> - >> vbus = vbus_is_present(udc); >> if (vbus != udc->vbus_prev) { >> if (vbus) { >> @@ -1769,7 +1765,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid) >> udc->vbus_prev = vbus; >> } >> >> -out: >> spin_unlock(&udc->lock); >> >> return IRQ_HANDLED; >> @@ -2109,6 +2104,8 @@ static int usba_udc_probe(struct platform_device *pdev) >> >> if (gpio_is_valid(udc->vbus_pin)) { >> if (!devm_gpio_request(&pdev->dev, udc->vbus_pin, >> "atmel_usba_udc")) { >> +irq_set_status_flags(gpio_to_irq(udc->vbus_pin), >> +IRQ_NOAUTOEN); > > not happy about still using the broken gpio_to_irq API > > Linus can pass the IRQ via board file? > > and via DT we can use IRQ directly Absolutely not the topic of this patch series. Maybe the subject of another patch later? Best regards, >> ret = devm_request_irq(&pdev->dev, >> gpio_to_irq(udc->vbus_pin), >> usba_vbus_irq, 0, >> @@ -2118,8 +2115,6 @@ static int usba_udc_probe(struct platform_device *pdev) >> dev_warn(&udc->pdev->dev, >> "failed to request vbus irq; " >> "assuming always on\n"); >> -} else { >> -disable_irq(gpio_to_irq(udc->vbus_pin)); >> } >> } else { >> /* gpio_request fail so use -EINVAL for gpio_is_valid */ >> -- >> 2.1.4 >> >> >> ___ >> linux-arm-kernel mailing list >> linux-arm-ker...@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > -- Nicolas Ferre -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 2/5] USB: gadget: atmel_usba_udc: Request an auto disabled Vbus signal IRQ instead of an auto enabled IRQ request followed by IRQ disable
Hello, On Fri, Jan 23, 2015 at 09:59:58AM +0100, Nicolas Ferre wrote: > Le 23/01/2015 08:43, Jean-Christophe PLAGNIOL-VILLARD a écrit : > > > On Jan 23, 2015, at 12:56 AM, Sylvain Rochet > > > wrote: > > > + irq_set_status_flags(gpio_to_irq(udc->vbus_pin), > > > + IRQ_NOAUTOEN); > > > > not happy about still using the broken gpio_to_irq API > > > > Linus can pass the IRQ via board file? > > > > and via DT we can use IRQ directly Although I agree on the general principle, this driver is also used by non-DT boards. I have concerns at least for at32ap700x-based boards which use this driver and are non-DT. The suggested change requires changing platform data of at32ap700x in such a way we may break it in my opinion. > Absolutely not the topic of this patch series. > Maybe the subject of another patch later? Indeed. This series is only about PM support, harmless rework, or necessary rework for PM. Changing the way we acquire the IRQ is not harmless, this should be checked carefully for DT and particularly non-DT boards. Sylvain -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 2/5] USB: gadget: atmel_usba_udc: Request an auto disabled Vbus signal IRQ instead of an auto enabled IRQ request followed by IRQ disable
> On Jan 23, 2015, at 12:56 AM, Sylvain Rochet > wrote: > > Vbus IRQ handler needs a started UDC driver to work because it uses > udc->driver, which is set by the UDC start handler. The previous way > chosen was to return from interrupt if udc->driver is NULL using a > spinlock around the check. > > We now request an auto disabled (IRQ_NOAUTOEN) Vbus signal IRQ instead > of an auto enabled IRQ followed by disable_irq(). This way we remove the > very small timeslot of enabled IRQ which existed previously between > request() and disable(). We don't need anymore to check if udc->driver > is NULL in IRQ handler. > > Signed-off-by: Sylvain Rochet > Suggested-by: Boris Brezillon > Acked-by: Boris Brezillon > --- > drivers/usb/gadget/udc/atmel_usba_udc.c | 9 ++--- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c > b/drivers/usb/gadget/udc/atmel_usba_udc.c > index 40e5fc7..f9f305f 100644 > --- a/drivers/usb/gadget/udc/atmel_usba_udc.c > +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c > @@ -1745,10 +1745,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid) > > spin_lock(&udc->lock); > > - /* May happen if Vbus pin toggles during probe() */ > - if (!udc->driver) > - goto out; > - > vbus = vbus_is_present(udc); > if (vbus != udc->vbus_prev) { > if (vbus) { > @@ -1769,7 +1765,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid) > udc->vbus_prev = vbus; > } > > -out: > spin_unlock(&udc->lock); > > return IRQ_HANDLED; > @@ -2109,6 +2104,8 @@ static int usba_udc_probe(struct platform_device *pdev) > > if (gpio_is_valid(udc->vbus_pin)) { > if (!devm_gpio_request(&pdev->dev, udc->vbus_pin, > "atmel_usba_udc")) { > + irq_set_status_flags(gpio_to_irq(udc->vbus_pin), > + IRQ_NOAUTOEN); not happy about still using the broken gpio_to_irq API Linus can pass the IRQ via board file? and via DT we can use IRQ directly Best Regards, J. > ret = devm_request_irq(&pdev->dev, > gpio_to_irq(udc->vbus_pin), > usba_vbus_irq, 0, > @@ -2118,8 +2115,6 @@ static int usba_udc_probe(struct platform_device *pdev) > dev_warn(&udc->pdev->dev, >"failed to request vbus irq; " >"assuming always on\n"); > - } else { > - disable_irq(gpio_to_irq(udc->vbus_pin)); > } > } else { > /* gpio_request fail so use -EINVAL for gpio_is_valid */ > -- > 2.1.4 > > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 2/5] USB: gadget: atmel_usba_udc: Request an auto disabled Vbus signal IRQ instead of an auto enabled IRQ request followed by IRQ disable
Le 22/01/2015 17:56, Sylvain Rochet a écrit : > Vbus IRQ handler needs a started UDC driver to work because it uses > udc->driver, which is set by the UDC start handler. The previous way > chosen was to return from interrupt if udc->driver is NULL using a > spinlock around the check. > > We now request an auto disabled (IRQ_NOAUTOEN) Vbus signal IRQ instead > of an auto enabled IRQ followed by disable_irq(). This way we remove the > very small timeslot of enabled IRQ which existed previously between > request() and disable(). We don't need anymore to check if udc->driver > is NULL in IRQ handler. > > Signed-off-by: Sylvain Rochet > Suggested-by: Boris Brezillon > Acked-by: Boris Brezillon Acked-by: Nicolas Ferre > --- > drivers/usb/gadget/udc/atmel_usba_udc.c | 9 ++--- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c > b/drivers/usb/gadget/udc/atmel_usba_udc.c > index 40e5fc7..f9f305f 100644 > --- a/drivers/usb/gadget/udc/atmel_usba_udc.c > +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c > @@ -1745,10 +1745,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid) > > spin_lock(&udc->lock); > > - /* May happen if Vbus pin toggles during probe() */ > - if (!udc->driver) > - goto out; > - > vbus = vbus_is_present(udc); > if (vbus != udc->vbus_prev) { > if (vbus) { > @@ -1769,7 +1765,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid) > udc->vbus_prev = vbus; > } > > -out: > spin_unlock(&udc->lock); > > return IRQ_HANDLED; > @@ -2109,6 +2104,8 @@ static int usba_udc_probe(struct platform_device *pdev) > > if (gpio_is_valid(udc->vbus_pin)) { > if (!devm_gpio_request(&pdev->dev, udc->vbus_pin, > "atmel_usba_udc")) { > + irq_set_status_flags(gpio_to_irq(udc->vbus_pin), > + IRQ_NOAUTOEN); > ret = devm_request_irq(&pdev->dev, > gpio_to_irq(udc->vbus_pin), > usba_vbus_irq, 0, > @@ -2118,8 +2115,6 @@ static int usba_udc_probe(struct platform_device *pdev) > dev_warn(&udc->pdev->dev, >"failed to request vbus irq; " >"assuming always on\n"); > - } else { > - disable_irq(gpio_to_irq(udc->vbus_pin)); > } > } else { > /* gpio_request fail so use -EINVAL for gpio_is_valid */ > -- Nicolas Ferre -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html