[U-Boot] [PATCH v3 6/9] dfu: Send correct DFU response from composite_setup

[Lukasz Majewski]

Hi Pantelis,

> DFU is a bit peculiar. It needs to hook to composite setup and
> return it's function descriptor.
> 
> So when get-descriptor request comes with a type of DFU_DT_FUNC
> we iterate over the configs, and functions, and when we find
> the DFU function we call the setup method which is prepared
> to return the appropriate function descriptor.

Sorry, but could you be more informative here? Have you had any non
standard problems? I wonder why dfu-util on my linux works OK without
this patch?

> 
> Signed-off-by: Pantelis Antoniou <panto at antoniou-consulting.com>
> ---
>  drivers/usb/gadget/composite.c | 27 +++++++++++++++++++++++++++
>  drivers/usb/gadget/ep0.c       |  1 +
>  drivers/usb/gadget/f_dfu.c     |  6 ++++--
>  3 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/composite.c
> b/drivers/usb/gadget/composite.c index ebb5131..6496436 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -773,6 +773,33 @@ composite_setup(struct usb_gadget *gadget, const
> struct usb_ctrlrequest *ctrl) if (value >= 0)
>  				value = min(w_length, (u16) value);
>  			break;
> +
> +#ifdef CONFIG_DFU_FUNCTION
> +		/* DFU is mighty weird */
				^^^^^^ - please explain this
				"wiredness". 

I don't recall such a hacks in linux kernel composite.c (any special
#ifdef). Am I missing something important in DFU?


Does your device have any special requirement, so you need this hack?

I generally don't like the idea to "patch" composite gadget code with
#ifdefs for special function. Please convince me.

> +		case DFU_DT_FUNC:
> +			w_value &= 0xff;
> +			list_for_each_entry(c, &cdev->configs, list)
> {
> +				if (w_value != 0) {
> +					w_value--;
> +					continue;
> +				}
> +
> +				list_for_each_entry(f,
> &c->functions, list) { +
> +					/* DFU function only */
> +					if (strcmp(f->name,
> "dfu") != 0)
> +						continue;
> +
> +					value = f->setup(f, ctrl);
> +					goto dfu_func_done;
> +				}
> +			}
> +dfu_func_done:
> +			if (value >= 0)
> +				value = min(w_length, (u16) value);
> +			break;
> +#endif
> +
>  		default:
>  			goto unknown;
>  		}
> diff --git a/drivers/usb/gadget/ep0.c b/drivers/usb/gadget/ep0.c
> index aa8f916..971d846 100644
> --- a/drivers/usb/gadget/ep0.c
> +++ b/drivers/usb/gadget/ep0.c
> @@ -221,6 +221,7 @@ static int ep0_get_descriptor (struct
> usb_device_instance *device, break;
>  
>  	case USB_DESCRIPTOR_TYPE_CONFIGURATION:
> +	case USB_DESCRIPTOR_TYPE_OTHER_SPEED_CONFIGURATION:
				^^^^^^^^^^^^^- why do you need that?
>  		{
>  			struct usb_configuration_descriptor
>  				*configuration_descriptor;
> diff --git a/drivers/usb/gadget/f_dfu.c b/drivers/usb/gadget/f_dfu.c
> index 10547e3..6494f5e 100644
> --- a/drivers/usb/gadget/f_dfu.c
> +++ b/drivers/usb/gadget/f_dfu.c
> @@ -534,8 +534,10 @@ dfu_handle(struct usb_function *f, const struct
> usb_ctrlrequest *ctrl) value = min(len, (u16) sizeof(dfu_func));
>  			memcpy(req->buf, &dfu_func, value);
>  		}
> -	} else /* DFU specific request */
> -		value = dfu_state[f_dfu->dfu_state] (f_dfu, ctrl,
> gadget, req);
> +		return value;
> +	}
> +
> +	value = dfu_state[f_dfu->dfu_state] (f_dfu, ctrl, gadget,
> req); 

Why do you change state even after receiving req_type ==
USB_TYPE_STANDARD? I would expect to change the dfu state only when DFU
specific request appears.



>  	if (value >= 0) {
>  		req->length = value;



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group