Re: [PATCH v4 13/24] usb: gadget: ci13xxx: don't use "advance" feature when setting address

[Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>]

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