[U-Boot] [PATCH 2/6] usb:g_dnl:f_usbd_thor: USB Download function to support THOR protocol

[Wolfgang Denk]

Dear Lukasz Majewski,

In message <1334215049-20362-3-git-send-email-l.majewski at samsung.com> you wrote:
> Implementation of USB Download function supporting THOR protocol.

Sorry, I hit "send" too fast...

> +	.bDescriptorSubtype = 0x00,	/* 0x00 */
> +	.bcdCDC =             0x0110,
...
> +	.bDescriptorType =    CS_INTERFACE, /* 0x24 */
> +	.bDescriptorSubtype = 0x01,	/* 0x01 */
> +	.bmCapabilities =     0x00,
> +	.bDataInterface =     0x01,
...
> +	.bDescriptorSubtype = 0x02,	/* 0x02 */
> +	.bmCapabilities =     0x00,
...
> +	.bDescriptorSubtype =  0x06,	/* 0x06 */
...
> +	.bEndpointAddress = 3 | USB_DIR_IN,
...
> +	.bInterval = 0x9,
...

There should be some header file with symbolic names for akll these
hard coded magic numbers.

> +};
> +
> +/*-------------------------------------------------------------------------*/
> +
> +

Drop that.   A single blank line is sufficient.

> +static struct usb_request *alloc_ep_req(struct usb_ep *ep, unsigned length)
> +{
> +	struct usb_request	*req;
> +
> +	req = usb_ep_alloc_request(ep, GFP_ATOMIC);
> +	if (req) {
> +		req->length = length;
> +		req->buf = malloc(length);
> +		if (!req->buf) {
> +			usb_ep_free_request(ep, req);
> +			req = NULL;
> +		}
> +	}
> +	return req;

Turn logic around to avoid deepo nesting of code - please apply
globally.  For example here:

	req = usb_ep_alloc_request(ep, GFP_ATOMIC);
	if (req == NULL)
		return NULL;

	req->buf = malloc(length);
	if (!req->buf) {
		usb_ep_free_request(ep, req);
		return NULL;
	}

	req->length = length;

	return req;

> +		debug("dev->out_req->length:%d dev->rxdata:%d\n",
> +		     dev->out_req->length, dev->rxdata);

Indentation by tAB only, please.


Also make sure to run your patches through checkpatch:

WARNING: space prohibited between function name and open parenthesis
'('
#588: FILE: drivers/usb/gadget/f_usbd_thor.c:465:
+__attribute__ ((__aligned__ (__alignof__ (long long))))

WARNING: space prohibited between function name and open parenthesis
'('
#588: FILE: drivers/usb/gadget/f_usbd_thor.c:465:
+__attribute__ ((__aligned__ (__alignof__ (long long))))

WARNING: __aligned(size) is preferred over
__attribute__((aligned(size)))
#588: FILE: drivers/usb/gadget/f_usbd_thor.c:465:
+__attribute__ ((__aligned__ (__alignof__ (long long))))

WARNING: space prohibited between function name and open parenthesis
'('
#590: FILE: drivers/usb/gadget/f_usbd_thor.c:467:
+__attribute__ ((__aligned__ (__alignof__ (long long))))

WARNING: space prohibited between function name and open parenthesis
'('
#590: FILE: drivers/usb/gadget/f_usbd_thor.c:467:
+__attribute__ ((__aligned__ (__alignof__ (long long))))

WARNING: __aligned(size) is preferred over
__attribute__((aligned(size)))
#590: FILE: drivers/usb/gadget/f_usbd_thor.c:467:
+__attribute__ ((__aligned__ (__alignof__ (long long))))



> +	switch (mode) {
> +	case END_BOOT:
> +		run_command("run bootcmd", 0);
> +		break;
> +	default:
> +		break;
> +	}

if ... else ... ?

> +	if (strncmp(usbd_rx_data_buf, recv_key, strlen(recv_key)) == 0) {
> +		printf("Download request from the Host PC\n");

Make this debug() ?

> +		msleep(30);

Are you sure it is a PC?

> +		strncpy(usbd_tx_data_buf, tx_key, strlen(tx_key));
> +		usbd_tx_data(usbd_tx_data_buf, strlen(tx_key));
> +	} else {
> +		puts("Wrong reply information\n");

It might be useful to print information about whart was received, and
what was expected?

> +		if (ret > 0) {
> +			ret = process_data(dnl);
> +
> +			if (ret < 0)
> +				return -1;
> +			else if (ret == 0)
> +				return 0;

Isn't this overkill?

			return process_data(dnl);

should be equivalent ?

> +	/* If the following pointers are set to NULL -> error */
> +	dnl->rx_buf = (unsigned int *) CONFIG_SYS_DOWN_ADDR;

Does it make sense to hardwire this address, and have it the same on
all boards?  Or would it be more useful if this could be changed by
the user (like by storing it in an environment variable) ?

> +	usb_downloader_intf_int.bInterfaceNumber = status;
> +	usb_downloader_cdc_union.bMasterInterface = status;

BTW:  CamelCaps names are forbidden in U-Boot.  Please fix globally.

Actually you should probably re-read the CodingStyle document and
re-think most variable names.

> +	/* note:  a status/notification endpoint is, strictly speaking,
> +	 * optional.  We don't treat it that way though!  It's simpler,
> +	 * and some newer profiles don't treat it as optional.
> +	 */

Incorrect multiline comment style.  Please fix globally.

> +	debug("%s: out_ep:%p out_req:%p\n",
> +	      __func__, dev->out_ep, dev->out_req);
> +	printf("%s: dnl: 0x%p\n", __func__, dnl);

Is this really useful and needed output for production mode?
Please check output globally, this seems to be way too verbose (OK for
testing, but not for production).

> +/* Samsung's IDs */
> +#define DRIVER_VENDOR_NUM 0x04E8
> +#define DRIVER_PRODUCT_NUM 0x6601
> +#define STRING_MANUFACTURER 25

Are there chances that other vendor / product / manufacurer ID's need
to be used here?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If I had to live my life again,  I'd  make  the  same  mistakes, only
sooner.                                          -- Tallulah Bankhead