Hi Sylvain,
On Tue, 20 Jan 2015 22:23:29 +0100
Sylvain Rochet <sylvain.rochet@xxxxxxxxxxxx> 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.
>
> This patch now request the Vbus signal IRQ in UDC start instead of UDC
> probe and release the IRQ in UDC stop before udc->driver is set back to
> NULL. This way we don't need the check about udc->driver in interruption
> handler, therefore we don't need the spinlock as well anymore.
>
> This was chosen against using set_irq_flags() to request a not auto
> enabled IRQ (IRQF_NOAUTOEN flag) because set_irq_flags() can't change
> just one flag, therefore it must be called with all flags, without
> respecting what the AIC previously did. Naively copying IRQ flags
> currently set by the AIC looked like error-prone if defaults flags
> change at some point in the future.
>
> Signed-off-by: Sylvain Rochet <sylvain.rochet@xxxxxxxxxxxx>
> Suggested-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> ---
> drivers/usb/gadget/udc/atmel_usba_udc.c | 64 ++++++++++++++++-----------------
> 1 file changed, 32 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index e207d75..546da63 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -1729,10 +1729,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) {
> @@ -1753,7 +1749,6 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
> udc->vbus_prev = vbus;
> }
>
> -out:
> spin_unlock(&udc->lock);
>
> return IRQ_HANDLED;
> @@ -1767,23 +1762,27 @@ static int atmel_usba_start(struct usb_gadget *gadget,
> unsigned long flags;
>
> spin_lock_irqsave(&udc->lock, flags);
> -
> udc->devstatus = 1 << USB_DEVICE_SELF_POWERED;
> udc->driver = driver;
> spin_unlock_irqrestore(&udc->lock, flags);
>
> ret = clk_prepare_enable(udc->pclk);
> if (ret)
> - return ret;
> + goto err_pclk;
> ret = clk_prepare_enable(udc->hclk);
> - if (ret) {
> - clk_disable_unprepare(udc->pclk);
> - return ret;
> - }
> + if (ret)
> + goto err_hclk;
>
> udc->vbus_prev = 0;
> - if (gpio_is_valid(udc->vbus_pin))
> - enable_irq(gpio_to_irq(udc->vbus_pin));
> + if (gpio_is_valid(udc->vbus_pin)) {
> + ret = request_irq(gpio_to_irq(udc->vbus_pin),
> + usba_vbus_irq, 0,
> + "atmel_usba_udc", udc);
> + if (ret) {
> + udc->vbus_pin = -ENODEV;
I guess you're trying to protect against free_irq by changing the
vbus_pin value (making it an invalid gpio id), but I think you should
leave it unchanged for two reasons:
1) If the request_irq call temporary fails (an ENOMEM for example) then
you should be able to retry later, and modifying the vbus_pin value
will prevent that.
2) atmel_usba_stop will never be called if the atmel_usba_start failed,
so there's no need to protect against this free_irq.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
