[PATCH] usb: gadget: lpc32xx_udc: Port to new start/stop interface

[PATCH] usb: gadget: lpc32xx_udc: Port to new start/stop interface

[Roland Stigge]

This patch adjusts the LPC32xx USB gadget driver to the new udc_start /
udc_stop interface.

Signed-off-by: Roland Stigge <stigge@antcom.de>

---

Applies to v3.6-rc1

 drivers/usb/gadget/lpc32xx_udc.c |   44 +++++++++++++--------------------------
 1 file changed, 15 insertions(+), 29 deletions(-)

--- linux-2.6.orig/drivers/usb/gadget/lpc32xx_udc.c
+++ linux-2.6/drivers/usb/gadget/lpc32xx_udc.c
@@ -2599,9 +2599,8 @@ static int lpc32xx_pullup(struct usb_gad
 	return 0;
 }
 
-static int lpc32xx_start(struct usb_gadget_driver *driver,
-			 int (*bind)(struct usb_gadget *));
-static int lpc32xx_stop(struct usb_gadget_driver *driver);
+static int lpc32xx_start(struct usb_gadget *, struct usb_gadget_driver *);
+static int lpc32xx_stop(struct usb_gadget *, struct usb_gadget_driver *);
 
 static const struct usb_gadget_ops lpc32xx_udc_ops = {
 	.get_frame		= lpc32xx_get_frame,
@@ -2609,8 +2608,8 @@ static const struct usb_gadget_ops lpc32
 	.set_selfpowered	= lpc32xx_set_selfpowered,
 	.vbus_session		= lpc32xx_vbus_session,
 	.pullup			= lpc32xx_pullup,
-	.start			= lpc32xx_start,
-	.stop			= lpc32xx_stop,
+	.udc_start		= lpc32xx_start,
+	.udc_stop		= lpc32xx_stop,
 };
 
 static void nop_release(struct device *dev)
@@ -2987,14 +2986,14 @@ static irqreturn_t lpc32xx_usb_vbus_irq(
 	return IRQ_HANDLED;
 }
 
