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

[Felipe Balbi <balbi@xxxxxx>]

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.

>  {
> -	/* 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.

-- 
balbi

Attachment: signature.asc
Description: Digital signature