subject:"\[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"

[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

2015-01-22 Thread Sylvain Rochet

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

2015-01-23 Thread Nicolas Ferre

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

2015-01-23 Thread Sylvain Rochet

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

2015-01-23 Thread Jean-Christophe PLAGNIOL-VILLARD


> 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

2015-02-05 Thread Nicolas Ferre

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

[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

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

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

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

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

5 matches

Site Navigation

Mail list logo

Footer information