-static int lpc32xx_start(struct usb_gadget_driver *driver,
-			 int (*bind)(struct usb_gadget *))
+static int lpc32xx_start(struct usb_gadget *gadget,
+			 struct usb_gadget_driver *driver)
 {
-	struct lpc32xx_udc *udc = &controller;
-	int retval, i;
+	struct lpc32xx_udc *udc =
+		container_of(gadget, struct lpc32xx_udc, gadget);
+	int i;
 
-	if (!driver || driver->max_speed < USB_SPEED_FULL ||
-	    !bind || !driver->setup) {
+	if (!driver || driver->max_speed < USB_SPEED_FULL || !driver->setup) {
 		dev_err(udc->dev, "bad parameter.\n");
 		return -EINVAL;
 	}
@@ -3011,18 +3010,6 @@ static int lpc32xx_start(struct usb_gadg
 	udc->selfpowered = 1;
 	udc->vbus = 0;
 
-	retval = bind(&udc->gadget);
-	if (retval) {
-		dev_err(udc->dev, "bind() returned %d\n", retval);
-		udc->enabled = 0;
-		udc->selfpowered = 0;
-		udc->driver = NULL;
-		udc->gadget.dev.driver = NULL;
-		return retval;
-	}
-
-	dev_dbg(udc->dev, "bound to %s\n", driver->driver.name);
-
 	/* Force VBUS process once to check for cable insertion */
 	udc->last_vbus = udc->vbus = 0;
 	schedule_work(&udc->vbus_job);
@@ -3034,12 +3021,14 @@ static int lpc32xx_start(struct usb_gadg
 	return 0;
 }
 
-static int lpc32xx_stop(struct usb_gadget_driver *driver)
+static int lpc32xx_stop(struct usb_gadget *gadget,
+			struct usb_gadget_driver *driver)
 {
 	int i;
-	struct lpc32xx_udc *udc = &controller;
+	struct lpc32xx_udc *udc =
+		container_of(gadget, struct lpc32xx_udc, gadget);
 
-	if (!driver || driver != udc->driver || !driver->unbind)
+	if (!driver || driver != udc->driver)
 		return -EINVAL;
 
 	/* Disable USB pullup */
@@ -3049,7 +3038,6 @@ static int lpc32xx_stop(struct usb_gadge
 		disable_irq(udc->udp_irq[i]);
 
 	if (udc->clocked) {
-
 		spin_lock(&udc->lock);
 		stop_activity(udc);
 		spin_unlock(&udc->lock);
@@ -3071,11 +3059,9 @@ static int lpc32xx_stop(struct usb_gadge
 	udc->enabled = 0;
 	pullup(udc, 0);
 
-	driver->unbind(&udc->gadget);
 	udc->gadget.dev.driver = NULL;
 	udc->driver = NULL;
 
-	dev_dbg(udc->dev, "unbound from %s\n", driver->driver.name);
 	return 0;
 }
 

Re: [PATCH] usb: gadget: lpc32xx_udc: Port to new start/stop interface

[Sebastian Andrzej Siewior]

On 08/16/2012 05:34 PM, Roland Stigge wrote:
> --- linux-2.6.orig/drivers/usb/gadget/lpc32xx_udc.c
> +++ linux-2.6/drivers/usb/gadget/lpc32xx_udc.c
> @@ -2987,14 +2986,14 @@ static irqreturn_t lpc32xx_usb_vbus_irq(
>   	return IRQ_HANDLED;
>   }
>
> -static int lpc32xx_start(struct usb_gadget_driver *driver,
> -			 int (*bind)(struct usb_gadget *))
> +static int lpc32xx_start(struct usb_gadget *gadget,
> +			 struct usb_gadget_driver *driver)
>   {
> -	struct lpc32xx_udc *udc =&controller;

I assume controller is a global var created at probe time and could be
removed now, right?

> -	int retval, i;
> +	struct lpc32xx_udc *udc =
> +		container_of(gadget, struct lpc32xx_udc, gadget);
> +	int i;


Sebastian

Re: [PATCH] usb: gadget: lpc32xx_udc: Port to new start/stop interface

[Roland Stigge]

On 08/16/2012 06:05 PM, Sebastian Andrzej Siewior wrote:
>> --- linux-2.6.orig/drivers/usb/gadget/lpc32xx_udc.c
>> +++ linux-2.6/drivers/usb/gadget/lpc32xx_udc.c
>> @@ -2987,14 +2986,14 @@ static irqreturn_t lpc32xx_usb_vbus_irq(
>>       return IRQ_HANDLED;
>>   }
>>
>> -static int lpc32xx_start(struct usb_gadget_driver *driver,
>> -             int (*bind)(struct usb_gadget *))
>> +static int lpc32xx_start(struct usb_gadget *gadget,
>> +             struct usb_gadget_driver *driver)
>>   {
>> -    struct lpc32xx_udc *udc =&controller;
> 
> I assume controller is a global var created at probe time and could be
> removed now, right?

Yes!

Re: [PATCH] usb: gadget: lpc32xx_udc: Port to new start/stop interface

[Roland Stigge]

Hi,

On 08/16/2012 06:16 PM, Roland Stigge wrote:
> On 08/16/2012 06:05 PM, Sebastian Andrzej Siewior wrote:
>>> --- linux-2.6.orig/drivers/usb/gadget/lpc32xx_udc.c
>>> +++ linux-2.6/drivers/usb/gadget/lpc32xx_udc.c
>>> @@ -2987,14 +2986,14 @@ static irqreturn_t lpc32xx_usb_vbus_irq(
>>>       return IRQ_HANDLED;
>>>   }
>>>
>>> -static int lpc32xx_start(struct usb_gadget_driver *driver,
>>> -             int (*bind)(struct usb_gadget *))
>>> +static int lpc32xx_start(struct usb_gadget *gadget,
>>> +             struct usb_gadget_driver *driver)
>>>   {
>>> -    struct lpc32xx_udc *udc =&controller;
>>
>> I assume controller is a global var created at probe time and could be
>> removed now, right?
> 
> Yes!

Well ;-) looking more closely into it, I'd like to keep it for now: It
is a more complex statically pre-initialized struct, finally being used
in probe() for more dynamic initialization, and it ends up being used by
many other functions, including lpc32xx_start() where accessing it now
via container_of() is just a bit more elegant than a direct &controller
of the global variable.

Also, since this device is a single controller in the LPC32xx SoC, I
would keep it until some other silicon uses several of this IP core
(which I doubt), at which point we would probably still keep the (global
static) controller and memcpy it to a dynamically allocated struct.

Sounds reasonable?

Thanks,

Roland

Re: [PATCH] usb: gadget: lpc32xx_udc: Port to new start/stop interface

[Sebastian Andrzej Siewior]

On 08/17/2012 11:10 AM, Roland Stigge wrote:
> Also, since this device is a single controller in the LPC32xx SoC, I
> would keep it until some other silicon uses several of this IP core
> (which I doubt), at which point we would probably still keep the (global
> static) controller and memcpy it to a dynamically allocated struct.
>
> Sounds reasonable?

Yes it does.

Some minor things:
- please use to_udc() in start then
- would it make sense to use platform_get_drvdata() in
   lpc32xx_udc_shutdown() ?
- could you please remove struct usb_endpoint_descriptor from struct
   lpc32xx_ep? It has been removed a while back from other drivers.

Your proc_udc_show() makes me sad. Felipe, didn't we want something
like this in udc-core?

> Thanks,
>
> Roland

Sebastian

Re: [PATCH] usb: gadget: lpc32xx_udc: Port to new start/stop interface

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1751 bytes --]

On Fri, Aug 17, 2012 at 11:10:19AM +0200, Roland Stigge wrote:
> Hi,
> 
> On 08/16/2012 06:16 PM, Roland Stigge wrote:
> > On 08/16/2012 06:05 PM, Sebastian Andrzej Siewior wrote:
> >>> --- linux-2.6.orig/drivers/usb/gadget/lpc32xx_udc.c
> >>> +++ linux-2.6/drivers/usb/gadget/lpc32xx_udc.c
> >>> @@ -2987,14 +2986,14 @@ static irqreturn_t lpc32xx_usb_vbus_irq(
> >>>       return IRQ_HANDLED;
> >>>   }
> >>>
> >>> -static int lpc32xx_start(struct usb_gadget_driver *driver,
> >>> -             int (*bind)(struct usb_gadget *))
> >>> +static int lpc32xx_start(struct usb_gadget *gadget,
> >>> +             struct usb_gadget_driver *driver)
> >>>   {
> >>> -    struct lpc32xx_udc *udc =&controller;
> >>
> >> I assume controller is a global var created at probe time and could be
> >> removed now, right?
> > 
> > Yes!
> 
> Well ;-) looking more closely into it, I'd like to keep it for now: It
> is a more complex statically pre-initialized struct, finally being used
> in probe() for more dynamic initialization, and it ends up being used by
> many other functions, including lpc32xx_start() where accessing it now
> via container_of() is just a bit more elegant than a direct &controller
> of the global variable.
> 
> Also, since this device is a single controller in the LPC32xx SoC, I
> would keep it until some other silicon uses several of this IP core
> (which I doubt), at which point we would probably still keep the (global
> static) controller and memcpy it to a dynamically allocated struct.
> 
> Sounds reasonable?

no it doesn't. Please remove that static global. Sorry but one of the
goals with udc_start/udc_stop was really to get rid of all those
nonsensical static globals.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: gadget: lpc32xx_udc: Port to new start/stop interface

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

On Fri, Aug 17, 2012 at 11:42:15AM +0200, Sebastian Andrzej Siewior wrote:
> On 08/17/2012 11:10 AM, Roland Stigge wrote:
> >Also, since this device is a single controller in the LPC32xx SoC, I
> >would keep it until some other silicon uses several of this IP core
> >(which I doubt), at which point we would probably still keep the (global
> >static) controller and memcpy it to a dynamically allocated struct.
> >
> >Sounds reasonable?
> 
> Yes it does.
> 
> Some minor things:
> - please use to_udc() in start then
> - would it make sense to use platform_get_drvdata() in
>   lpc32xx_udc_shutdown() ?
> - could you please remove struct usb_endpoint_descriptor from struct
>   lpc32xx_ep? It has been removed a while back from other drivers.
> 
> Your proc_udc_show() makes me sad. Felipe, didn't we want something
> like this in udc-core?

Yes we do, though not a single file with a bunch of values. We want one
value per file as it should be.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: gadget: lpc32xx_udc: Port to new start/stop interface

[Sebastian Andrzej Siewior]

On 08/17/2012 12:51 PM, Felipe Balbi wrote:
>> Sounds reasonable?
>
> no it doesn't. Please remove that static global. Sorry but one of the
> goals with udc_start/udc_stop was really to get rid of all those
> nonsensical static globals.

I wouldn't insist on it. If you look at pxa25x, " static struct
pxa25x_udc memory = {" they do the same thing. It is only a simple way
to onetime initialize variables.

Sebastian

Re: [PATCH] usb: gadget: lpc32xx_udc: Port to new start/stop interface

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 855 bytes --]

On Fri, Aug 17, 2012 at 01:01:44PM +0200, Sebastian Andrzej Siewior wrote:
> On 08/17/2012 12:51 PM, Felipe Balbi wrote:
> >>Sounds reasonable?
> >
> >no it doesn't. Please remove that static global. Sorry but one of the
> >goals with udc_start/udc_stop was really to get rid of all those
> >nonsensical static globals.
> 
> I wouldn't insist on it. If you look at pxa25x, " static struct
> pxa25x_udc memory = {" they do the same thing. It is only a simple way
> to onetime initialize variables.

fair enough. Though that's wrong too and should be changed. The whole
idea of allowing multiple UDCs on the same kernel image is mostly to aid
development. Specially on pre-silicon phase. We could have a bunch of
PCIe FPGA boards and instantiate a different controller on each one and
have the same kernel work with them all.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] usb: gadget: lpc32xx_udc: Port to new start/stop interface

[Sebastian Andrzej Siewior]

On 08/17/2012 01:02 PM, Felipe Balbi wrote:
> On Fri, Aug 17, 2012 at 01:01:44PM +0200, Sebastian Andrzej Siewior wrote:
>> On 08/17/2012 12:51 PM, Felipe Balbi wrote:
>>>> Sounds reasonable?
>>>
>>> no it doesn't. Please remove that static global. Sorry but one of the
>>> goals with udc_start/udc_stop was really to get rid of all those
>>> nonsensical static globals.
>>
>> I wouldn't insist on it. If you look at pxa25x, " static struct
>> pxa25x_udc memory = {" they do the same thing. It is only a simple way
>> to onetime initialize variables.
>
> fair enough. Though that's wrong too and should be changed. The whole
> idea of allowing multiple UDCs on the same kernel image is mostly to aid
> development. Specially on pre-silicon phase. We could have a bunch of
> PCIe FPGA boards and instantiate a different controller on each one and
> have the same kernel work with them all.

True. And he said he is going to fix it once they have multiple UDC. I
didn't remove the global thingy from any driver I converted if it was
used as a way to easy initialize things. And I kept only one reference
to it in the probe code. This is the only thing I ask for.

Sebastian

Re: [PATCH] usb: gadget: lpc32xx_udc: Port to new start/stop interface

[Roland Stigge]

On 08/17/2012 01:02 PM, Felipe Balbi wrote:
> On Fri, Aug 17, 2012 at 01:01:44PM +0200, Sebastian Andrzej Siewior wrote:
>> On 08/17/2012 12:51 PM, Felipe Balbi wrote:
>>>> Sounds reasonable?
>>>
>>> no it doesn't. Please remove that static global. Sorry but one of the
>>> goals with udc_start/udc_stop was really to get rid of all those
>>> nonsensical static globals.
>>
>> I wouldn't insist on it. If you look at pxa25x, " static struct
>> pxa25x_udc memory = {" they do the same thing. It is only a simple way
>> to onetime initialize variables.
> 
> fair enough. Though that's wrong too and should be changed. The whole
> idea of allowing multiple UDCs on the same kernel image is mostly to aid
> development. Specially on pre-silicon phase. We could have a bunch of
> PCIe FPGA boards and instantiate a different controller on each one and
> have the same kernel work with them all.

Yes, that's a good thing and I like to support it.

How about the following: Below, I show how the initialization of the
current controller is done (statically). Removing this struct
initialization completely would make the code much uglier, introducing
many individual assignments.

Would it be OK to kzalloc() on initialization to enable multiple UDCs
and then memcpy() from a statically prepared template as below? (Some
dynamic pointer adjustments necessary, of course.)

Thanks in advance,

Roland



static struct lpc32xx_udc controller = {
        .gadget = {
                .ops    = &lpc32xx_udc_ops,
                .ep0    = &controller.ep[0].ep,
                .name   = driver_name,
                .dev    = {
                        .init_name = "gadget",
                        .release = nop_release,
                }
        },
        .ep[0] = {
                .ep = {
                        .name   = "ep0",
                        .ops    = &lpc32xx_ep_ops,
                },
                .udc            = &controller,
                .maxpacket      = 64,
                .hwep_num_base  = 0,
                .hwep_num       = 0, /* Can be 0 or 1, has special
handling */
                .lep            = 0,
                .eptype         = EP_CTL_TYPE,
        },
        .ep[1] = {
                .ep = {
                        .name   = "ep1-int",
                        .ops    = &lpc32xx_ep_ops,
                },
                .udc            = &controller,
                .maxpacket      = 64,
                .hwep_num_base  = 2,
                .hwep_num       = 0, /* 2 or 3, will be set later */
                .lep            = 1,
                .eptype         = EP_INT_TYPE,
        },
        .ep[2] = {
                .ep = {
                        .name   = "ep2-bulk",
                        .ops    = &lpc32xx_ep_ops,
                },
                .udc            = &controller,
                .maxpacket      = 64,
                .hwep_num_base  = 4,
                .hwep_num       = 0, /* 4 or 5, will be set later */
                .lep            = 2,
                .eptype         = EP_BLK_TYPE,
        },
        .ep[3] = {
                .ep = {
                        .name   = "ep3-iso",
                        .ops    = &lpc32xx_ep_ops,
                },
                .udc            = &controller,
                .maxpacket      = 1023,
                .hwep_num_base  = 6,
                .hwep_num       = 0, /* 6 or 7, will be set later */
                .lep            = 3,
                .eptype         = EP_ISO_TYPE,
        },
        .ep[4] = {
                .ep = {
                        .name   = "ep4-int",
                        .ops    = &lpc32xx_ep_ops,
                },
                .udc            = &controller,
                .maxpacket      = 64,
                .hwep_num_base  = 8,
                .hwep_num       = 0, /* 8 or 9, will be set later */
                .lep            = 4,
                .eptype         = EP_INT_TYPE,
        },
        .ep[5] = {
                .ep = {
                        .name   = "ep5-bulk",
                        .ops    = &lpc32xx_ep_ops,
                },
                .udc            = &controller,
                .maxpacket      = 64,
                .hwep_num_base  = 10,
                .hwep_num       = 0, /* 10 or 11, will be set later */
                .lep            = 5,
                .eptype         = EP_BLK_TYPE,
        },
        .ep[6] = {
                .ep = {
                        .name   = "ep6-iso",
                        .ops    = &lpc32xx_ep_ops,
                },
                .udc            = &controller,
                .maxpacket      = 1023,
                .hwep_num_base  = 12,
                .hwep_num       = 0, /* 12 or 13, will be set later */
                .lep            = 6,
                .eptype         = EP_ISO_TYPE,
        },
        .ep[7] = {
                .ep = {
                        .name   = "ep7-int",
                        .ops    = &lpc32xx_ep_ops,
                },
                .udc            = &controller,
                .maxpacket      = 64,
                .hwep_num_base  = 14,
                .hwep_num       = 0,
                .lep            = 7,
                .eptype         = EP_INT_TYPE,
        },
        .ep[8] = {
                .ep = {
                        .name   = "ep8-bulk",
                        .ops    = &lpc32xx_ep_ops,
                },
                .udc            = &controller,
                .maxpacket      = 64,
                .hwep_num_base  = 16,
                .hwep_num       = 0,
                .lep            = 8,
                .eptype         = EP_BLK_TYPE,
        },
        .ep[9] = {
                .ep = {
                        .name   = "ep9-iso",
                        .ops    = &lpc32xx_ep_ops,
                },
                .udc            = &controller,
                .maxpacket      = 1023,
                .hwep_num_base  = 18,
                .hwep_num       = 0,
                .lep            = 9,
                .eptype         = EP_ISO_TYPE,
        },
        .ep[10] = {
                .ep = {
                        .name   = "ep10-int",
                        .ops    = &lpc32xx_ep_ops,
                },
                .udc            = &controller,
                .maxpacket      = 64,
                .hwep_num_base  = 20,
                .hwep_num       = 0,
                .lep            = 10,
                .eptype         = EP_INT_TYPE,
        },
        .ep[11] = {
                .ep = {
                        .name   = "ep11-bulk",
                        .ops    = &lpc32xx_ep_ops,
                },
                .udc            = &controller,
                .maxpacket      = 64,
                .hwep_num_base  = 22,
                .hwep_num       = 0,
                .lep            = 11,
                .eptype         = EP_BLK_TYPE,
        },
        .ep[12] = {
                .ep = {
                        .name   = "ep12-iso",
                        .ops    = &lpc32xx_ep_ops,
                },
                .udc            = &controller,
                .maxpacket      = 1023,
                .hwep_num_base  = 24,
                .hwep_num       = 0,
                .lep            = 12,
                .eptype         = EP_ISO_TYPE,
        },
        .ep[13] = {
                .ep = {
                        .name   = "ep13-int",
                        .ops    = &lpc32xx_ep_ops,
                },
                .udc            = &controller,
                .maxpacket      = 64,
                .hwep_num_base  = 26,
                .hwep_num       = 0,
                .lep            = 13,
                .eptype         = EP_INT_TYPE,
        },
        .ep[14] = {
                .ep = {
                        .name   = "ep14-bulk",
                        .ops    = &lpc32xx_ep_ops,
                },
                .udc            = &controller,
                .maxpacket      = 64,
                .hwep_num_base  = 28,
                .hwep_num       = 0,
                .lep            = 14,
                .eptype         = EP_BLK_TYPE,
        },
        .ep[15] = {
                .ep = {
                        .name   = "ep15-bulk",
                        .ops    = &lpc32xx_ep_ops,
                },
                .udc            = &controller,
                .maxpacket      = 1023,
                .hwep_num_base  = 30,
                .hwep_num       = 0,
                .lep            = 15,
                .eptype         = EP_BLK_TYPE,
        },
};

Re: [PATCH] usb: gadget: lpc32xx_udc: Port to new start/stop interface

[Sebastian Andrzej Siewior]

On 08/17/2012 01:32 PM, Roland Stigge wrote:
> How about the following: Below, I show how the initialization of the
> current controller is done (statically). Removing this struct
> initialization completely would make the code much uglier, introducing
> many individual assignments.

I said what I had to say, I leave it up to Felipe.

> static struct lpc32xx_udc controller = {
>          .gadget = {
>                  .ops    =&lpc32xx_udc_ops,
>                  .ep0    =&controller.ep[0].ep,
>                  .name   = driver_name,
>                  .dev    = {
>                          .init_name = "gadget",
>                          .release = nop_release,
>                  }
>          },
>          .ep[0] = {
>                  .ep = {
>                          .name   = "ep0",
>                          .ops    =&lpc32xx_ep_ops,
>                  },
>                  .udc            =&controller,
>                  .maxpacket      = 64,
>                  .hwep_num_base  = 0,
>                  .hwep_num       = 0, /* Can be 0 or 1, has special
> handling */

Can it be 0 or 1 or has it to be 0? Who would change and why?

>                  .lep            = 0,
>                  .eptype         = EP_CTL_TYPE,
>          },
>          .ep[1] = {
>                  .ep = {
>                          .name   = "ep1-int",
>                          .ops    =&lpc32xx_ep_ops,
>                  },
>                  .udc            =&controller,
>                  .maxpacket      = 64,
>                  .hwep_num_base  = 2,
>                  .hwep_num       = 0, /* 2 or 3, will be set later */

Why not now?

>                  .lep            = 1,
>                  .eptype         = EP_INT_TYPE,

Do you have any restrictions on these endpoints? I mean can you pick
any endpoints for BULK/INT/ISO or have they been pre-defined by HW?

Sebastian

Re: [PATCH] usb: gadget: lpc32xx_udc: Port to new start/stop interface

[Roland Stigge]

On 17/08/12 11:42, Sebastian Andrzej Siewior wrote:
> Some minor things:
> - please use to_udc() in start then
> - would it make sense to use platform_get_drvdata() in
>   lpc32xx_udc_shutdown() ?
> - could you please remove struct usb_endpoint_descriptor from struct
>   lpc32xx_ep? It has been removed a while back from other drivers.

I merged these changes into the patch set, and will repost it tomorrow
after I can test it more thoroughly.

Thanks,

Roland

Re: [PATCH] usb: gadget: lpc32xx_udc: Port to new start/stop interface

[Roland Stigge]

On 17/08/12 13:39, Sebastian Andrzej Siewior wrote:
>>                  .lep            = 0,
>>                  .eptype         = EP_CTL_TYPE,
>>          },
>>          .ep[1] = {
>>                  .ep = {
>>                          .name   = "ep1-int",
>>                          .ops    =&lpc32xx_ep_ops,
>>                  },
>>                  .udc            =&controller,
>>                  .maxpacket      = 64,
>>                  .hwep_num_base  = 2,
>>                  .hwep_num       = 0, /* 2 or 3, will be set later */
> 
> Why not now?

It's assigned dynamically at runtime, depending on RX or TX.

>>                  .lep            = 1,
>>                  .eptype         = EP_INT_TYPE,
> 
> Do you have any restrictions on these endpoints? I mean can you pick
> any endpoints for BULK/INT/ISO or have they been pre-defined by HW?

The latter, see LPC32xx user manual, 14.1.2.

Roland

Re: [PATCH] usb: gadget: lpc32xx_udc: Port to new start/stop interface

[Sebastian Andrzej Siewior]

On 08/19/2012 02:33 PM, Roland Stigge wrote:
> On 17/08/12 11:42, Sebastian Andrzej Siewior wrote:
>> Some minor things:
>> - please use to_udc() in start then
>> - would it make sense to use platform_get_drvdata() in
>>    lpc32xx_udc_shutdown() ?
>> - could you please remove struct usb_endpoint_descriptor from struct
>>    lpc32xx_ep? It has been removed a while back from other drivers.
>
> I merged these changes into the patch set, and will repost it tomorrow
> after I can test it more thoroughly.

Thanks.
While you are playing with it, I saw that you handle vbus (->pull())
manually. This was the case in the "old" start/stop case but in the new
one, the udc-core is / should handle it.

>
> Thanks,
>
> Roland

Sebastian

Re: [PATCH] usb: gadget: lpc32xx_udc: Port to new start/stop interface

[Roland Stigge]

Hi,

On 08/20/2012 09:07 AM, Sebastian Andrzej Siewior wrote:
>> I merged these changes into the patch set, and will repost it tomorrow
>> after I can test it more thoroughly.
> 
> Thanks.
> While you are playing with it, I saw that you handle vbus (->pull())
> manually. This was the case in the "old" start/stop case but in the new
> one, the udc-core is / should handle it.

I included the respective changes into my patch set that I'm posting
separately now.

Thanks for the note,

Roland