On Thu, 3 May 2012 15:51:56 +0300, Felipe Balbi <balbi@xxxxxx> wrote:
> On Wed, May 02, 2012 at 05:13:42PM +0300, Alexander Shishkin wrote:
> > Newer versions of the chipidea controller support the "advance" setting
> > of usb address, which means instead of setting it immediately, deferring
> > it till the status completion. Unfortunately, older versions of the
> > controller don't have this feature, so in order to support those too, we
> > simply don't use it. It's about 4 lines of extra code, and isn't in any
> > way critical to performance.
>
> in fact this is what specification asks:
>
> "The device does not change its device address until after the Status
> stage of this request is completed successfully."
>
> > With this patch, ci13xxx_udc driver works with the chipidea controller in
> > kirkwood (feroceon SoC), as found in, for example, sheevaplug.
> >
> > Signed-off-by: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> > ---
> > drivers/usb/gadget/ci13xxx_udc.c | 22 ++++++++++++----------
> > drivers/usb/gadget/ci13xxx_udc.h | 1 +
> > 2 files changed, 13 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/ci13xxx_udc.c b/drivers/usb/gadget/ci13xxx_udc.c
> > index 8802906..1da415f 100644
> > --- a/drivers/usb/gadget/ci13xxx_udc.c
> > +++ b/drivers/usb/gadget/ci13xxx_udc.c
> > @@ -722,14 +722,13 @@ static int hw_test_and_set_setup_guard(struct ci13xxx *udc)
> > * hw_usb_set_address: configures USB address (execute without interruption)
> > * @value: new USB address
> > *
> > - * This function returns an error code
> > + * This function explicitly sets the address, without the "USBADRA" (advance)
> > + * feature, which is not supported by older versions of the controller.
> > */
> > -static int hw_usb_set_address(struct ci13xxx *udc, u8 value)
> > +static void hw_usb_set_address(struct ci13xxx *udc, u8 value)
>
> changing return from int to void isn't part of $SUBJECT.
Do you really think it's worth a separate patch? No problem with me, but
I'm already feeling bad for spamming the list.
> > {
> > - /* advance */
> > - hw_write(udc, OP_DEVICEADDR, DEVICEADDR_USBADR | DEVICEADDR_USBADRA,
> > - value << ffs_nr(DEVICEADDR_USBADR) | DEVICEADDR_USBADRA);
> > - return 0;
> > + hw_write(udc, OP_DEVICEADDR, DEVICEADDR_USBADR,
> > + value << ffs_nr(DEVICEADDR_USBADR));
> > }
> >
> > /**
> > @@ -1831,6 +1830,11 @@ isr_setup_status_complete(struct usb_ep *ep, struct usb_request *req)
> >
> > trace(udc->dev, "%s, %p", ep->name, req);
> >
> > + if (udc->addr_to_set) {
> > + hw_usb_set_address(udc, udc->addr_to_set);
> > + udc->addr_to_set = 0;
> > + }
>
> this won't work always. A SetAddress with address zero is valid and will
> put the device into Default State. This is used at least by USB{20,30}CV
> tools. I guess it's better to use a flag together with a field to hold
> the address.
Ok, thanks, will fix.
Regards,
--
Alex
--
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
