[U-Boot] [PATCH 2/7] dfu:usb: DFU USB function (f_dfu) support for g_dnl composite gadget

[Marek Vasut]

Dear Lukasz Majewski,

> Support for f_dfu USB function.
> 
> Signed-off-by: Lukasz Majewski <l.majewski at samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
> Cc: Marek Vasut <marex at denx.de>
> ---
[...]
> +
> +static const char dfu_name[] = "Device Firmware Upgrade";
> +
> +/* static strings, in UTF-8 */
> +/*
> + * dfu_genericiguration specific


generi....what?

> + */
> +static struct usb_string strings_dfu_generic[] = {
> +	[0].s = dfu_name,
> +	{  }			/* end of list */
> +};
> +
> +static struct usb_gadget_strings stringtab_dfu_generic = {
> +	.language	= 0x0409,	/* en-us */
> +	.strings	= strings_dfu_generic,
> +};
> +
> +static struct usb_gadget_strings *dfu_generic_strings[] = {
> +	&stringtab_dfu_generic,
> +	NULL,
> +};
> +
> +/*
> + * usb_function specific
> + */
> +static struct usb_gadget_strings stringtab_dfu = {
> +	.language	= 0x0409,	/* en-us */
> +	/*
> +	 * .strings
> +	 *
> +	 * assigned during initialization,
> +	 * depends on number of flash entities
> +	 *
> +	 */
> +};
> +
> +static struct usb_gadget_strings *dfu_strings[] = {
> +	&stringtab_dfu,
> +	NULL,
> +};
> +
> +/*------------------------------------------------------------------------
> -*/ +
> +static void dnload_request_complete(struct usb_ep *ep, struct usb_request
> *req) +{
> +	struct f_dfu *f_dfu = req->context;
> +
> +	dfu_write(dfu_get_entity(f_dfu->altsetting), req->buf,
> +		  req->length, f_dfu->blk_seq_num);
> +
> +	if (req->length == 0) {
> +		puts("DOWNLOAD ... OK\n");
> +		puts("Ctrl+C to exit ...\n");
> +	}
> +}
> +
> +static void handle_getstatus(struct usb_request *req)
> +{
> +	struct dfu_status *dstat = (struct dfu_status *)req->buf;
> +	struct f_dfu *f_dfu = req->context;
> +
> +	switch (f_dfu->dfu_state) {
> +	case DFU_STATE_dfuDNLOAD_SYNC:

What's this crazy cammel case in here ? :-)

> +	case DFU_STATE_dfuDNBUSY:
> +		f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_IDLE;
> +		break;
> +	case DFU_STATE_dfuMANIFEST_SYNC:
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	/* send status response */
> +	dstat->bStatus = f_dfu->dfu_status;
> +	/* FIXME: set dstat->bwPollTimeout */

FIXME ... so fix it ;-)

> +	dstat->bState = f_dfu->dfu_state;
> +	dstat->iString = 0;
> +}
> +
> +static void handle_getstate(struct usb_request *req)
> +{
> +	struct f_dfu *f_dfu = req->context;
> +
> +	((u8 *)req->buf)[0] = f_dfu->dfu_state & 0xff;

Now ... this is ubercrazy ... can't this be made without this typecasting 
voodoo?

> +	req->actual = sizeof(u8);
> +}
> +
[...]
> +static int handle_dnload(struct usb_gadget *gadget, u16 len)
> +{
> +	struct usb_composite_dev *cdev = get_gadget_data(gadget);
> +	struct usb_request *req = cdev->req;
> +	struct f_dfu *f_dfu = req->context;
> +
> +	if (len == 0)
> +		f_dfu->dfu_state = DFU_STATE_dfuMANIFEST_SYNC;
> +
> +	req->complete = dnload_request_complete;
> +
> +	return len;
> +}

One newline too much below.

> +
> +
> +static int
> +dfu_handle(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
> +{
> +	struct usb_gadget *gadget = f->config->cdev->gadget;
> +	struct usb_request *req = f->config->cdev->req;
> +	struct f_dfu *f_dfu = f->config->cdev->req->context;
> +	u16 len = le16_to_cpu(ctrl->wLength);
> +	u16 w_value = le16_to_cpu(ctrl->wValue);
> +	int value = 0;
> +	int standard;
> +
> +	standard = (ctrl->bRequestType & USB_TYPE_MASK)
> +						== USB_TYPE_STANDARD;
> +
> +	debug("w_value: 0x%x len: 0x%x\n", w_value, len);
> +	debug("standard: 0x%x ctrl->bRequest: 0x%x f_dfu->dfu_state: 0x%x\n",
> +	       standard, ctrl->bRequest, f_dfu->dfu_state);
> +
> +	if (!standard) {

This function doesn't fit on my screen ... that means it's waaaay too long and 
shall be split into more functions ;-)

That's also remove the need for your crazy conditional assignment of standard.

> +		switch (f_dfu->dfu_state) {
> +		case DFU_STATE_appIDLE:
> +			switch (ctrl->bRequest) {
> +			case USB_REQ_DFU_GETSTATUS:
> +				handle_getstatus(req);
> +				value = RET_STAT_LEN;
> +				break;
[...]

> +
> +/*------------------------------------------------------------------------
> -*/ +
> +static int
> +dfu_prepare_strings(struct f_dfu *f_dfu, int n)
> +{
> +	struct dfu_entity *de = NULL;
> +	int i = 0;
> +
> +	f_dfu->strings = calloc(((n + 1) * sizeof(struct usb_string)), 1);

calloc(n, 1) ... that's the same as malloc(), isn't it ?

> +	if (!f_dfu->strings)
> +		goto enomem;
> +
> +	for (i = 0; i < n; ++i) {
> +		de = dfu_get_entity(i);
> +		f_dfu->strings[i].s = de->name;
> +	}
> +
> +	f_dfu->strings[i].id = 0;
> +	f_dfu->strings[i].s = NULL;
> +
> +	return 0;
> +
> +enomem:
> +	while (i)
> +		f_dfu->strings[--i].s = NULL;
> +
> +	kfree(f_dfu->strings);
> +
> +	return -ENOMEM;
> +}
> +
> +static int dfu_prepare_function(struct f_dfu *f_dfu, int n)
> +{
> +	struct usb_interface_descriptor *d;
> +	int i = 0;
> +
> +	f_dfu->function = calloc(n * sizeof(struct usb_descriptor_header *), 1);

DITTO

> +	if (!f_dfu->function)
> +		goto enomem;
> +
> +	for (i = 0; i < n; ++i) {
> +		d = kzalloc(sizeof(*d), GFP_KERNEL);

Why use the kernel alternatives ? just use malloc()/free() ?

> +		if (!d)
> +			goto enomem;
> +
> +		d->bLength =		sizeof(*d);
> +		d->bDescriptorType =	USB_DT_INTERFACE;
> +		d->bAlternateSetting =	i;
> +		d->bNumEndpoints =	0;
> +		d->bInterfaceClass =	USB_CLASS_APP_SPEC;
> +		d->bInterfaceSubClass =	1;
> +		d->bInterfaceProtocol =	2;
> +
> +		f_dfu->function[i] = (struct usb_descriptor_header *)d;
> +	}
> +	f_dfu->function[i] = NULL;
> +
> +	return 0;
> +
> +enomem:
> +	while (i) {
> +		kfree(f_dfu->function[--i]);
> +		f_dfu->function[i] = NULL;
> +	}
> +	kfree(f_dfu->function);
> +
> +	return -ENOMEM;
> +}
> +
> +static int dfu_bind(struct usb_configuration *c, struct usb_function *f)
> +{
> +	struct usb_composite_dev *cdev = c->cdev;
> +	struct f_dfu *f_dfu = func_to_dfu(f);
> +	int alt_num = dfu_get_alt_number();
> +	int rv, id, i;
> +
> +	id = usb_interface_id(c, f);
> +	if (id < 0)
> +		return id;
> +	dfu_intf_runtime.bInterfaceNumber = id;
> +
> +	f_dfu->dfu_state = DFU_STATE_appIDLE;
> +	f_dfu->dfu_status = DFU_STATUS_OK;
> +
> +	rv = dfu_prepare_function(f_dfu, alt_num);
> +	if (rv)
> +		goto error;
> +
> +	rv = dfu_prepare_strings(f_dfu, alt_num);
> +	if (rv)
> +		goto error;
> +	for (i = 0; i < alt_num; i++) {
> +		id = usb_string_id(cdev);
> +		if (id < 0)
> +			return id;
> +		f_dfu->strings[i].id = id;
> +		((struct usb_interface_descriptor *)f_dfu->function[i])
> +			->iInterface = id;
> +	}
> +
> +	stringtab_dfu.strings = f_dfu->strings;
> +
> +	cdev->req->context = f_dfu;
> +
> +	return 0;
> +error:
> +	return rv;

So just return the retval ...

> +}
> +

Every time I review patches and find multiple small issues, I feel bad :-(